Re: [PATCH] BaseTools/tools_def AARCH64: move GCC49/GGC5 to 4 KB section alignment

[Ard Biesheuvel <ard.biesheuvel@linaro.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