Not needed any more on modern toolchains, they are better
in not creating a GOT without this trick.
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
MdePkg/Include/X64/ProcessorBind.h | 14 --------------
1 file changed, 14 deletions(-)
diff --git a/MdePkg/Include/X64/ProcessorBind.h b/MdePkg/Include/X64/ProcessorBind.h
index f0a4d00142b9..afbb4b6273fb 100644
--- a/MdePkg/Include/X64/ProcessorBind.h
+++ b/MdePkg/Include/X64/ProcessorBind.h
@@ -21,20 +21,6 @@
#pragma pack()
#endif
-#if defined (__GNUC__) && defined (__pic__) && !defined (USING_LTO) && !defined (__APPLE__)
-//
-// Mark all symbol declarations and references as hidden, meaning they will
-// not be subject to symbol preemption. This allows the compiler to refer to
-// symbols directly using relative references rather than via the GOT, which
-// contains absolute symbol addresses that are subject to runtime relocation.
-//
-// The LTO linker will not emit GOT based relocations when all symbol
-// references can be resolved locally, and so there is no need to set the
-// pragma in that case (and doing so will cause other issues).
-//
- #pragma GCC visibility push (hidden)
-#endif
-
#if defined (__INTEL_COMPILER)
//
// Disable ICC's remark #869: "Parameter" was never referenced warning.
--
2.40.0
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#103343): https://edk2.groups.io/g/devel/message/103343
Mute This Topic: https://groups.io/mt/98404595/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-> On 21. Apr 2023, at 06:45, Gerd Hoffmann <kraxel@redhat.com> wrote: > > Not needed any more on modern toolchains, they are better > in not creating a GOT without this trick. Hi Gerd, Thanks! Just out of interest, how did you test this and what were the results? Best regards, Marvin > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > --- > MdePkg/Include/X64/ProcessorBind.h | 14 -------------- > 1 file changed, 14 deletions(-) > > diff --git a/MdePkg/Include/X64/ProcessorBind.h b/MdePkg/Include/X64/ProcessorBind.h > index f0a4d00142b9..afbb4b6273fb 100644 > --- a/MdePkg/Include/X64/ProcessorBind.h > +++ b/MdePkg/Include/X64/ProcessorBind.h > @@ -21,20 +21,6 @@ > #pragma pack() > #endif > > -#if defined (__GNUC__) && defined (__pic__) && !defined (USING_LTO) && !defined (__APPLE__) > -// > -// Mark all symbol declarations and references as hidden, meaning they will > -// not be subject to symbol preemption. This allows the compiler to refer to > -// symbols directly using relative references rather than via the GOT, which > -// contains absolute symbol addresses that are subject to runtime relocation. > -// > -// The LTO linker will not emit GOT based relocations when all symbol > -// references can be resolved locally, and so there is no need to set the > -// pragma in that case (and doing so will cause other issues). > -// > - #pragma GCC visibility push (hidden) > -#endif > - > #if defined (__INTEL_COMPILER) > // > // Disable ICC's remark #869: "Parameter" was never referenced warning. > -- > 2.40.0 > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#103371): https://edk2.groups.io/g/devel/message/103371 Mute This Topic: https://groups.io/mt/98404595/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On Fri, Apr 21, 2023 at 06:01:11AM +0000, Marvin Häuser wrote: > > > On 21. Apr 2023, at 06:45, Gerd Hoffmann <kraxel@redhat.com> wrote: > > > > Not needed any more on modern toolchains, they are better > > in not creating a GOT without this trick. > > Hi Gerd, > > Thanks! Just out of interest, how did you test this and what were the results? Patch #1, adding a linker script assert as suggested by ard, then: * compile + test on my local workstation (fedora 37, gcc 12). * run CI * compile on some older distros: - rhel-8 (gcc 8) - ubuntu-18.04 (gcc 7) take care, Gerd -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#103374): https://edk2.groups.io/g/devel/message/103374 Mute This Topic: https://groups.io/mt/98404595/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On Fri, 21 Apr 2023 at 08:49, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> On Fri, Apr 21, 2023 at 06:01:11AM +0000, Marvin Häuser wrote:
> >
> > > On 21. Apr 2023, at 06:45, Gerd Hoffmann <kraxel@redhat.com> wrote:
> > >
> > > Not needed any more on modern toolchains, they are better
> > > in not creating a GOT without this trick.
> >
> > Hi Gerd,
> >
> > Thanks! Just out of interest, how did you test this and what were the results?
>
> Patch #1, adding a linker script assert as suggested by ard, then:
>
> * compile + test on my local workstation (fedora 37, gcc 12).
> * run CI
> * compile on some older distros:
> - rhel-8 (gcc 8)
> - ubuntu-18.04 (gcc 7)
>
I just realized that on x86, GenFw has some code to deal with GOT
entries if they are emitted. I'm not sure how often that gets
exercised, given our prior use of hidden visibility, but at least the
GOT entries should be covered by relocations if they exist.
*However*, one thing we are not taking into account is the fact that
relaxations are not usually reflected in the relocations emitted by
the compiler when using --emit-relocs. So we might end up with
occurrences like the below (taken from the Linux kernel but the idea
is the same)
ffffffff82fa59d5: 4c 8d 0d 24 66 88 ff lea -0x7799dc(%rip),%r9
ffffffff82fa59d8: R_X86_64_REX_GOTPCRELX level4_kernel_pgt-0x4
ffffffff82fa59dc: 49 8d 69 67 lea 0x67(%r9),%rbp
ffffffff82fa59e0: 4c 8d 15 19 76 88 ff lea -0x7789e7(%rip),%r10
ffffffff82fa59e3: R_X86_64_REX_GOTPCRELX level3_kernel_pgt-0x4
So here, the GOT loads have been relaxed into LEA instructions, but
GenFw will decode the immediate and assume it points to the GOT entry
rather than the variable itself, and happily emit a PE relocation for
it.
So it would be better to ASSERT() on non-empty GOT, and ignore such
GOTPCREL relocations instead of attempting to relocate the GOT entries
they (used to) refer to.
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#103380): https://edk2.groups.io/g/devel/message/103380
Mute This Topic: https://groups.io/mt/98404595/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
> On 21. Apr 2023, at 09:21, Ard Biesheuvel <ardb@kernel.org> wrote: > > On Fri, 21 Apr 2023 at 08:49, Gerd Hoffmann <kraxel@redhat.com> wrote: >> >>> On Fri, Apr 21, 2023 at 06:01:11AM +0000, Marvin Häuser wrote: >>> >>>> On 21. Apr 2023, at 06:45, Gerd Hoffmann <kraxel@redhat.com> wrote: >>>> >>>> Not needed any more on modern toolchains, they are better >>>> in not creating a GOT without this trick. >>> >>> Hi Gerd, >>> >>> Thanks! Just out of interest, how did you test this and what were the results? >> >> Patch #1, adding a linker script assert as suggested by ard, then: >> >> * compile + test on my local workstation (fedora 37, gcc 12). >> * run CI >> * compile on some older distros: >> - rhel-8 (gcc 8) >> - ubuntu-18.04 (gcc 7) >> > > I just realized that on x86, GenFw has some code to deal with GOT > entries if they are emitted. I'm not sure how often that gets > exercised, given our prior use of hidden visibility, but at least the > GOT entries should be covered by relocations if they exist. > > *However*, one thing we are not taking into account is the fact that > relaxations are not usually reflected in the relocations emitted by > the compiler when using --emit-relocs. So we might end up with > occurrences like the below (taken from the Linux kernel but the idea > is the same) > > ffffffff82fa59d5: 4c 8d 0d 24 66 88 ff lea -0x7799dc(%rip),%r9 > ffffffff82fa59d8: R_X86_64_REX_GOTPCRELX level4_kernel_pgt-0x4 > ffffffff82fa59dc: 49 8d 69 67 lea 0x67(%r9),%rbp > ffffffff82fa59e0: 4c 8d 15 19 76 88 ff lea -0x7789e7(%rip),%r10 > ffffffff82fa59e3: R_X86_64_REX_GOTPCRELX level3_kernel_pgt-0x4 > > So here, the GOT loads have been relaxed into LEA instructions, but > GenFw will decode the immediate and assume it points to the GOT entry > rather than the variable itself, and happily emit a PE relocation for > it. > > So it would be better to ASSERT() on non-empty GOT, and ignore such > GOTPCREL relocations instead of attempting to relocate the GOT entries > they (used to) refer to. Hmm, we’ve been toying with using only PIE relocs for X64 for a bit and finally merged it into master, so far no issues: https://github.com/acidanthera/audk/commit/92bb32130bcd0c35e48bdc308a18e5bc74cbaa42 https://github.com/acidanthera/audk/commit/42988773a06f9d6bf345fcbe82c1082ff1cfa2af In fact (I *did not* confirm this, it’s only a report I got), it seems to fix something regarding the stack protector. I’d not be surprised if there are edge-cases where -q does not get all necessary relocs when PIE is enabled. Best regards, Marvin -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#103381): https://edk2.groups.io/g/devel/message/103381 Mute This Topic: https://groups.io/mt/98404595/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
Gerd, Does this series need rework following this discussion, or is it ready to merge? -- Rebecca Cran On 4/21/23 01:46, Marvin Häuser wrote: > >> On 21. Apr 2023, at 09:21, Ard Biesheuvel <ardb@kernel.org> wrote: >> >> On Fri, 21 Apr 2023 at 08:49, Gerd Hoffmann <kraxel@redhat.com> wrote: >>> >>> On Fri, Apr 21, 2023 at 06:01:11AM +0000, Marvin Häuser wrote: >>>> >>>>> On 21. Apr 2023, at 06:45, Gerd Hoffmann <kraxel@redhat.com> wrote: >>>>> >>>>> Not needed any more on modern toolchains, they are better >>>>> in not creating a GOT without this trick. >>>> >>>> Hi Gerd, >>>> >>>> Thanks! Just out of interest, how did you test this and what were >>>> the results? >>> >>> Patch #1, adding a linker script assert as suggested by ard, then: >>> >>> * compile + test on my local workstation (fedora 37, gcc 12). >>> * run CI >>> * compile on some older distros: >>> - rhel-8 (gcc 8) >>> - ubuntu-18.04 (gcc 7) >>> >> >> I just realized that on x86, GenFw has some code to deal with GOT >> entries if they are emitted. I'm not sure how often that gets >> exercised, given our prior use of hidden visibility, but at least the >> GOT entries should be covered by relocations if they exist. >> >> *However*, one thing we are not taking into account is the fact that >> relaxations are not usually reflected in the relocations emitted by >> the compiler when using --emit-relocs. So we might end up with >> occurrences like the below (taken from the Linux kernel but the idea >> is the same) >> >> ffffffff82fa59d5: 4c 8d 0d 24 66 88 ff lea >> -0x7799dc(%rip),%r9 >> ffffffff82fa59d8: R_X86_64_REX_GOTPCRELX >> level4_kernel_pgt-0x4 >> ffffffff82fa59dc: 49 8d 69 67 lea 0x67(%r9),%rbp >> ffffffff82fa59e0: 4c 8d 15 19 76 88 ff lea >> -0x7789e7(%rip),%r10 >> ffffffff82fa59e3: R_X86_64_REX_GOTPCRELX >> level3_kernel_pgt-0x4 >> >> So here, the GOT loads have been relaxed into LEA instructions, but >> GenFw will decode the immediate and assume it points to the GOT entry >> rather than the variable itself, and happily emit a PE relocation for >> it. >> >> So it would be better to ASSERT() on non-empty GOT, and ignore such >> GOTPCREL relocations instead of attempting to relocate the GOT entries >> they (used to) refer to. > > Hmm, we’ve been toying with using only PIE relocs for X64 for a bit > and finally merged it into master, so far no issues: > https://github.com/acidanthera/audk/commit/92bb32130bcd0c35e48bdc308a18e5bc74cbaa42 > https://github.com/acidanthera/audk/commit/42988773a06f9d6bf345fcbe82c1082ff1cfa2af > > In fact (I *did not* confirm this, it’s only a report I got), it seems > to fix something regarding the stack protector. I’d not be surprised > if there are edge-cases where -q does not get all necessary relocs > when PIE is enabled. > > Best regards, > Marvin -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#104135): https://edk2.groups.io/g/devel/message/104135 Mute This Topic: https://groups.io/mt/98404595/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On Fri, May 05, 2023 at 08:33:41AM -0600, Rebecca Cran wrote: > Gerd, > > > Does this series need rework following this discussion, or is it ready to > merge? I think we are good to go. The ASSERT suggested by Ard here ... > > > > So it would be better to ASSERT() on non-empty GOT, and ignore such > > > GOTPCREL relocations instead of attempting to relocate the GOT entries > > > they (used to) refer to. ... is added to the linker scripts by patch #1 of the series. take care, Gerd -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#104328): https://edk2.groups.io/g/devel/message/104328 Mute This Topic: https://groups.io/mt/98404595/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
> On 21. Apr 2023, at 08:49, Gerd Hoffmann <kraxel@redhat.com> wrote: > > On Fri, Apr 21, 2023 at 06:01:11AM +0000, Marvin Häuser wrote: >> >>>> On 21. Apr 2023, at 06:45, Gerd Hoffmann <kraxel@redhat.com> wrote: >>> >>> Not needed any more on modern toolchains, they are better >>> in not creating a GOT without this trick. >> >> Hi Gerd, >> >> Thanks! Just out of interest, how did you test this and what were the results? > > Patch #1, adding a linker script assert as suggested by ard, then: > > * compile + test on my local workstation (fedora 37, gcc 12). > * run CI > * compile on some older distros: > - rhel-8 (gcc 8) > - ubuntu-18.04 (gcc 7) Did you include NOOPT? GCC5 specifies USING_LTO for DEBUG and RELEASE, thus the visibility would not be changed anyway. If this works, I hope the series makes it in time for the next stable tag. :) Thanks! Best regards, Marvin > > take care, > Gerd > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#103376): https://edk2.groups.io/g/devel/message/103376 Mute This Topic: https://groups.io/mt/98404595/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
> > Patch #1, adding a linker script assert as suggested by ard, then: > > > > * compile + test on my local workstation (fedora 37, gcc 12). > > * run CI > > * compile on some older distros: > > - rhel-8 (gcc 8) > > - ubuntu-18.04 (gcc 7) > > Did you include NOOPT? GCC5 specifies USING_LTO for DEBUG and RELEASE, thus the visibility would not be changed anyway. CI does noopt builds. take care, Gerd -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#103399): https://edk2.groups.io/g/devel/message/103399 Mute This Topic: https://groups.io/mt/98404595/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2026 Red Hat, Inc.