I have no comments for this change. Reviewed-by: Liming Gao <[email protected]>
> -----Original Message----- > From: Laszlo Ersek [mailto:[email protected]] > Sent: Tuesday, December 11, 2018 2:19 AM > To: Ard Biesheuvel <[email protected]> > Cc: [email protected]; Gao, Liming <[email protected]> > 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 <[email protected]> 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 <[email protected]> > >>> --- > >>> 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 <[email protected]> > > Thanks! > Laszlo _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
