On Wed, 4 Sep 2019 at 08:37, Leif Lindholm <[email protected]> wrote:
>
> On Wed, Sep 04, 2019 at 05:01:58AM -0700, Ard Biesheuvel wrote:
> > On Wed, 4 Sep 2019 at 04:49, Leif Lindholm <[email protected]> wrote:
> > >
> > > On Tue, Sep 03, 2019 at 09:17:33PM -0700, Ard Biesheuvel wrote:
> > > > We take great care to avoid GOT based relocations in EDK2 executables,
> > > > primarily because they are pointless - we don't care about things like
> > > > the CoW footprint or relocations that target read-only sections, and so
> > > > GOT entries only bloat the binary.
> > > >
> > > > However, in some cases (e.g., when building the relocatable PrePi SEC
> > > > module in ArmVirtPkg with the CLANG38 toolchain), we may end up with
> > > > some GOT based relocations nonetheless, which break the build since
> > > > GenFw does not know how to deal with them.
> > > >
> > > > The relocations emitted in this case are ADRP/LDR instruction pairs
> > > > that are annotated as GOT based, which means that it is the linker's
> > > > job to emit the GOT entry and tag it with an appropriate dynamic
> > > > relocation that ensures that the correct absolute value is stored into
> > > > the GOT entry when the executable is loaded. This dynamic relocation
> > > > not visible to GenFw, and so populating the PE/COFF relocation section
> > > > for these entries is non-trivial.
> > > >
> > > > Since each ADRP/LDR pair refers to a single symbol that is local to the
> > > > binary (given that shared libraries are not supported), we can actually
> > > > convert the ADRP/LDR pair into an ADRP/ADD pair that produces the symbol
> > > > address directly rather than loading it from memory. This leaves the
> > > > GOT entry in the binary, but since it is now unused, it is no longer
> > > > necessary to emit a PE/COFF relocation entry for it.
> > > >
> > > > Signed-off-by: Ard Biesheuvel <[email protected]>
> > >
> > > This is a very neat fix. My only concern is that I am not able to
> > > reproduce the issue on my local Buster with clang7 (default). Is it
> > > reproducible with clang8?
> > >
> >
> > I managed to reproduce it on Ubuntu Bionic with clang 6. It may also
> > be related to the version of ld.gold or the LLVM gold plugin.
> >
> > You should be able to test this patch for correctness by stripping the
> > no-pie/no-pic options from the GCC5 command line, and checking any
> > produced .dll with readelf -r to see whether any GOT based relocations
> > were emitted, and whether the resulting binary still runs. I will do
> > the same locally.
>
> By removing the -no-pic/-no-pie flags, I get the GCC5 profile to
> display the issue. And by adding this patch, the build issue goes
> away.
>
> The image remains bootable with -kernel.
>
> I won't claim to have double checked the arithmetic/encodings, but
> apart from that:
> Reviewed-by: Leif Lindholm <[email protected]>
>
Thanks all
Pushed as adb59b633c12..d2687f23c909
> > > > ---
> > > > BaseTools/Source/C/GenFw/Elf64Convert.c | 28 +++++++++++++++++++-
> > > > 1 file changed, 27 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/BaseTools/Source/C/GenFw/Elf64Convert.c
> > > > b/BaseTools/Source/C/GenFw/Elf64Convert.c
> > > > index 3d6319c821e9..d574300ac4fe 100644
> > > > --- a/BaseTools/Source/C/GenFw/Elf64Convert.c
> > > > +++ b/BaseTools/Source/C/GenFw/Elf64Convert.c
> > > > @@ -1017,6 +1017,31 @@ WriteSections64 (
> > > > } else if (mEhdr->e_machine == EM_AARCH64) {
> > > >
> > > > switch (ELF_R_TYPE(Rel->r_info)) {
> > > > + INT64 Offset;
> > > > +
> > > > + case R_AARCH64_LD64_GOT_LO12_NC:
> > > > + //
> > > > + // Convert into an ADD instruction - see
> > > > R_AARCH64_ADR_GOT_PAGE below.
> > > > + //
> > > > + *(UINT32 *)Targ &= 0x3ff;
> > > > + *(UINT32 *)Targ |= 0x91000000 | ((Sym->st_value & 0xfff)
> > > > << 10);
> > > > + break;
> > > > +
> > > > + case R_AARCH64_ADR_GOT_PAGE:
> > > > + //
> > > > + // This relocation points to the GOT entry that contains
> > > > the absolute
> > > > + // address of the symbol we are referring to. Since EDK2
> > > > only uses
> > > > + // fully linked binaries, we can avoid the indirection,
> > > > and simply
> > > > + // refer to the symbol directly. This implies having to
> > > > patch the
> > > > + // subsequent LDR instruction (covered by a
> > > > R_AARCH64_LD64_GOT_LO12_NC
> > > > + // relocation) into an ADD instruction - this is handled
> > > > above.
> > > > + //
> > > > + Offset = (Sym->st_value - (Rel->r_offset & ~0xfff)) >> 12;
> > > > +
> > > > + *(UINT32 *)Targ &= 0x9000001f;
> > > > + *(UINT32 *)Targ |= ((Offset & 0x1ffffc) << (5 - 2)) |
> > > > ((Offset & 0x3) << 29);
> > > > +
> > > > + /* fall through */
> > > >
> > > > case R_AARCH64_ADR_PREL_PG_HI21:
> > > > //
> > > > @@ -1037,7 +1062,6 @@ WriteSections64 (
> > > > // Attempt to convert the ADRP into an ADR instruction.
> > > > // This is only possible if the symbol is within +/- 1
> > > > MB.
> > > > //
> > > > - INT64 Offset;
> > > >
> > > > // Decode the ADRP instruction
> > > > Offset = (INT32)((*(UINT32 *)Targ & 0xffffe0) << 8);
> > > > @@ -1212,6 +1236,8 @@ WriteRelocations64 (
> > > > case R_AARCH64_LDST32_ABS_LO12_NC:
> > > > case R_AARCH64_LDST64_ABS_LO12_NC:
> > > > case R_AARCH64_LDST128_ABS_LO12_NC:
> > > > + case R_AARCH64_ADR_GOT_PAGE:
> > > > + case R_AARCH64_LD64_GOT_LO12_NC:
> > > > //
> > > > // No fixups are required for relative relocations,
> > > > provided that
> > > > // the relative offsets between sections have been
> > > > preserved in
> > > > --
> > > > 2.17.1
> > > >
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#46819): https://edk2.groups.io/g/devel/message/46819
Mute This Topic: https://groups.io/mt/33134949/21656
Group Owner: [email protected]
Unsubscribe: https://edk2.groups.io/g/devel/unsub [[email protected]]
-=-=-=-=-=-=-=-=-=-=-=-
