* [PATCH] BaseTools/GenFw AARCH64: fix up GOT based relative relocations
@ 2019-09-04 4:17 Ard Biesheuvel
2019-09-04 11:49 ` Leif Lindholm
0 siblings, 1 reply; 7+ messages in thread
From: Ard Biesheuvel @ 2019-09-04 4:17 UTC (permalink / raw)
To: devel; +Cc: leif.lindholm, liming.gao, Ard Biesheuvel
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>
---
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] BaseTools/GenFw AARCH64: fix up GOT based relative relocations
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
0 siblings, 1 reply; 7+ messages in thread
From: Leif Lindholm @ 2019-09-04 11:49 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: devel, liming.gao
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?
/
Leif
> ---
> 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
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] BaseTools/GenFw AARCH64: fix up GOT based relative relocations
2019-09-04 11:49 ` Leif Lindholm
@ 2019-09-04 12:01 ` Ard Biesheuvel
2019-09-04 14:22 ` Ard Biesheuvel
2019-09-04 15:37 ` Leif Lindholm
0 siblings, 2 replies; 7+ messages in thread
From: Ard Biesheuvel @ 2019-09-04 12:01 UTC (permalink / raw)
To: Leif Lindholm; +Cc: edk2-devel-groups-io, Gao, Liming
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.
> > ---
> > 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
> >
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] BaseTools/GenFw AARCH64: fix up GOT based relative relocations
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
1 sibling, 1 reply; 7+ messages in thread
From: Ard Biesheuvel @ 2019-09-04 14:22 UTC (permalink / raw)
To: Leif Lindholm; +Cc: edk2-devel-groups-io, Gao, Liming
On Wed, 4 Sep 2019 at 05:01, Ard Biesheuvel <ard.biesheuvel@linaro.org> 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.
>
This worked for me, so I am fairly confident that this change is correct.
Once we have this change in, I'll double check whether the produced
CLANG38 ArmVirtQemuKernel images work as expected.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-devel] [PATCH] BaseTools/GenFw AARCH64: fix up GOT based relative relocations
2019-09-04 14:22 ` Ard Biesheuvel
@ 2019-09-04 14:35 ` Liming Gao
0 siblings, 0 replies; 7+ messages in thread
From: Liming Gao @ 2019-09-04 14:35 UTC (permalink / raw)
To: devel@edk2.groups.io, ard.biesheuvel@linaro.org, Leif Lindholm
Ack-by: Liming Gao <liming.gao@intel.com>
> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Ard Biesheuvel
> Sent: Wednesday, September 4, 2019 10:22 PM
> To: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: edk2-devel-groups-io <devel@edk2.groups.io>; Gao, Liming <liming.gao@intel.com>
> Subject: Re: [edk2-devel] [PATCH] BaseTools/GenFw AARCH64: fix up GOT based relative relocations
>
> On Wed, 4 Sep 2019 at 05:01, Ard Biesheuvel <ard.biesheuvel@linaro.org> 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.
> >
>
> This worked for me, so I am fairly confident that this change is correct.
>
> Once we have this change in, I'll double check whether the produced
> CLANG38 ArmVirtQemuKernel images work as expected.
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] BaseTools/GenFw AARCH64: fix up GOT based relative relocations
2019-09-04 12:01 ` Ard Biesheuvel
2019-09-04 14:22 ` Ard Biesheuvel
@ 2019-09-04 15:37 ` Leif Lindholm
2019-09-04 16:10 ` Ard Biesheuvel
1 sibling, 1 reply; 7+ messages in thread
From: Leif Lindholm @ 2019-09-04 15:37 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: edk2-devel-groups-io, Gao, Liming
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
> > >
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] BaseTools/GenFw AARCH64: fix up GOT based relative relocations
2019-09-04 15:37 ` Leif Lindholm
@ 2019-09-04 16:10 ` Ard Biesheuvel
0 siblings, 0 replies; 7+ messages in thread
From: Ard Biesheuvel @ 2019-09-04 16:10 UTC (permalink / raw)
To: Leif Lindholm; +Cc: edk2-devel-groups-io, Gao, Liming
On Wed, 4 Sep 2019 at 08:37, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>
> 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 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
> > > >
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-09-04 16:10 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
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
2019-09-04 16:10 ` Ard Biesheuvel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox