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

[Leif Lindholm <leif.lindholm@linaro.org>]

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?

> +//
> +// 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
when those cirquits may already be in use.)

/
    Leif

> +// GOT indirected symbol references are emitted, we need to use the pragma, or
> +// GOT based cross object references could be emitted, e.g., in libraries, and
> +// these cannot be relaxed to ordinary symbol references at link time.
> +//
> +#pragma GCC visibility push (hidden)
> -- 
> 2.19.2
>