From: "Leif Lindholm" <leif.lindholm@linaro.org>
To: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: edk2-devel-groups-io <devel@edk2.groups.io>,
"Gao, Liming" <liming.gao@intel.com>
Subject: Re: [PATCH] BaseTools/GenFw AARCH64: fix up GOT based relative relocations
Date: Wed, 4 Sep 2019 16:37:56 +0100 [thread overview]
Message-ID: <20190904153756.GM29255@bivouac.eciton.net> (raw)
In-Reply-To: <CAKv+Gu95W4Su85CgPEOuGowCtB_xXpOjC4n9ZzC1fAn2AdQVAQ@mail.gmail.com>
On Wed, Sep 04, 2019 at 05:01:58AM -0700, Ard Biesheuvel wrote:
> On Wed, 4 Sep 2019 at 04:49, Leif Lindholm <leif.lindholm@linaro.org> 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 <ard.biesheuvel@linaro.org>
> >
> > 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 <leif.lindholm@linaro.org>
Thanks!
> > > ---
> > > 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
> > >
next prev parent reply other threads:[~2019-09-04 15:38 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-09-04 4:17 [PATCH] BaseTools/GenFw AARCH64: fix up GOT based relative relocations Ard Biesheuvel
2019-09-04 11:49 ` Leif Lindholm
2019-09-04 12:01 ` Ard Biesheuvel
2019-09-04 14:22 ` Ard Biesheuvel
2019-09-04 14:35 ` [edk2-devel] " Liming Gao
2019-09-04 15:37 ` Leif Lindholm [this message]
2019-09-04 16:10 ` Ard Biesheuvel
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190904153756.GM29255@bivouac.eciton.net \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox