* [PATCH] BaseTools/tools_def AARCH64: move GCC49/GGC5 to 4 KB section alignment
@ 2018-12-10 14:13 Ard Biesheuvel
2018-12-10 18:08 ` Laszlo Ersek
2018-12-10 19:06 ` Leif Lindholm
0 siblings, 2 replies; 7+ messages in thread
From: Ard Biesheuvel @ 2018-12-10 14:13 UTC (permalink / raw)
To: edk2-devel; +Cc: leif.lindholm, liming.gao, Ard Biesheuvel
Since 4 KB section alignment is required when mapping PE/COFF images
with strict permissions, update the default section alignment when
using GCC49 and GCC5 in RELEASE mode. Note that XIP modules such as
SEC, PEIMs or PEI core are not affected by this change, since the
override to 32 byte aligment remains in effect.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
BaseTools/Conf/tools_def.template | 24 +++++++++++---------
1 file changed, 13 insertions(+), 11 deletions(-)
diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def.template
index e0e68fd7fb49..5d34333dc54f 100755
--- a/BaseTools/Conf/tools_def.template
+++ b/BaseTools/Conf/tools_def.template
@@ -4263,11 +4263,11 @@ DEFINE GCC49_ARM_ASM_FLAGS = DEF(GCC48_ARM_ASM_FLAGS)
DEFINE GCC49_AARCH64_ASM_FLAGS = DEF(GCC48_AARCH64_ASM_FLAGS)
DEFINE GCC49_ARM_CC_FLAGS = DEF(GCC48_ARM_CC_FLAGS)
DEFINE GCC49_ARM_CC_XIPFLAGS = DEF(GCC48_ARM_CC_XIPFLAGS)
-DEFINE GCC49_AARCH64_CC_FLAGS = $(ARCHCC_FLAGS) $(PLATFORM_FLAGS) DEF(GCC44_ALL_CC_FLAGS) DEF(GCC_AARCH64_CC_FLAGS)
+DEFINE GCC49_AARCH64_CC_FLAGS = $(ARCHCC_FLAGS) $(PLATFORM_FLAGS) DEF(GCC44_ALL_CC_FLAGS) DEF(GCC_AARCH64_CC_FLAGS) -mcmodel=small
DEFINE GCC49_AARCH64_CC_XIPFLAGS = DEF(GCC48_AARCH64_CC_XIPFLAGS)
DEFINE GCC49_ARM_DLINK_FLAGS = DEF(GCC48_ARM_DLINK_FLAGS)
DEFINE GCC49_ARM_DLINK2_FLAGS = DEF(GCC48_ARM_DLINK2_FLAGS)
-DEFINE GCC49_AARCH64_DLINK_FLAGS = DEF(GCC48_AARCH64_DLINK_FLAGS)
+DEFINE GCC49_AARCH64_DLINK_FLAGS = DEF(GCC48_AARCH64_DLINK_FLAGS) -z common-page-size=0x1000
DEFINE GCC49_AARCH64_DLINK2_FLAGS = DEF(GCC48_AARCH64_DLINK2_FLAGS)
DEFINE GCC49_ARM_ASLDLINK_FLAGS = DEF(GCC48_ARM_ASLDLINK_FLAGS)
DEFINE GCC49_AARCH64_ASLDLINK_FLAGS = DEF(GCC48_AARCH64_ASLDLINK_FLAGS)
@@ -5034,15 +5034,16 @@ RELEASE_GCC49_ARM_CC_FLAGS = DEF(GCC49_ARM_CC_FLAGS) -Wno-unused-but-set-v
*_GCC49_AARCH64_VFRPP_FLAGS = $(ARCHCC_FLAGS) $(PLATFORM_FLAGS) DEF(GCC_VFRPP_FLAGS)
*_GCC49_AARCH64_CC_XIPFLAGS = DEF(GCC49_AARCH64_CC_XIPFLAGS)
- DEBUG_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -O0 -mcmodel=small
- DEBUG_GCC49_AARCH64_DLINK_FLAGS = DEF(GCC49_AARCH64_DLINK_FLAGS) -z common-page-size=0x1000
+ DEBUG_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -O0
+ DEBUG_GCC49_AARCH64_DLINK_FLAGS = DEF(GCC49_AARCH64_DLINK_FLAGS)
DEBUG_GCC49_AARCH64_DLINK_XIPFLAGS = -z common-page-size=0x20
-RELEASE_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -Wno-unused-but-set-variable -Wno-unused-const-variable -mcmodel=tiny
+RELEASE_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -Wno-unused-but-set-variable -Wno-unused-const-variable
RELEASE_GCC49_AARCH64_DLINK_FLAGS = DEF(GCC49_AARCH64_DLINK_FLAGS)
+RELEASE_GCC49_AARCH64_DLINK_XIPFLAGS = -z common-page-size=0x20
- NOOPT_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -O0 -mcmodel=small
- NOOPT_GCC49_AARCH64_DLINK_FLAGS = DEF(GCC49_AARCH64_DLINK_FLAGS) -z common-page-size=0x1000 -O0
+ NOOPT_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -O0
+ NOOPT_GCC49_AARCH64_DLINK_FLAGS = DEF(GCC49_AARCH64_DLINK_FLAGS) -O0
NOOPT_GCC49_AARCH64_DLINK_XIPFLAGS = -z common-page-size=0x20 -O0
####################################################################################
@@ -5189,14 +5190,15 @@ RELEASE_GCC5_ARM_DLINK_FLAGS = DEF(GCC5_ARM_DLINK_FLAGS) -flto -Os -L$(WORKS
*_GCC5_AARCH64_VFRPP_FLAGS = $(ARCHCC_FLAGS) $(PLATFORM_FLAGS) DEF(GCC_VFRPP_FLAGS)
*_GCC5_AARCH64_CC_XIPFLAGS = DEF(GCC5_AARCH64_CC_XIPFLAGS)
- DEBUG_GCC5_AARCH64_CC_FLAGS = DEF(GCC5_AARCH64_CC_FLAGS) -flto -Wno-unused-but-set-variable -Wno-unused-const-variable -mcmodel=small
- DEBUG_GCC5_AARCH64_DLINK_FLAGS = DEF(GCC5_AARCH64_DLINK_FLAGS) -z common-page-size=0x1000 -flto -Os -L$(WORKSPACE)/ArmPkg/Library/GccLto -llto-aarch64 -Wl,-plugin-opt=-pass-through=-llto-aarch64 -Wno-lto-type-mismatch -mcmodel=small
+ DEBUG_GCC5_AARCH64_CC_FLAGS = DEF(GCC5_AARCH64_CC_FLAGS) -flto -Wno-unused-but-set-variable -Wno-unused-const-variable
+ DEBUG_GCC5_AARCH64_DLINK_FLAGS = DEF(GCC5_AARCH64_DLINK_FLAGS) -flto -Os -L$(WORKSPACE)/ArmPkg/Library/GccLto -llto-aarch64 -Wl,-plugin-opt=-pass-through=-llto-aarch64 -Wno-lto-type-mismatch
DEBUG_GCC5_AARCH64_DLINK_XIPFLAGS = -z common-page-size=0x20
-RELEASE_GCC5_AARCH64_CC_FLAGS = DEF(GCC5_AARCH64_CC_FLAGS) -flto -Wno-unused-but-set-variable -Wno-unused-const-variable -mcmodel=tiny
+RELEASE_GCC5_AARCH64_CC_FLAGS = DEF(GCC5_AARCH64_CC_FLAGS) -flto -Wno-unused-but-set-variable -Wno-unused-const-variable
RELEASE_GCC5_AARCH64_DLINK_FLAGS = DEF(GCC5_AARCH64_DLINK_FLAGS) -flto -Os -L$(WORKSPACE)/ArmPkg/Library/GccLto -llto-aarch64 -Wl,-plugin-opt=-pass-through=-llto-aarch64 -Wno-lto-type-mismatch
+RELEASE_GCC5_AARCH64_DLINK_XIPFLAGS = -z common-page-size=0x20
- NOOPT_GCC5_AARCH64_CC_FLAGS = DEF(GCC5_AARCH64_CC_FLAGS) -O0 -mcmodel=small
+ NOOPT_GCC5_AARCH64_CC_FLAGS = DEF(GCC5_AARCH64_CC_FLAGS) -O0
NOOPT_GCC5_AARCH64_DLINK_FLAGS = DEF(GCC5_AARCH64_DLINK_FLAGS) -z common-page-size=0x1000 -O0
NOOPT_GCC5_AARCH64_DLINK_XIPFLAGS = -z common-page-size=0x20 -O0
--
2.19.2
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] BaseTools/tools_def AARCH64: move GCC49/GGC5 to 4 KB section alignment
2018-12-10 14:13 [PATCH] BaseTools/tools_def AARCH64: move GCC49/GGC5 to 4 KB section alignment Ard Biesheuvel
@ 2018-12-10 18:08 ` Laszlo Ersek
2018-12-10 18:12 ` Ard Biesheuvel
2018-12-10 19:06 ` Leif Lindholm
1 sibling, 1 reply; 7+ messages in thread
From: Laszlo Ersek @ 2018-12-10 18:08 UTC (permalink / raw)
To: Ard Biesheuvel, edk2-devel; +Cc: liming.gao
On 12/10/18 15:13, Ard Biesheuvel wrote:
> Since 4 KB section alignment is required when mapping PE/COFF images
> with strict permissions, update the default section alignment when
> using GCC49 and GCC5 in RELEASE mode. Note that XIP modules such as
> SEC, PEIMs or PEI core are not affected by this change, since the
> override to 32 byte aligment remains in effect.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> BaseTools/Conf/tools_def.template | 24 +++++++++++---------
> 1 file changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def.template
> index e0e68fd7fb49..5d34333dc54f 100755
> --- a/BaseTools/Conf/tools_def.template
> +++ b/BaseTools/Conf/tools_def.template
> @@ -4263,11 +4263,11 @@ DEFINE GCC49_ARM_ASM_FLAGS = DEF(GCC48_ARM_ASM_FLAGS)
> DEFINE GCC49_AARCH64_ASM_FLAGS = DEF(GCC48_AARCH64_ASM_FLAGS)
> DEFINE GCC49_ARM_CC_FLAGS = DEF(GCC48_ARM_CC_FLAGS)
> DEFINE GCC49_ARM_CC_XIPFLAGS = DEF(GCC48_ARM_CC_XIPFLAGS)
> -DEFINE GCC49_AARCH64_CC_FLAGS = $(ARCHCC_FLAGS) $(PLATFORM_FLAGS) DEF(GCC44_ALL_CC_FLAGS) DEF(GCC_AARCH64_CC_FLAGS)
> +DEFINE GCC49_AARCH64_CC_FLAGS = $(ARCHCC_FLAGS) $(PLATFORM_FLAGS) DEF(GCC44_ALL_CC_FLAGS) DEF(GCC_AARCH64_CC_FLAGS) -mcmodel=small
> DEFINE GCC49_AARCH64_CC_XIPFLAGS = DEF(GCC48_AARCH64_CC_XIPFLAGS)
> DEFINE GCC49_ARM_DLINK_FLAGS = DEF(GCC48_ARM_DLINK_FLAGS)
> DEFINE GCC49_ARM_DLINK2_FLAGS = DEF(GCC48_ARM_DLINK2_FLAGS)
> -DEFINE GCC49_AARCH64_DLINK_FLAGS = DEF(GCC48_AARCH64_DLINK_FLAGS)
> +DEFINE GCC49_AARCH64_DLINK_FLAGS = DEF(GCC48_AARCH64_DLINK_FLAGS) -z common-page-size=0x1000
> DEFINE GCC49_AARCH64_DLINK2_FLAGS = DEF(GCC48_AARCH64_DLINK2_FLAGS)
> DEFINE GCC49_ARM_ASLDLINK_FLAGS = DEF(GCC48_ARM_ASLDLINK_FLAGS)
> DEFINE GCC49_AARCH64_ASLDLINK_FLAGS = DEF(GCC48_AARCH64_ASLDLINK_FLAGS)
> @@ -5034,15 +5034,16 @@ RELEASE_GCC49_ARM_CC_FLAGS = DEF(GCC49_ARM_CC_FLAGS) -Wno-unused-but-set-v
> *_GCC49_AARCH64_VFRPP_FLAGS = $(ARCHCC_FLAGS) $(PLATFORM_FLAGS) DEF(GCC_VFRPP_FLAGS)
> *_GCC49_AARCH64_CC_XIPFLAGS = DEF(GCC49_AARCH64_CC_XIPFLAGS)
>
> - DEBUG_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -O0 -mcmodel=small
> - DEBUG_GCC49_AARCH64_DLINK_FLAGS = DEF(GCC49_AARCH64_DLINK_FLAGS) -z common-page-size=0x1000
> + DEBUG_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -O0
> + DEBUG_GCC49_AARCH64_DLINK_FLAGS = DEF(GCC49_AARCH64_DLINK_FLAGS)
> DEBUG_GCC49_AARCH64_DLINK_XIPFLAGS = -z common-page-size=0x20
>
> -RELEASE_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -Wno-unused-but-set-variable -Wno-unused-const-variable -mcmodel=tiny
> +RELEASE_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -Wno-unused-but-set-variable -Wno-unused-const-variable
> RELEASE_GCC49_AARCH64_DLINK_FLAGS = DEF(GCC49_AARCH64_DLINK_FLAGS)
> +RELEASE_GCC49_AARCH64_DLINK_XIPFLAGS = -z common-page-size=0x20
>
> - NOOPT_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -O0 -mcmodel=small
> - NOOPT_GCC49_AARCH64_DLINK_FLAGS = DEF(GCC49_AARCH64_DLINK_FLAGS) -z common-page-size=0x1000 -O0
> + NOOPT_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -O0
> + NOOPT_GCC49_AARCH64_DLINK_FLAGS = DEF(GCC49_AARCH64_DLINK_FLAGS) -O0
> NOOPT_GCC49_AARCH64_DLINK_XIPFLAGS = -z common-page-size=0x20 -O0
>
> ####################################################################################
> @@ -5189,14 +5190,15 @@ RELEASE_GCC5_ARM_DLINK_FLAGS = DEF(GCC5_ARM_DLINK_FLAGS) -flto -Os -L$(WORKS
> *_GCC5_AARCH64_VFRPP_FLAGS = $(ARCHCC_FLAGS) $(PLATFORM_FLAGS) DEF(GCC_VFRPP_FLAGS)
> *_GCC5_AARCH64_CC_XIPFLAGS = DEF(GCC5_AARCH64_CC_XIPFLAGS)
>
> - DEBUG_GCC5_AARCH64_CC_FLAGS = DEF(GCC5_AARCH64_CC_FLAGS) -flto -Wno-unused-but-set-variable -Wno-unused-const-variable -mcmodel=small
> - DEBUG_GCC5_AARCH64_DLINK_FLAGS = DEF(GCC5_AARCH64_DLINK_FLAGS) -z common-page-size=0x1000 -flto -Os -L$(WORKSPACE)/ArmPkg/Library/GccLto -llto-aarch64 -Wl,-plugin-opt=-pass-through=-llto-aarch64 -Wno-lto-type-mismatch -mcmodel=small
> + DEBUG_GCC5_AARCH64_CC_FLAGS = DEF(GCC5_AARCH64_CC_FLAGS) -flto -Wno-unused-but-set-variable -Wno-unused-const-variable
> + DEBUG_GCC5_AARCH64_DLINK_FLAGS = DEF(GCC5_AARCH64_DLINK_FLAGS) -flto -Os -L$(WORKSPACE)/ArmPkg/Library/GccLto -llto-aarch64 -Wl,-plugin-opt=-pass-through=-llto-aarch64 -Wno-lto-type-mismatch
> DEBUG_GCC5_AARCH64_DLINK_XIPFLAGS = -z common-page-size=0x20
>
> -RELEASE_GCC5_AARCH64_CC_FLAGS = DEF(GCC5_AARCH64_CC_FLAGS) -flto -Wno-unused-but-set-variable -Wno-unused-const-variable -mcmodel=tiny
> +RELEASE_GCC5_AARCH64_CC_FLAGS = DEF(GCC5_AARCH64_CC_FLAGS) -flto -Wno-unused-but-set-variable -Wno-unused-const-variable
> RELEASE_GCC5_AARCH64_DLINK_FLAGS = DEF(GCC5_AARCH64_DLINK_FLAGS) -flto -Os -L$(WORKSPACE)/ArmPkg/Library/GccLto -llto-aarch64 -Wl,-plugin-opt=-pass-through=-llto-aarch64 -Wno-lto-type-mismatch
> +RELEASE_GCC5_AARCH64_DLINK_XIPFLAGS = -z common-page-size=0x20
>
> - NOOPT_GCC5_AARCH64_CC_FLAGS = DEF(GCC5_AARCH64_CC_FLAGS) -O0 -mcmodel=small
> + NOOPT_GCC5_AARCH64_CC_FLAGS = DEF(GCC5_AARCH64_CC_FLAGS) -O0
> NOOPT_GCC5_AARCH64_DLINK_FLAGS = DEF(GCC5_AARCH64_DLINK_FLAGS) -z common-page-size=0x1000 -O0
> NOOPT_GCC5_AARCH64_DLINK_XIPFLAGS = -z common-page-size=0x20 -O0
>
>
Looking at this patch with "--word-diff" helps a little (it's too bad we
can't break these mile long option sequences to multiple lines). So:
* For GCC49:
- The compiler option "-mcmodel=small" is upstreamed from both
NOOPT_GCC49_AARCH64_CC_FLAGS and DEBUG_GCC49_AARCH64_CC_FLAGS to
GCC49_AARCH64_CC_FLAGS. This is a no-op for DEBUG and NOOPT, however the
option will now also apply to RELEASE.
- As a consequence, "-mcmodel=tiny" is not usable for RELEASE, and it is
removed.
- The same upstreaming occurs for "-z common-page-size=0x1000", in
GCC49_AARCH64_DLINK_FLAGS. RELEASE needs no fixup, it simply benefits
from the change. However, RELEASE_GCC49_AARCH64_DLINK_XIPFLAGS needs an
override, to stick with the original 0x20 alignment.
* For GCC5:
- GCC5_AARCH64_CC_FLAGS is defined as GCC49_AARCH64_CC_FLAGS, therefore
the "-mcmodel=small" inheritance from the above is automatic. Only the
DEBUG/NOOPT removals, and the RELEASE fixup are needed.
- GCC5_AARCH64_DLINK_FLAGS is defined as GCC49_AARCH64_DLINK_FLAGS,
therefore the same inheritance pattern applies to "-z
common-page-size=0x1000". I'm noticing an omission here however: "-z
common-page-size=0x1000" should have been removed from
NOOPT_GCC5_AARCH64_DLINK_FLAGS as well.
- The RELEASE_GCC5_AARCH64_DLINK_XIPFLAGS override is spelled out the same.
So... do you agree that NOOPT_GCC5_AARCH64_DLINK_FLAGS should have been
trimmed as well?
Furthermore, I suggest squeezing the word RELEASE into the subject,
given that the change is only observable in RELEASE builds.
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] BaseTools/tools_def AARCH64: move GCC49/GGC5 to 4 KB section alignment
2018-12-10 18:08 ` Laszlo Ersek
@ 2018-12-10 18:12 ` Ard Biesheuvel
2018-12-10 18:19 ` Laszlo Ersek
0 siblings, 1 reply; 7+ messages in thread
From: Ard Biesheuvel @ 2018-12-10 18:12 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: edk2-devel@lists.01.org, Gao, Liming
On Mon, 10 Dec 2018 at 19:08, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 12/10/18 15:13, Ard Biesheuvel wrote:
> > Since 4 KB section alignment is required when mapping PE/COFF images
> > with strict permissions, update the default section alignment when
> > using GCC49 and GCC5 in RELEASE mode. Note that XIP modules such as
> > SEC, PEIMs or PEI core are not affected by this change, since the
> > override to 32 byte aligment remains in effect.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> > BaseTools/Conf/tools_def.template | 24 +++++++++++---------
> > 1 file changed, 13 insertions(+), 11 deletions(-)
> >
> > diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def.template
> > index e0e68fd7fb49..5d34333dc54f 100755
> > --- a/BaseTools/Conf/tools_def.template
> > +++ b/BaseTools/Conf/tools_def.template
> > @@ -4263,11 +4263,11 @@ DEFINE GCC49_ARM_ASM_FLAGS = DEF(GCC48_ARM_ASM_FLAGS)
> > DEFINE GCC49_AARCH64_ASM_FLAGS = DEF(GCC48_AARCH64_ASM_FLAGS)
> > DEFINE GCC49_ARM_CC_FLAGS = DEF(GCC48_ARM_CC_FLAGS)
> > DEFINE GCC49_ARM_CC_XIPFLAGS = DEF(GCC48_ARM_CC_XIPFLAGS)
> > -DEFINE GCC49_AARCH64_CC_FLAGS = $(ARCHCC_FLAGS) $(PLATFORM_FLAGS) DEF(GCC44_ALL_CC_FLAGS) DEF(GCC_AARCH64_CC_FLAGS)
> > +DEFINE GCC49_AARCH64_CC_FLAGS = $(ARCHCC_FLAGS) $(PLATFORM_FLAGS) DEF(GCC44_ALL_CC_FLAGS) DEF(GCC_AARCH64_CC_FLAGS) -mcmodel=small
> > DEFINE GCC49_AARCH64_CC_XIPFLAGS = DEF(GCC48_AARCH64_CC_XIPFLAGS)
> > DEFINE GCC49_ARM_DLINK_FLAGS = DEF(GCC48_ARM_DLINK_FLAGS)
> > DEFINE GCC49_ARM_DLINK2_FLAGS = DEF(GCC48_ARM_DLINK2_FLAGS)
> > -DEFINE GCC49_AARCH64_DLINK_FLAGS = DEF(GCC48_AARCH64_DLINK_FLAGS)
> > +DEFINE GCC49_AARCH64_DLINK_FLAGS = DEF(GCC48_AARCH64_DLINK_FLAGS) -z common-page-size=0x1000
> > DEFINE GCC49_AARCH64_DLINK2_FLAGS = DEF(GCC48_AARCH64_DLINK2_FLAGS)
> > DEFINE GCC49_ARM_ASLDLINK_FLAGS = DEF(GCC48_ARM_ASLDLINK_FLAGS)
> > DEFINE GCC49_AARCH64_ASLDLINK_FLAGS = DEF(GCC48_AARCH64_ASLDLINK_FLAGS)
> > @@ -5034,15 +5034,16 @@ RELEASE_GCC49_ARM_CC_FLAGS = DEF(GCC49_ARM_CC_FLAGS) -Wno-unused-but-set-v
> > *_GCC49_AARCH64_VFRPP_FLAGS = $(ARCHCC_FLAGS) $(PLATFORM_FLAGS) DEF(GCC_VFRPP_FLAGS)
> > *_GCC49_AARCH64_CC_XIPFLAGS = DEF(GCC49_AARCH64_CC_XIPFLAGS)
> >
> > - DEBUG_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -O0 -mcmodel=small
> > - DEBUG_GCC49_AARCH64_DLINK_FLAGS = DEF(GCC49_AARCH64_DLINK_FLAGS) -z common-page-size=0x1000
> > + DEBUG_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -O0
> > + DEBUG_GCC49_AARCH64_DLINK_FLAGS = DEF(GCC49_AARCH64_DLINK_FLAGS)
> > DEBUG_GCC49_AARCH64_DLINK_XIPFLAGS = -z common-page-size=0x20
> >
> > -RELEASE_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -Wno-unused-but-set-variable -Wno-unused-const-variable -mcmodel=tiny
> > +RELEASE_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -Wno-unused-but-set-variable -Wno-unused-const-variable
> > RELEASE_GCC49_AARCH64_DLINK_FLAGS = DEF(GCC49_AARCH64_DLINK_FLAGS)
> > +RELEASE_GCC49_AARCH64_DLINK_XIPFLAGS = -z common-page-size=0x20
> >
> > - NOOPT_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -O0 -mcmodel=small
> > - NOOPT_GCC49_AARCH64_DLINK_FLAGS = DEF(GCC49_AARCH64_DLINK_FLAGS) -z common-page-size=0x1000 -O0
> > + NOOPT_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -O0
> > + NOOPT_GCC49_AARCH64_DLINK_FLAGS = DEF(GCC49_AARCH64_DLINK_FLAGS) -O0
> > NOOPT_GCC49_AARCH64_DLINK_XIPFLAGS = -z common-page-size=0x20 -O0
> >
> > ####################################################################################
> > @@ -5189,14 +5190,15 @@ RELEASE_GCC5_ARM_DLINK_FLAGS = DEF(GCC5_ARM_DLINK_FLAGS) -flto -Os -L$(WORKS
> > *_GCC5_AARCH64_VFRPP_FLAGS = $(ARCHCC_FLAGS) $(PLATFORM_FLAGS) DEF(GCC_VFRPP_FLAGS)
> > *_GCC5_AARCH64_CC_XIPFLAGS = DEF(GCC5_AARCH64_CC_XIPFLAGS)
> >
> > - DEBUG_GCC5_AARCH64_CC_FLAGS = DEF(GCC5_AARCH64_CC_FLAGS) -flto -Wno-unused-but-set-variable -Wno-unused-const-variable -mcmodel=small
> > - DEBUG_GCC5_AARCH64_DLINK_FLAGS = DEF(GCC5_AARCH64_DLINK_FLAGS) -z common-page-size=0x1000 -flto -Os -L$(WORKSPACE)/ArmPkg/Library/GccLto -llto-aarch64 -Wl,-plugin-opt=-pass-through=-llto-aarch64 -Wno-lto-type-mismatch -mcmodel=small
> > + DEBUG_GCC5_AARCH64_CC_FLAGS = DEF(GCC5_AARCH64_CC_FLAGS) -flto -Wno-unused-but-set-variable -Wno-unused-const-variable
> > + DEBUG_GCC5_AARCH64_DLINK_FLAGS = DEF(GCC5_AARCH64_DLINK_FLAGS) -flto -Os -L$(WORKSPACE)/ArmPkg/Library/GccLto -llto-aarch64 -Wl,-plugin-opt=-pass-through=-llto-aarch64 -Wno-lto-type-mismatch
> > DEBUG_GCC5_AARCH64_DLINK_XIPFLAGS = -z common-page-size=0x20
> >
> > -RELEASE_GCC5_AARCH64_CC_FLAGS = DEF(GCC5_AARCH64_CC_FLAGS) -flto -Wno-unused-but-set-variable -Wno-unused-const-variable -mcmodel=tiny
> > +RELEASE_GCC5_AARCH64_CC_FLAGS = DEF(GCC5_AARCH64_CC_FLAGS) -flto -Wno-unused-but-set-variable -Wno-unused-const-variable
> > RELEASE_GCC5_AARCH64_DLINK_FLAGS = DEF(GCC5_AARCH64_DLINK_FLAGS) -flto -Os -L$(WORKSPACE)/ArmPkg/Library/GccLto -llto-aarch64 -Wl,-plugin-opt=-pass-through=-llto-aarch64 -Wno-lto-type-mismatch
> > +RELEASE_GCC5_AARCH64_DLINK_XIPFLAGS = -z common-page-size=0x20
> >
> > - NOOPT_GCC5_AARCH64_CC_FLAGS = DEF(GCC5_AARCH64_CC_FLAGS) -O0 -mcmodel=small
> > + NOOPT_GCC5_AARCH64_CC_FLAGS = DEF(GCC5_AARCH64_CC_FLAGS) -O0
> > NOOPT_GCC5_AARCH64_DLINK_FLAGS = DEF(GCC5_AARCH64_DLINK_FLAGS) -z common-page-size=0x1000 -O0
> > NOOPT_GCC5_AARCH64_DLINK_XIPFLAGS = -z common-page-size=0x20 -O0
> >
> >
>
> Looking at this patch with "--word-diff" helps a little (it's too bad we
> can't break these mile long option sequences to multiple lines). So:
>
> * For GCC49:
>
> - The compiler option "-mcmodel=small" is upstreamed from both
> NOOPT_GCC49_AARCH64_CC_FLAGS and DEBUG_GCC49_AARCH64_CC_FLAGS to
> GCC49_AARCH64_CC_FLAGS. This is a no-op for DEBUG and NOOPT, however the
> option will now also apply to RELEASE.
>
> - As a consequence, "-mcmodel=tiny" is not usable for RELEASE, and it is
> removed.
>
> - The same upstreaming occurs for "-z common-page-size=0x1000", in
> GCC49_AARCH64_DLINK_FLAGS. RELEASE needs no fixup, it simply benefits
> from the change. However, RELEASE_GCC49_AARCH64_DLINK_XIPFLAGS needs an
> override, to stick with the original 0x20 alignment.
>
> * For GCC5:
>
> - GCC5_AARCH64_CC_FLAGS is defined as GCC49_AARCH64_CC_FLAGS, therefore
> the "-mcmodel=small" inheritance from the above is automatic. Only the
> DEBUG/NOOPT removals, and the RELEASE fixup are needed.
>
> - GCC5_AARCH64_DLINK_FLAGS is defined as GCC49_AARCH64_DLINK_FLAGS,
> therefore the same inheritance pattern applies to "-z
> common-page-size=0x1000". I'm noticing an omission here however: "-z
> common-page-size=0x1000" should have been removed from
> NOOPT_GCC5_AARCH64_DLINK_FLAGS as well.
>
> - The RELEASE_GCC5_AARCH64_DLINK_XIPFLAGS override is spelled out the same.
>
>
> So... do you agree that NOOPT_GCC5_AARCH64_DLINK_FLAGS should have been
> trimmed as well?
>
Yes, well spotted.
> Furthermore, I suggest squeezing the word RELEASE into the subject,
> given that the change is only observable in RELEASE builds.
>
Sure, I'll add that as well.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] BaseTools/tools_def AARCH64: move GCC49/GGC5 to 4 KB section alignment
2018-12-10 18:12 ` Ard Biesheuvel
@ 2018-12-10 18:19 ` Laszlo Ersek
2018-12-11 13:34 ` Gao, Liming
0 siblings, 1 reply; 7+ messages in thread
From: Laszlo Ersek @ 2018-12-10 18:19 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: edk2-devel@lists.01.org, Gao, Liming
On 12/10/18 19:12, Ard Biesheuvel wrote:
> On Mon, 10 Dec 2018 at 19:08, Laszlo Ersek <lersek@redhat.com> wrote:
>>
>> On 12/10/18 15:13, Ard Biesheuvel wrote:
>>> Since 4 KB section alignment is required when mapping PE/COFF images
>>> with strict permissions, update the default section alignment when
>>> using GCC49 and GCC5 in RELEASE mode. Note that XIP modules such as
>>> SEC, PEIMs or PEI core are not affected by this change, since the
>>> override to 32 byte aligment remains in effect.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> ---
>>> BaseTools/Conf/tools_def.template | 24 +++++++++++---------
>>> 1 file changed, 13 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def.template
>>> index e0e68fd7fb49..5d34333dc54f 100755
>>> --- a/BaseTools/Conf/tools_def.template
>>> +++ b/BaseTools/Conf/tools_def.template
>>> @@ -4263,11 +4263,11 @@ DEFINE GCC49_ARM_ASM_FLAGS = DEF(GCC48_ARM_ASM_FLAGS)
>>> DEFINE GCC49_AARCH64_ASM_FLAGS = DEF(GCC48_AARCH64_ASM_FLAGS)
>>> DEFINE GCC49_ARM_CC_FLAGS = DEF(GCC48_ARM_CC_FLAGS)
>>> DEFINE GCC49_ARM_CC_XIPFLAGS = DEF(GCC48_ARM_CC_XIPFLAGS)
>>> -DEFINE GCC49_AARCH64_CC_FLAGS = $(ARCHCC_FLAGS) $(PLATFORM_FLAGS) DEF(GCC44_ALL_CC_FLAGS) DEF(GCC_AARCH64_CC_FLAGS)
>>> +DEFINE GCC49_AARCH64_CC_FLAGS = $(ARCHCC_FLAGS) $(PLATFORM_FLAGS) DEF(GCC44_ALL_CC_FLAGS) DEF(GCC_AARCH64_CC_FLAGS) -mcmodel=small
>>> DEFINE GCC49_AARCH64_CC_XIPFLAGS = DEF(GCC48_AARCH64_CC_XIPFLAGS)
>>> DEFINE GCC49_ARM_DLINK_FLAGS = DEF(GCC48_ARM_DLINK_FLAGS)
>>> DEFINE GCC49_ARM_DLINK2_FLAGS = DEF(GCC48_ARM_DLINK2_FLAGS)
>>> -DEFINE GCC49_AARCH64_DLINK_FLAGS = DEF(GCC48_AARCH64_DLINK_FLAGS)
>>> +DEFINE GCC49_AARCH64_DLINK_FLAGS = DEF(GCC48_AARCH64_DLINK_FLAGS) -z common-page-size=0x1000
>>> DEFINE GCC49_AARCH64_DLINK2_FLAGS = DEF(GCC48_AARCH64_DLINK2_FLAGS)
>>> DEFINE GCC49_ARM_ASLDLINK_FLAGS = DEF(GCC48_ARM_ASLDLINK_FLAGS)
>>> DEFINE GCC49_AARCH64_ASLDLINK_FLAGS = DEF(GCC48_AARCH64_ASLDLINK_FLAGS)
>>> @@ -5034,15 +5034,16 @@ RELEASE_GCC49_ARM_CC_FLAGS = DEF(GCC49_ARM_CC_FLAGS) -Wno-unused-but-set-v
>>> *_GCC49_AARCH64_VFRPP_FLAGS = $(ARCHCC_FLAGS) $(PLATFORM_FLAGS) DEF(GCC_VFRPP_FLAGS)
>>> *_GCC49_AARCH64_CC_XIPFLAGS = DEF(GCC49_AARCH64_CC_XIPFLAGS)
>>>
>>> - DEBUG_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -O0 -mcmodel=small
>>> - DEBUG_GCC49_AARCH64_DLINK_FLAGS = DEF(GCC49_AARCH64_DLINK_FLAGS) -z common-page-size=0x1000
>>> + DEBUG_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -O0
>>> + DEBUG_GCC49_AARCH64_DLINK_FLAGS = DEF(GCC49_AARCH64_DLINK_FLAGS)
>>> DEBUG_GCC49_AARCH64_DLINK_XIPFLAGS = -z common-page-size=0x20
>>>
>>> -RELEASE_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -Wno-unused-but-set-variable -Wno-unused-const-variable -mcmodel=tiny
>>> +RELEASE_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -Wno-unused-but-set-variable -Wno-unused-const-variable
>>> RELEASE_GCC49_AARCH64_DLINK_FLAGS = DEF(GCC49_AARCH64_DLINK_FLAGS)
>>> +RELEASE_GCC49_AARCH64_DLINK_XIPFLAGS = -z common-page-size=0x20
>>>
>>> - NOOPT_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -O0 -mcmodel=small
>>> - NOOPT_GCC49_AARCH64_DLINK_FLAGS = DEF(GCC49_AARCH64_DLINK_FLAGS) -z common-page-size=0x1000 -O0
>>> + NOOPT_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -O0
>>> + NOOPT_GCC49_AARCH64_DLINK_FLAGS = DEF(GCC49_AARCH64_DLINK_FLAGS) -O0
>>> NOOPT_GCC49_AARCH64_DLINK_XIPFLAGS = -z common-page-size=0x20 -O0
>>>
>>> ####################################################################################
>>> @@ -5189,14 +5190,15 @@ RELEASE_GCC5_ARM_DLINK_FLAGS = DEF(GCC5_ARM_DLINK_FLAGS) -flto -Os -L$(WORKS
>>> *_GCC5_AARCH64_VFRPP_FLAGS = $(ARCHCC_FLAGS) $(PLATFORM_FLAGS) DEF(GCC_VFRPP_FLAGS)
>>> *_GCC5_AARCH64_CC_XIPFLAGS = DEF(GCC5_AARCH64_CC_XIPFLAGS)
>>>
>>> - DEBUG_GCC5_AARCH64_CC_FLAGS = DEF(GCC5_AARCH64_CC_FLAGS) -flto -Wno-unused-but-set-variable -Wno-unused-const-variable -mcmodel=small
>>> - DEBUG_GCC5_AARCH64_DLINK_FLAGS = DEF(GCC5_AARCH64_DLINK_FLAGS) -z common-page-size=0x1000 -flto -Os -L$(WORKSPACE)/ArmPkg/Library/GccLto -llto-aarch64 -Wl,-plugin-opt=-pass-through=-llto-aarch64 -Wno-lto-type-mismatch -mcmodel=small
>>> + DEBUG_GCC5_AARCH64_CC_FLAGS = DEF(GCC5_AARCH64_CC_FLAGS) -flto -Wno-unused-but-set-variable -Wno-unused-const-variable
>>> + DEBUG_GCC5_AARCH64_DLINK_FLAGS = DEF(GCC5_AARCH64_DLINK_FLAGS) -flto -Os -L$(WORKSPACE)/ArmPkg/Library/GccLto -llto-aarch64 -Wl,-plugin-opt=-pass-through=-llto-aarch64 -Wno-lto-type-mismatch
>>> DEBUG_GCC5_AARCH64_DLINK_XIPFLAGS = -z common-page-size=0x20
>>>
>>> -RELEASE_GCC5_AARCH64_CC_FLAGS = DEF(GCC5_AARCH64_CC_FLAGS) -flto -Wno-unused-but-set-variable -Wno-unused-const-variable -mcmodel=tiny
>>> +RELEASE_GCC5_AARCH64_CC_FLAGS = DEF(GCC5_AARCH64_CC_FLAGS) -flto -Wno-unused-but-set-variable -Wno-unused-const-variable
>>> RELEASE_GCC5_AARCH64_DLINK_FLAGS = DEF(GCC5_AARCH64_DLINK_FLAGS) -flto -Os -L$(WORKSPACE)/ArmPkg/Library/GccLto -llto-aarch64 -Wl,-plugin-opt=-pass-through=-llto-aarch64 -Wno-lto-type-mismatch
>>> +RELEASE_GCC5_AARCH64_DLINK_XIPFLAGS = -z common-page-size=0x20
>>>
>>> - NOOPT_GCC5_AARCH64_CC_FLAGS = DEF(GCC5_AARCH64_CC_FLAGS) -O0 -mcmodel=small
>>> + NOOPT_GCC5_AARCH64_CC_FLAGS = DEF(GCC5_AARCH64_CC_FLAGS) -O0
>>> NOOPT_GCC5_AARCH64_DLINK_FLAGS = DEF(GCC5_AARCH64_DLINK_FLAGS) -z common-page-size=0x1000 -O0
>>> NOOPT_GCC5_AARCH64_DLINK_XIPFLAGS = -z common-page-size=0x20 -O0
>>>
>>>
>>
>> Looking at this patch with "--word-diff" helps a little (it's too bad we
>> can't break these mile long option sequences to multiple lines). So:
>>
>> * For GCC49:
>>
>> - The compiler option "-mcmodel=small" is upstreamed from both
>> NOOPT_GCC49_AARCH64_CC_FLAGS and DEBUG_GCC49_AARCH64_CC_FLAGS to
>> GCC49_AARCH64_CC_FLAGS. This is a no-op for DEBUG and NOOPT, however the
>> option will now also apply to RELEASE.
>>
>> - As a consequence, "-mcmodel=tiny" is not usable for RELEASE, and it is
>> removed.
>>
>> - The same upstreaming occurs for "-z common-page-size=0x1000", in
>> GCC49_AARCH64_DLINK_FLAGS. RELEASE needs no fixup, it simply benefits
>> from the change. However, RELEASE_GCC49_AARCH64_DLINK_XIPFLAGS needs an
>> override, to stick with the original 0x20 alignment.
>>
>> * For GCC5:
>>
>> - GCC5_AARCH64_CC_FLAGS is defined as GCC49_AARCH64_CC_FLAGS, therefore
>> the "-mcmodel=small" inheritance from the above is automatic. Only the
>> DEBUG/NOOPT removals, and the RELEASE fixup are needed.
>>
>> - GCC5_AARCH64_DLINK_FLAGS is defined as GCC49_AARCH64_DLINK_FLAGS,
>> therefore the same inheritance pattern applies to "-z
>> common-page-size=0x1000". I'm noticing an omission here however: "-z
>> common-page-size=0x1000" should have been removed from
>> NOOPT_GCC5_AARCH64_DLINK_FLAGS as well.
>>
>> - The RELEASE_GCC5_AARCH64_DLINK_XIPFLAGS override is spelled out the same.
>>
>>
>> So... do you agree that NOOPT_GCC5_AARCH64_DLINK_FLAGS should have been
>> trimmed as well?
>>
>
> Yes, well spotted.
>
>> Furthermore, I suggest squeezing the word RELEASE into the subject,
>> given that the change is only observable in RELEASE builds.
>>
>
> Sure, I'll add that as well.
>
Thanks! Personally I don't need to see a repost to believe you. :)
With those updates:
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] BaseTools/tools_def AARCH64: move GCC49/GGC5 to 4 KB section alignment
2018-12-10 14:13 [PATCH] BaseTools/tools_def AARCH64: move GCC49/GGC5 to 4 KB section alignment Ard Biesheuvel
2018-12-10 18:08 ` Laszlo Ersek
@ 2018-12-10 19:06 ` Leif Lindholm
1 sibling, 0 replies; 7+ messages in thread
From: Leif Lindholm @ 2018-12-10 19:06 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: edk2-devel, liming.gao
On Mon, Dec 10, 2018 at 03:13:39PM +0100, Ard Biesheuvel wrote:
> Since 4 KB section alignment is required when mapping PE/COFF images
> with strict permissions, update the default section alignment when
> using GCC49 and GCC5 in RELEASE mode. Note that XIP modules such as
> SEC, PEIMs or PEI core are not affected by this change, since the
> override to 32 byte aligment remains in effect.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
We have at least one platform in the wild that refuses to load
applications that don't have a 4KB aligned codeoffset.
>From experimentation, Visual Studio builds already enforce this on
AArch64.
I guess my only question/comment would be if GCC48 is left out
intentionally?
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
Tested-by: Leif Lindholm <leif.lindholm@linaro.org>
> ---
> BaseTools/Conf/tools_def.template | 24 +++++++++++---------
> 1 file changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def.template
> index e0e68fd7fb49..5d34333dc54f 100755
> --- a/BaseTools/Conf/tools_def.template
> +++ b/BaseTools/Conf/tools_def.template
> @@ -4263,11 +4263,11 @@ DEFINE GCC49_ARM_ASM_FLAGS = DEF(GCC48_ARM_ASM_FLAGS)
> DEFINE GCC49_AARCH64_ASM_FLAGS = DEF(GCC48_AARCH64_ASM_FLAGS)
> DEFINE GCC49_ARM_CC_FLAGS = DEF(GCC48_ARM_CC_FLAGS)
> DEFINE GCC49_ARM_CC_XIPFLAGS = DEF(GCC48_ARM_CC_XIPFLAGS)
> -DEFINE GCC49_AARCH64_CC_FLAGS = $(ARCHCC_FLAGS) $(PLATFORM_FLAGS) DEF(GCC44_ALL_CC_FLAGS) DEF(GCC_AARCH64_CC_FLAGS)
> +DEFINE GCC49_AARCH64_CC_FLAGS = $(ARCHCC_FLAGS) $(PLATFORM_FLAGS) DEF(GCC44_ALL_CC_FLAGS) DEF(GCC_AARCH64_CC_FLAGS) -mcmodel=small
> DEFINE GCC49_AARCH64_CC_XIPFLAGS = DEF(GCC48_AARCH64_CC_XIPFLAGS)
> DEFINE GCC49_ARM_DLINK_FLAGS = DEF(GCC48_ARM_DLINK_FLAGS)
> DEFINE GCC49_ARM_DLINK2_FLAGS = DEF(GCC48_ARM_DLINK2_FLAGS)
> -DEFINE GCC49_AARCH64_DLINK_FLAGS = DEF(GCC48_AARCH64_DLINK_FLAGS)
> +DEFINE GCC49_AARCH64_DLINK_FLAGS = DEF(GCC48_AARCH64_DLINK_FLAGS) -z common-page-size=0x1000
> DEFINE GCC49_AARCH64_DLINK2_FLAGS = DEF(GCC48_AARCH64_DLINK2_FLAGS)
> DEFINE GCC49_ARM_ASLDLINK_FLAGS = DEF(GCC48_ARM_ASLDLINK_FLAGS)
> DEFINE GCC49_AARCH64_ASLDLINK_FLAGS = DEF(GCC48_AARCH64_ASLDLINK_FLAGS)
> @@ -5034,15 +5034,16 @@ RELEASE_GCC49_ARM_CC_FLAGS = DEF(GCC49_ARM_CC_FLAGS) -Wno-unused-but-set-v
> *_GCC49_AARCH64_VFRPP_FLAGS = $(ARCHCC_FLAGS) $(PLATFORM_FLAGS) DEF(GCC_VFRPP_FLAGS)
> *_GCC49_AARCH64_CC_XIPFLAGS = DEF(GCC49_AARCH64_CC_XIPFLAGS)
>
> - DEBUG_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -O0 -mcmodel=small
> - DEBUG_GCC49_AARCH64_DLINK_FLAGS = DEF(GCC49_AARCH64_DLINK_FLAGS) -z common-page-size=0x1000
> + DEBUG_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -O0
> + DEBUG_GCC49_AARCH64_DLINK_FLAGS = DEF(GCC49_AARCH64_DLINK_FLAGS)
> DEBUG_GCC49_AARCH64_DLINK_XIPFLAGS = -z common-page-size=0x20
>
> -RELEASE_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -Wno-unused-but-set-variable -Wno-unused-const-variable -mcmodel=tiny
> +RELEASE_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -Wno-unused-but-set-variable -Wno-unused-const-variable
> RELEASE_GCC49_AARCH64_DLINK_FLAGS = DEF(GCC49_AARCH64_DLINK_FLAGS)
> +RELEASE_GCC49_AARCH64_DLINK_XIPFLAGS = -z common-page-size=0x20
>
> - NOOPT_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -O0 -mcmodel=small
> - NOOPT_GCC49_AARCH64_DLINK_FLAGS = DEF(GCC49_AARCH64_DLINK_FLAGS) -z common-page-size=0x1000 -O0
> + NOOPT_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -O0
> + NOOPT_GCC49_AARCH64_DLINK_FLAGS = DEF(GCC49_AARCH64_DLINK_FLAGS) -O0
> NOOPT_GCC49_AARCH64_DLINK_XIPFLAGS = -z common-page-size=0x20 -O0
>
> ####################################################################################
> @@ -5189,14 +5190,15 @@ RELEASE_GCC5_ARM_DLINK_FLAGS = DEF(GCC5_ARM_DLINK_FLAGS) -flto -Os -L$(WORKS
> *_GCC5_AARCH64_VFRPP_FLAGS = $(ARCHCC_FLAGS) $(PLATFORM_FLAGS) DEF(GCC_VFRPP_FLAGS)
> *_GCC5_AARCH64_CC_XIPFLAGS = DEF(GCC5_AARCH64_CC_XIPFLAGS)
>
> - DEBUG_GCC5_AARCH64_CC_FLAGS = DEF(GCC5_AARCH64_CC_FLAGS) -flto -Wno-unused-but-set-variable -Wno-unused-const-variable -mcmodel=small
> - DEBUG_GCC5_AARCH64_DLINK_FLAGS = DEF(GCC5_AARCH64_DLINK_FLAGS) -z common-page-size=0x1000 -flto -Os -L$(WORKSPACE)/ArmPkg/Library/GccLto -llto-aarch64 -Wl,-plugin-opt=-pass-through=-llto-aarch64 -Wno-lto-type-mismatch -mcmodel=small
> + DEBUG_GCC5_AARCH64_CC_FLAGS = DEF(GCC5_AARCH64_CC_FLAGS) -flto -Wno-unused-but-set-variable -Wno-unused-const-variable
> + DEBUG_GCC5_AARCH64_DLINK_FLAGS = DEF(GCC5_AARCH64_DLINK_FLAGS) -flto -Os -L$(WORKSPACE)/ArmPkg/Library/GccLto -llto-aarch64 -Wl,-plugin-opt=-pass-through=-llto-aarch64 -Wno-lto-type-mismatch
> DEBUG_GCC5_AARCH64_DLINK_XIPFLAGS = -z common-page-size=0x20
>
> -RELEASE_GCC5_AARCH64_CC_FLAGS = DEF(GCC5_AARCH64_CC_FLAGS) -flto -Wno-unused-but-set-variable -Wno-unused-const-variable -mcmodel=tiny
> +RELEASE_GCC5_AARCH64_CC_FLAGS = DEF(GCC5_AARCH64_CC_FLAGS) -flto -Wno-unused-but-set-variable -Wno-unused-const-variable
> RELEASE_GCC5_AARCH64_DLINK_FLAGS = DEF(GCC5_AARCH64_DLINK_FLAGS) -flto -Os -L$(WORKSPACE)/ArmPkg/Library/GccLto -llto-aarch64 -Wl,-plugin-opt=-pass-through=-llto-aarch64 -Wno-lto-type-mismatch
> +RELEASE_GCC5_AARCH64_DLINK_XIPFLAGS = -z common-page-size=0x20
>
> - NOOPT_GCC5_AARCH64_CC_FLAGS = DEF(GCC5_AARCH64_CC_FLAGS) -O0 -mcmodel=small
> + NOOPT_GCC5_AARCH64_CC_FLAGS = DEF(GCC5_AARCH64_CC_FLAGS) -O0
> NOOPT_GCC5_AARCH64_DLINK_FLAGS = DEF(GCC5_AARCH64_DLINK_FLAGS) -z common-page-size=0x1000 -O0
> NOOPT_GCC5_AARCH64_DLINK_XIPFLAGS = -z common-page-size=0x20 -O0
>
> --
> 2.19.2
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] BaseTools/tools_def AARCH64: move GCC49/GGC5 to 4 KB section alignment
2018-12-10 18:19 ` Laszlo Ersek
@ 2018-12-11 13:34 ` Gao, Liming
2018-12-11 13:53 ` Ard Biesheuvel
0 siblings, 1 reply; 7+ messages in thread
From: Gao, Liming @ 2018-12-11 13:34 UTC (permalink / raw)
To: Laszlo Ersek, Ard Biesheuvel; +Cc: edk2-devel@lists.01.org
I have no comments for this change. Reviewed-by: Liming Gao <liming.gao@intel.com>
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Tuesday, December 11, 2018 2:19 AM
> To: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: edk2-devel@lists.01.org; Gao, Liming <liming.gao@intel.com>
> Subject: Re: [edk2] [PATCH] BaseTools/tools_def AARCH64: move GCC49/GGC5 to 4 KB section alignment
>
> On 12/10/18 19:12, Ard Biesheuvel wrote:
> > On Mon, 10 Dec 2018 at 19:08, Laszlo Ersek <lersek@redhat.com> wrote:
> >>
> >> On 12/10/18 15:13, Ard Biesheuvel wrote:
> >>> Since 4 KB section alignment is required when mapping PE/COFF images
> >>> with strict permissions, update the default section alignment when
> >>> using GCC49 and GCC5 in RELEASE mode. Note that XIP modules such as
> >>> SEC, PEIMs or PEI core are not affected by this change, since the
> >>> override to 32 byte aligment remains in effect.
> >>>
> >>> Contributed-under: TianoCore Contribution Agreement 1.1
> >>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >>> ---
> >>> BaseTools/Conf/tools_def.template | 24 +++++++++++---------
> >>> 1 file changed, 13 insertions(+), 11 deletions(-)
> >>>
> >>> diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def.template
> >>> index e0e68fd7fb49..5d34333dc54f 100755
> >>> --- a/BaseTools/Conf/tools_def.template
> >>> +++ b/BaseTools/Conf/tools_def.template
> >>> @@ -4263,11 +4263,11 @@ DEFINE GCC49_ARM_ASM_FLAGS = DEF(GCC48_ARM_ASM_FLAGS)
> >>> DEFINE GCC49_AARCH64_ASM_FLAGS = DEF(GCC48_AARCH64_ASM_FLAGS)
> >>> DEFINE GCC49_ARM_CC_FLAGS = DEF(GCC48_ARM_CC_FLAGS)
> >>> DEFINE GCC49_ARM_CC_XIPFLAGS = DEF(GCC48_ARM_CC_XIPFLAGS)
> >>> -DEFINE GCC49_AARCH64_CC_FLAGS = $(ARCHCC_FLAGS) $(PLATFORM_FLAGS) DEF(GCC44_ALL_CC_FLAGS)
> DEF(GCC_AARCH64_CC_FLAGS)
> >>> +DEFINE GCC49_AARCH64_CC_FLAGS = $(ARCHCC_FLAGS) $(PLATFORM_FLAGS) DEF(GCC44_ALL_CC_FLAGS)
> DEF(GCC_AARCH64_CC_FLAGS) -mcmodel=small
> >>> DEFINE GCC49_AARCH64_CC_XIPFLAGS = DEF(GCC48_AARCH64_CC_XIPFLAGS)
> >>> DEFINE GCC49_ARM_DLINK_FLAGS = DEF(GCC48_ARM_DLINK_FLAGS)
> >>> DEFINE GCC49_ARM_DLINK2_FLAGS = DEF(GCC48_ARM_DLINK2_FLAGS)
> >>> -DEFINE GCC49_AARCH64_DLINK_FLAGS = DEF(GCC48_AARCH64_DLINK_FLAGS)
> >>> +DEFINE GCC49_AARCH64_DLINK_FLAGS = DEF(GCC48_AARCH64_DLINK_FLAGS) -z common-page-size=0x1000
> >>> DEFINE GCC49_AARCH64_DLINK2_FLAGS = DEF(GCC48_AARCH64_DLINK2_FLAGS)
> >>> DEFINE GCC49_ARM_ASLDLINK_FLAGS = DEF(GCC48_ARM_ASLDLINK_FLAGS)
> >>> DEFINE GCC49_AARCH64_ASLDLINK_FLAGS = DEF(GCC48_AARCH64_ASLDLINK_FLAGS)
> >>> @@ -5034,15 +5034,16 @@ RELEASE_GCC49_ARM_CC_FLAGS = DEF(GCC49_ARM_CC_FLAGS) -Wno-unused-but-set-v
> >>> *_GCC49_AARCH64_VFRPP_FLAGS = $(ARCHCC_FLAGS) $(PLATFORM_FLAGS) DEF(GCC_VFRPP_FLAGS)
> >>> *_GCC49_AARCH64_CC_XIPFLAGS = DEF(GCC49_AARCH64_CC_XIPFLAGS)
> >>>
> >>> - DEBUG_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -O0 -mcmodel=small
> >>> - DEBUG_GCC49_AARCH64_DLINK_FLAGS = DEF(GCC49_AARCH64_DLINK_FLAGS) -z common-page-size=0x1000
> >>> + DEBUG_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -O0
> >>> + DEBUG_GCC49_AARCH64_DLINK_FLAGS = DEF(GCC49_AARCH64_DLINK_FLAGS)
> >>> DEBUG_GCC49_AARCH64_DLINK_XIPFLAGS = -z common-page-size=0x20
> >>>
> >>> -RELEASE_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -Wno-unused-but-set-variable
> -Wno-unused-const-variable -mcmodel=tiny
> >>> +RELEASE_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -Wno-unused-but-set-variable
> -Wno-unused-const-variable
> >>> RELEASE_GCC49_AARCH64_DLINK_FLAGS = DEF(GCC49_AARCH64_DLINK_FLAGS)
> >>> +RELEASE_GCC49_AARCH64_DLINK_XIPFLAGS = -z common-page-size=0x20
> >>>
> >>> - NOOPT_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -O0 -mcmodel=small
> >>> - NOOPT_GCC49_AARCH64_DLINK_FLAGS = DEF(GCC49_AARCH64_DLINK_FLAGS) -z common-page-size=0x1000 -O0
> >>> + NOOPT_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -O0
> >>> + NOOPT_GCC49_AARCH64_DLINK_FLAGS = DEF(GCC49_AARCH64_DLINK_FLAGS) -O0
> >>> NOOPT_GCC49_AARCH64_DLINK_XIPFLAGS = -z common-page-size=0x20 -O0
> >>>
> >>> ####################################################################################
> >>> @@ -5189,14 +5190,15 @@ RELEASE_GCC5_ARM_DLINK_FLAGS = DEF(GCC5_ARM_DLINK_FLAGS) -flto -Os -L$(WORKS
> >>> *_GCC5_AARCH64_VFRPP_FLAGS = $(ARCHCC_FLAGS) $(PLATFORM_FLAGS) DEF(GCC_VFRPP_FLAGS)
> >>> *_GCC5_AARCH64_CC_XIPFLAGS = DEF(GCC5_AARCH64_CC_XIPFLAGS)
> >>>
> >>> - DEBUG_GCC5_AARCH64_CC_FLAGS = DEF(GCC5_AARCH64_CC_FLAGS) -flto -Wno-unused-but-set-variable
> -Wno-unused-const-variable -mcmodel=small
> >>> - DEBUG_GCC5_AARCH64_DLINK_FLAGS = DEF(GCC5_AARCH64_DLINK_FLAGS) -z common-page-size=0x1000 -flto -Os
> -L$(WORKSPACE)/ArmPkg/Library/GccLto -llto-aarch64 -Wl,-plugin-opt=-pass-through=-llto-aarch64 -Wno-lto-type-mismatch
> -mcmodel=small
> >>> + DEBUG_GCC5_AARCH64_CC_FLAGS = DEF(GCC5_AARCH64_CC_FLAGS) -flto -Wno-unused-but-set-variable
> -Wno-unused-const-variable
> >>> + DEBUG_GCC5_AARCH64_DLINK_FLAGS = DEF(GCC5_AARCH64_DLINK_FLAGS) -flto -Os
> -L$(WORKSPACE)/ArmPkg/Library/GccLto -llto-aarch64 -Wl,-plugin-opt=-pass-through=-llto-aarch64 -Wno-lto-type-mismatch
> >>> DEBUG_GCC5_AARCH64_DLINK_XIPFLAGS = -z common-page-size=0x20
> >>>
> >>> -RELEASE_GCC5_AARCH64_CC_FLAGS = DEF(GCC5_AARCH64_CC_FLAGS) -flto -Wno-unused-but-set-variable
> -Wno-unused-const-variable -mcmodel=tiny
> >>> +RELEASE_GCC5_AARCH64_CC_FLAGS = DEF(GCC5_AARCH64_CC_FLAGS) -flto -Wno-unused-but-set-variable
> -Wno-unused-const-variable
> >>> RELEASE_GCC5_AARCH64_DLINK_FLAGS = DEF(GCC5_AARCH64_DLINK_FLAGS) -flto -Os
> -L$(WORKSPACE)/ArmPkg/Library/GccLto -llto-aarch64 -Wl,-plugin-opt=-pass-through=-llto-aarch64 -Wno-lto-type-mismatch
> >>> +RELEASE_GCC5_AARCH64_DLINK_XIPFLAGS = -z common-page-size=0x20
> >>>
> >>> - NOOPT_GCC5_AARCH64_CC_FLAGS = DEF(GCC5_AARCH64_CC_FLAGS) -O0 -mcmodel=small
> >>> + NOOPT_GCC5_AARCH64_CC_FLAGS = DEF(GCC5_AARCH64_CC_FLAGS) -O0
> >>> NOOPT_GCC5_AARCH64_DLINK_FLAGS = DEF(GCC5_AARCH64_DLINK_FLAGS) -z common-page-size=0x1000 -O0
> >>> NOOPT_GCC5_AARCH64_DLINK_XIPFLAGS = -z common-page-size=0x20 -O0
> >>>
> >>>
> >>
> >> Looking at this patch with "--word-diff" helps a little (it's too bad we
> >> can't break these mile long option sequences to multiple lines). So:
> >>
> >> * For GCC49:
> >>
> >> - The compiler option "-mcmodel=small" is upstreamed from both
> >> NOOPT_GCC49_AARCH64_CC_FLAGS and DEBUG_GCC49_AARCH64_CC_FLAGS to
> >> GCC49_AARCH64_CC_FLAGS. This is a no-op for DEBUG and NOOPT, however the
> >> option will now also apply to RELEASE.
> >>
> >> - As a consequence, "-mcmodel=tiny" is not usable for RELEASE, and it is
> >> removed.
> >>
> >> - The same upstreaming occurs for "-z common-page-size=0x1000", in
> >> GCC49_AARCH64_DLINK_FLAGS. RELEASE needs no fixup, it simply benefits
> >> from the change. However, RELEASE_GCC49_AARCH64_DLINK_XIPFLAGS needs an
> >> override, to stick with the original 0x20 alignment.
> >>
> >> * For GCC5:
> >>
> >> - GCC5_AARCH64_CC_FLAGS is defined as GCC49_AARCH64_CC_FLAGS, therefore
> >> the "-mcmodel=small" inheritance from the above is automatic. Only the
> >> DEBUG/NOOPT removals, and the RELEASE fixup are needed.
> >>
> >> - GCC5_AARCH64_DLINK_FLAGS is defined as GCC49_AARCH64_DLINK_FLAGS,
> >> therefore the same inheritance pattern applies to "-z
> >> common-page-size=0x1000". I'm noticing an omission here however: "-z
> >> common-page-size=0x1000" should have been removed from
> >> NOOPT_GCC5_AARCH64_DLINK_FLAGS as well.
> >>
> >> - The RELEASE_GCC5_AARCH64_DLINK_XIPFLAGS override is spelled out the same.
> >>
> >>
> >> So... do you agree that NOOPT_GCC5_AARCH64_DLINK_FLAGS should have been
> >> trimmed as well?
> >>
> >
> > Yes, well spotted.
> >
> >> Furthermore, I suggest squeezing the word RELEASE into the subject,
> >> given that the change is only observable in RELEASE builds.
> >>
> >
> > Sure, I'll add that as well.
> >
>
> Thanks! Personally I don't need to see a repost to believe you. :)
>
> With those updates:
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>
> Thanks!
> Laszlo
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] BaseTools/tools_def AARCH64: move GCC49/GGC5 to 4 KB section alignment
2018-12-11 13:34 ` Gao, Liming
@ 2018-12-11 13:53 ` Ard Biesheuvel
0 siblings, 0 replies; 7+ messages in thread
From: Ard Biesheuvel @ 2018-12-11 13:53 UTC (permalink / raw)
To: Gao, Liming; +Cc: Laszlo Ersek, edk2-devel@lists.01.org
On Tue, 11 Dec 2018 at 14:34, Gao, Liming <liming.gao@intel.com> wrote:
>
> I have no comments for this change. Reviewed-by: Liming Gao <liming.gao@intel.com>
>
Thanks all
Pushed as 765fb87c2b70..de3c440e8a54
> > -----Original Message-----
> > From: Laszlo Ersek [mailto:lersek@redhat.com]
> > Sent: Tuesday, December 11, 2018 2:19 AM
> > To: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Cc: edk2-devel@lists.01.org; Gao, Liming <liming.gao@intel.com>
> > Subject: Re: [edk2] [PATCH] BaseTools/tools_def AARCH64: move GCC49/GGC5 to 4 KB section alignment
> >
> > On 12/10/18 19:12, Ard Biesheuvel wrote:
> > > On Mon, 10 Dec 2018 at 19:08, Laszlo Ersek <lersek@redhat.com> wrote:
> > >>
> > >> On 12/10/18 15:13, Ard Biesheuvel wrote:
> > >>> Since 4 KB section alignment is required when mapping PE/COFF images
> > >>> with strict permissions, update the default section alignment when
> > >>> using GCC49 and GCC5 in RELEASE mode. Note that XIP modules such as
> > >>> SEC, PEIMs or PEI core are not affected by this change, since the
> > >>> override to 32 byte aligment remains in effect.
> > >>>
> > >>> Contributed-under: TianoCore Contribution Agreement 1.1
> > >>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > >>> ---
> > >>> BaseTools/Conf/tools_def.template | 24 +++++++++++---------
> > >>> 1 file changed, 13 insertions(+), 11 deletions(-)
> > >>>
> > >>> diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def.template
> > >>> index e0e68fd7fb49..5d34333dc54f 100755
> > >>> --- a/BaseTools/Conf/tools_def.template
> > >>> +++ b/BaseTools/Conf/tools_def.template
> > >>> @@ -4263,11 +4263,11 @@ DEFINE GCC49_ARM_ASM_FLAGS = DEF(GCC48_ARM_ASM_FLAGS)
> > >>> DEFINE GCC49_AARCH64_ASM_FLAGS = DEF(GCC48_AARCH64_ASM_FLAGS)
> > >>> DEFINE GCC49_ARM_CC_FLAGS = DEF(GCC48_ARM_CC_FLAGS)
> > >>> DEFINE GCC49_ARM_CC_XIPFLAGS = DEF(GCC48_ARM_CC_XIPFLAGS)
> > >>> -DEFINE GCC49_AARCH64_CC_FLAGS = $(ARCHCC_FLAGS) $(PLATFORM_FLAGS) DEF(GCC44_ALL_CC_FLAGS)
> > DEF(GCC_AARCH64_CC_FLAGS)
> > >>> +DEFINE GCC49_AARCH64_CC_FLAGS = $(ARCHCC_FLAGS) $(PLATFORM_FLAGS) DEF(GCC44_ALL_CC_FLAGS)
> > DEF(GCC_AARCH64_CC_FLAGS) -mcmodel=small
> > >>> DEFINE GCC49_AARCH64_CC_XIPFLAGS = DEF(GCC48_AARCH64_CC_XIPFLAGS)
> > >>> DEFINE GCC49_ARM_DLINK_FLAGS = DEF(GCC48_ARM_DLINK_FLAGS)
> > >>> DEFINE GCC49_ARM_DLINK2_FLAGS = DEF(GCC48_ARM_DLINK2_FLAGS)
> > >>> -DEFINE GCC49_AARCH64_DLINK_FLAGS = DEF(GCC48_AARCH64_DLINK_FLAGS)
> > >>> +DEFINE GCC49_AARCH64_DLINK_FLAGS = DEF(GCC48_AARCH64_DLINK_FLAGS) -z common-page-size=0x1000
> > >>> DEFINE GCC49_AARCH64_DLINK2_FLAGS = DEF(GCC48_AARCH64_DLINK2_FLAGS)
> > >>> DEFINE GCC49_ARM_ASLDLINK_FLAGS = DEF(GCC48_ARM_ASLDLINK_FLAGS)
> > >>> DEFINE GCC49_AARCH64_ASLDLINK_FLAGS = DEF(GCC48_AARCH64_ASLDLINK_FLAGS)
> > >>> @@ -5034,15 +5034,16 @@ RELEASE_GCC49_ARM_CC_FLAGS = DEF(GCC49_ARM_CC_FLAGS) -Wno-unused-but-set-v
> > >>> *_GCC49_AARCH64_VFRPP_FLAGS = $(ARCHCC_FLAGS) $(PLATFORM_FLAGS) DEF(GCC_VFRPP_FLAGS)
> > >>> *_GCC49_AARCH64_CC_XIPFLAGS = DEF(GCC49_AARCH64_CC_XIPFLAGS)
> > >>>
> > >>> - DEBUG_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -O0 -mcmodel=small
> > >>> - DEBUG_GCC49_AARCH64_DLINK_FLAGS = DEF(GCC49_AARCH64_DLINK_FLAGS) -z common-page-size=0x1000
> > >>> + DEBUG_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -O0
> > >>> + DEBUG_GCC49_AARCH64_DLINK_FLAGS = DEF(GCC49_AARCH64_DLINK_FLAGS)
> > >>> DEBUG_GCC49_AARCH64_DLINK_XIPFLAGS = -z common-page-size=0x20
> > >>>
> > >>> -RELEASE_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -Wno-unused-but-set-variable
> > -Wno-unused-const-variable -mcmodel=tiny
> > >>> +RELEASE_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -Wno-unused-but-set-variable
> > -Wno-unused-const-variable
> > >>> RELEASE_GCC49_AARCH64_DLINK_FLAGS = DEF(GCC49_AARCH64_DLINK_FLAGS)
> > >>> +RELEASE_GCC49_AARCH64_DLINK_XIPFLAGS = -z common-page-size=0x20
> > >>>
> > >>> - NOOPT_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -O0 -mcmodel=small
> > >>> - NOOPT_GCC49_AARCH64_DLINK_FLAGS = DEF(GCC49_AARCH64_DLINK_FLAGS) -z common-page-size=0x1000 -O0
> > >>> + NOOPT_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -O0
> > >>> + NOOPT_GCC49_AARCH64_DLINK_FLAGS = DEF(GCC49_AARCH64_DLINK_FLAGS) -O0
> > >>> NOOPT_GCC49_AARCH64_DLINK_XIPFLAGS = -z common-page-size=0x20 -O0
> > >>>
> > >>> ####################################################################################
> > >>> @@ -5189,14 +5190,15 @@ RELEASE_GCC5_ARM_DLINK_FLAGS = DEF(GCC5_ARM_DLINK_FLAGS) -flto -Os -L$(WORKS
> > >>> *_GCC5_AARCH64_VFRPP_FLAGS = $(ARCHCC_FLAGS) $(PLATFORM_FLAGS) DEF(GCC_VFRPP_FLAGS)
> > >>> *_GCC5_AARCH64_CC_XIPFLAGS = DEF(GCC5_AARCH64_CC_XIPFLAGS)
> > >>>
> > >>> - DEBUG_GCC5_AARCH64_CC_FLAGS = DEF(GCC5_AARCH64_CC_FLAGS) -flto -Wno-unused-but-set-variable
> > -Wno-unused-const-variable -mcmodel=small
> > >>> - DEBUG_GCC5_AARCH64_DLINK_FLAGS = DEF(GCC5_AARCH64_DLINK_FLAGS) -z common-page-size=0x1000 -flto -Os
> > -L$(WORKSPACE)/ArmPkg/Library/GccLto -llto-aarch64 -Wl,-plugin-opt=-pass-through=-llto-aarch64 -Wno-lto-type-mismatch
> > -mcmodel=small
> > >>> + DEBUG_GCC5_AARCH64_CC_FLAGS = DEF(GCC5_AARCH64_CC_FLAGS) -flto -Wno-unused-but-set-variable
> > -Wno-unused-const-variable
> > >>> + DEBUG_GCC5_AARCH64_DLINK_FLAGS = DEF(GCC5_AARCH64_DLINK_FLAGS) -flto -Os
> > -L$(WORKSPACE)/ArmPkg/Library/GccLto -llto-aarch64 -Wl,-plugin-opt=-pass-through=-llto-aarch64 -Wno-lto-type-mismatch
> > >>> DEBUG_GCC5_AARCH64_DLINK_XIPFLAGS = -z common-page-size=0x20
> > >>>
> > >>> -RELEASE_GCC5_AARCH64_CC_FLAGS = DEF(GCC5_AARCH64_CC_FLAGS) -flto -Wno-unused-but-set-variable
> > -Wno-unused-const-variable -mcmodel=tiny
> > >>> +RELEASE_GCC5_AARCH64_CC_FLAGS = DEF(GCC5_AARCH64_CC_FLAGS) -flto -Wno-unused-but-set-variable
> > -Wno-unused-const-variable
> > >>> RELEASE_GCC5_AARCH64_DLINK_FLAGS = DEF(GCC5_AARCH64_DLINK_FLAGS) -flto -Os
> > -L$(WORKSPACE)/ArmPkg/Library/GccLto -llto-aarch64 -Wl,-plugin-opt=-pass-through=-llto-aarch64 -Wno-lto-type-mismatch
> > >>> +RELEASE_GCC5_AARCH64_DLINK_XIPFLAGS = -z common-page-size=0x20
> > >>>
> > >>> - NOOPT_GCC5_AARCH64_CC_FLAGS = DEF(GCC5_AARCH64_CC_FLAGS) -O0 -mcmodel=small
> > >>> + NOOPT_GCC5_AARCH64_CC_FLAGS = DEF(GCC5_AARCH64_CC_FLAGS) -O0
> > >>> NOOPT_GCC5_AARCH64_DLINK_FLAGS = DEF(GCC5_AARCH64_DLINK_FLAGS) -z common-page-size=0x1000 -O0
> > >>> NOOPT_GCC5_AARCH64_DLINK_XIPFLAGS = -z common-page-size=0x20 -O0
> > >>>
> > >>>
> > >>
> > >> Looking at this patch with "--word-diff" helps a little (it's too bad we
> > >> can't break these mile long option sequences to multiple lines). So:
> > >>
> > >> * For GCC49:
> > >>
> > >> - The compiler option "-mcmodel=small" is upstreamed from both
> > >> NOOPT_GCC49_AARCH64_CC_FLAGS and DEBUG_GCC49_AARCH64_CC_FLAGS to
> > >> GCC49_AARCH64_CC_FLAGS. This is a no-op for DEBUG and NOOPT, however the
> > >> option will now also apply to RELEASE.
> > >>
> > >> - As a consequence, "-mcmodel=tiny" is not usable for RELEASE, and it is
> > >> removed.
> > >>
> > >> - The same upstreaming occurs for "-z common-page-size=0x1000", in
> > >> GCC49_AARCH64_DLINK_FLAGS. RELEASE needs no fixup, it simply benefits
> > >> from the change. However, RELEASE_GCC49_AARCH64_DLINK_XIPFLAGS needs an
> > >> override, to stick with the original 0x20 alignment.
> > >>
> > >> * For GCC5:
> > >>
> > >> - GCC5_AARCH64_CC_FLAGS is defined as GCC49_AARCH64_CC_FLAGS, therefore
> > >> the "-mcmodel=small" inheritance from the above is automatic. Only the
> > >> DEBUG/NOOPT removals, and the RELEASE fixup are needed.
> > >>
> > >> - GCC5_AARCH64_DLINK_FLAGS is defined as GCC49_AARCH64_DLINK_FLAGS,
> > >> therefore the same inheritance pattern applies to "-z
> > >> common-page-size=0x1000". I'm noticing an omission here however: "-z
> > >> common-page-size=0x1000" should have been removed from
> > >> NOOPT_GCC5_AARCH64_DLINK_FLAGS as well.
> > >>
> > >> - The RELEASE_GCC5_AARCH64_DLINK_XIPFLAGS override is spelled out the same.
> > >>
> > >>
> > >> So... do you agree that NOOPT_GCC5_AARCH64_DLINK_FLAGS should have been
> > >> trimmed as well?
> > >>
> > >
> > > Yes, well spotted.
> > >
> > >> Furthermore, I suggest squeezing the word RELEASE into the subject,
> > >> given that the change is only observable in RELEASE builds.
> > >>
> > >
> > > Sure, I'll add that as well.
> > >
> >
> > Thanks! Personally I don't need to see a repost to believe you. :)
> >
> > With those updates:
> >
> > Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> >
> > Thanks!
> > Laszlo
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-12-11 13:53 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-12-10 14:13 [PATCH] BaseTools/tools_def AARCH64: move GCC49/GGC5 to 4 KB section alignment Ard Biesheuvel
2018-12-10 18:08 ` Laszlo Ersek
2018-12-10 18:12 ` Ard Biesheuvel
2018-12-10 18:19 ` Laszlo Ersek
2018-12-11 13:34 ` Gao, Liming
2018-12-11 13:53 ` Ard Biesheuvel
2018-12-10 19:06 ` Leif Lindholm
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox