Re: [PATCH 2/2] ArmVirtPkg/PrePiUniCoreRelocatable CLANG38: work around build issues

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On Tue, 11 Dec 2018 at 13:57, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>
> Not my package, but a couple of minor style/language comments below
> (because this is awkward enough any future readers/users will need all
> the help they can get).
>
> On Tue, Dec 11, 2018 at 01:19:36PM +0100, Ard Biesheuvel wrote:
> > The self-relocating PrePi module that is used by the ArmVirtQemuKernel
> > and ArmVirtXen targets runs the linker in PIE mode so that it emits
> > dynamic relocations into the final image in a way that permits the
> > module to relocate itself into place before calling into the C code.
> >
> > When building these targets using the CLANG38 toolchain, we switch
> > from the BFD to the GOLD linker, which behaves a bit differently when
> > building PIE executables, and insists on emitting GOT indirected symbol
> > references throughout, which means a) that we end up with absolute
> > addresses (which need to be fixed up at load time) for no good reason,
> > and b) we have to add support for handling GOT entries to GenFw if we
> > want to convert them into PE/COFF.
> >
> > So instead, let's emit a shared library. Since the ELF image only serves
> > as the input to GenFw, this does not lead to any loss of functionality,
> > although it does require the -Bsymbolic linker option to be added to
> > ensure that no symbol based dynamic relocations are emitted (which
> > would, e.g., permit lazy binding for shared libraries). So for all
> > other toolchains, the linker option changes are a no-op.
> >
> > Then, we have to convince CLANG38/GOLD that there is no need to refer
> > to symbols via a GOT entry. This is done by forcing hidden visibility
> > for all symbols in all components that make up the PrePi SEC module:
> > this informs the linker that a symbol is never exported or preempted,
> > making it safe to refer to it directly from anywhere in the code,
> > rather than indirectly via a GOT entry.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> >  ArmVirtPkg/ArmVirtQemuKernel.dsc                    | 12 ++++++++--
> >  ArmVirtPkg/ArmVirtXen.dsc                           | 12 ++++++++--
> >  ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf |  2 +-
> >  ArmVirtPkg/Include/Platform/Hidden.h                | 23 ++++++++++++++++++++
> >  4 files changed, 44 insertions(+), 5 deletions(-)
> >
> > diff --git a/ArmVirtPkg/ArmVirtQemuKernel.dsc b/ArmVirtPkg/ArmVirtQemuKernel.dsc
> > index 9928919bf5b0..c4324a9e264b 100644
> > --- a/ArmVirtPkg/ArmVirtQemuKernel.dsc
> > +++ b/ArmVirtPkg/ArmVirtQemuKernel.dsc
> > @@ -68,11 +68,19 @@ [LibraryClasses.common]
> >  [LibraryClasses.common.UEFI_DRIVER]
> >    UefiScsiLib|MdePkg/Library/UefiScsiLib/UefiScsiLib.inf
> >
> > -[BuildOptions.ARM.EDKII.SEC, BuildOptions.ARM.EDKII.BASE]
> > +[BuildOptions.common.EDKII.SEC, BuildOptions.common.EDKII.BASE]
> >    # Avoid MOVT/MOVW instruction pairs in code that may end up in the PIE
> >    # executable we build for the relocatable PrePi. They are not runtime
> >    # relocatable in ELF.
> > -  *_CLANG35_*_CC_FLAGS = -mno-movt
> > +  *_CLANG35_ARM_CC_FLAGS = -mno-movt
> > +
> > +  #
> > +  # CLANG38 with LTO support enabled uses the GNU GOLD linker, which insists
> > +  # on emitting GOT based symbol references when running in shared mode, unless
> > +  # we override visibility to 'hidden' in all modules that make up the PrePi
> > +  # build.
> > +  #
> > +  GCC:*_CLANG38_*_CC_FLAGS = -include $(WORKSPACE)/ArmVirtPkg/Include/Platform/Hidden.h
> >
> >  ################################################################################
> >  #
> > diff --git a/ArmVirtPkg/ArmVirtXen.dsc b/ArmVirtPkg/ArmVirtXen.dsc
> > index 20fae9e675bb..e083666f54ea 100644
> > --- a/ArmVirtPkg/ArmVirtXen.dsc
> > +++ b/ArmVirtPkg/ArmVirtXen.dsc
> > @@ -57,11 +57,19 @@ [LibraryClasses]
> >  [LibraryClasses.common.UEFI_DRIVER]
> >    UefiScsiLib|MdePkg/Library/UefiScsiLib/UefiScsiLib.inf
> >
> > -[BuildOptions.ARM.EDKII.SEC, BuildOptions.ARM.EDKII.BASE]
> > +[BuildOptions.common.EDKII.SEC, BuildOptions.common.EDKII.BASE]
> >    # Avoid MOVT/MOVW instruction pairs in code that may end up in the PIE
> >    # executable we build for the relocatable PrePi. They are not runtime
> >    # relocatable in ELF.
> > -  *_CLANG35_*_CC_FLAGS = -mno-movt
> > +  *_CLANG35_ARM_CC_FLAGS = -mno-movt
> > +
> > +  #
> > +  # CLANG38 with LTO support enabled uses the GNU GOLD linker, which insists
> > +  # on emitting GOT based symbol references when running in shared mode, unless
> > +  # we override visibility to 'hidden' in all modules that make up the PrePi
> > +  # build.
> > +  #
> > +  GCC:*_CLANG38_*_CC_FLAGS = -include $(WORKSPACE)/ArmVirtPkg/Include/Platform/Hidden.h
> >
> >  ################################################################################
> >  #
> > diff --git a/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf b/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf
> > index 034ddb41cb48..5fe6cd8eb481 100755
> > --- a/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf
> > +++ b/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf
> > @@ -105,4 +105,4 @@ [Pcd]
> >    gArmTokenSpaceGuid.PcdFvBaseAddress
> >
> >  [BuildOptions]
> > -  GCC:*_*_*_DLINK_FLAGS = -pie -Wl,-T,$(MODULE_DIR)/Scripts/PrePi-PIE.lds
> > +  GCC:*_*_*_DLINK_FLAGS = -shared -Wl,-Bsymbolic -Wl,-T,$(MODULE_DIR)/Scripts/PrePi-PIE.lds
> > diff --git a/ArmVirtPkg/Include/Platform/Hidden.h b/ArmVirtPkg/Include/Platform/Hidden.h
> > new file mode 100644
> > index 000000000000..20e9f5d896b2
> > --- /dev/null
> > +++ b/ArmVirtPkg/Include/Platform/Hidden.h
> > @@ -0,0 +1,23 @@
> > +/** @file
> > +
> > +  Copyright (c) 2018, Linaro Limited. All rights reserved.
> > +
> > +  This program and the accompanying materials
> > +  are licensed and made available under the terms and conditions of the BSD License
> > +  which accompanies this distribution.  The full text of the license may be found at
> > +  http://opensource.org/licenses/bsd-license.php
> > +
> > +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> > +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> > +
> > +**/
> > +
>
> I know this isn't intended to be explicitly included, but could we
> still have in include guard?
>

Sure.

> > +//
> > +// Setting the GCC -fvisibility=hidden command line option is not quite the same
> > +// as setting the pragma below: the former only affects definitions, whereas the
> > +// latter affects extern declarations as well. So if we want to ensure that no
>
> s/latter/pragma/? (Just saves on recursive natural language parsing
>

Fair enough.