[FreeBSD] avoid marking __stack_chk_guard symbol as dso_local on PPC64

[adalava]

Summary

Amends 1cb9f37a17ab restricting the logic to PPC64 as it introduces regression of FreeBSD/amd64 binaries compiled in freestanding mode (as kernel and modules) as discussed in https://reviews.llvm.org/D109090.

Bug report https://bugs.llvm.org/show_bug.cgi?id=51590

[dim]

Note that this is the version as we have it in the FreeBSD base system version of llvm 13.0.0. Without the isPPC64() check, we would get unexpected R_X86_64_REX_GOTP relocations in our kernel modules, which the kernel module loader cannot handle; see e.g. https://reviews.llvm.org/D109090#3113552 and further comments.

In that comment I mentioned that maybe we could get away with a freestanding check at this point. However, the addition of the isPPC64() check seems to be enough for now.

[MaskRay]

readelf -r without -W may not show the full relocation type name.
The relocation type is R_X86_64_REX_GOTPCRELX, not R_X86_64_REX_GOTP.

As I mentioned, -fno-pic may need GOT-generating relocations as well to avoid copy relocations. Not handling R_X86_64_REX_GOTPCRELX is FreeBSD's limitation which should be fixed https://maskray.me/blog/2021-08-29-all-about-global-offset-table

I haven't read all the PowerPC discussions but it seems that there may be a dso_local PowerPC64 ELFv2 issue as well which should be properly fixed.

With all these said, I am fine that this is pushed to fix the immediate problem for FreeBSD. But if FreeBSD 14.0.0 plans to fix R_X86_64_REX_GOTPCRELX, this patch is probably not very necessary.

The issue could also be avoided by defaulting to -mstack-protector-guard=tls for FreeBSD, like Linux.

[adalava]

In D113443#3147470, @MaskRay wrote:

readelf -r without -W may not show the full relocation type name.
The relocation type is R_X86_64_REX_GOTPCRELX, not R_X86_64_REX_GOTP.

As I mentioned, -fno-pic may need GOT-generating relocations as well to avoid copy relocations. Not handling R_X86_64_REX_GOTPCRELX is FreeBSD's limitation which should be fixed https://maskray.me/blog/2021-08-29-all-about-global-offset-table

I haven't read all the PowerPC discussions but it seems that there may be a dso_local PowerPC64 ELFv2 issue as well which should be properly fixed.

With all these said, I am fine that this is pushed to fix the immediate problem for FreeBSD. But if FreeBSD 14.0.0 plans to fix R_X86_64_REX_GOTPCRELX, this patch is probably not very necessary.

The issue could also be avoided by defaulting to -mstack-protector-guard=tls for FreeBSD, like Linux.

I don't think fixing R_X86_64_REX_GOTPCRELX is planned, we should open a bug for this.

@MaskRay @nemanjai Would be possible to make this patch backported in time for LLVM 13.1 release? With D109090 alone FreeBSD/powerpc64* regression is fixed, but introduces a new regression affecting FreeBSD/amd64.
The FreeBSD sources already contain this patch but there are other ports/packages that uses LLVM "vanilla" sources.

[arichardson]

Just ran into this trying to build FreeBSD amd64 with recent LLVM. I'm okay with this workaround being merged as long as the comment is updated.

llvm/lib/CodeGen/TargetLoweringBase.cpp

1992

Whether it's in libc.so or not should not matter, if TM.getRelocationModel() == Reloc::Static you are linking libc.a which means __stack_chk_guard is indeed dso_local. The ideal solution IMO would be to remove the FreeBSD special case completelty.

That said, I'm fine with having a ppc64 workaround here seeing as it is needed to use LLVM for PPC64 FreeBSD. However, the comment should be updated to say this is a workaround for PPC64 stack protector codegen not being correct rather than mentioning libc.so which is not relevant.

[arichardson]

llvm/lib/CodeGen/TargetLoweringBase.cpp

1992

Or are the broken PPC64 dynamic binaries are built with -fno-pic?

[arichardson]

llvm/lib/CodeGen/TargetLoweringBase.cpp

1991–1997

maybe something like this?

[myfreeweb]

Encountering this as well with llvm16, decided to research this bit:

Not handling R_X86_64_REX_GOTPCRELX is FreeBSD's limitation which should be fixed

..I can't find any dynamic loader that would handle GOTPCREL stuff? I've looked at glibc and the Linux kernel and it really doesn't seem like these are ever handled in a loader?

[adalava]

Update comments

[adalava]

sorry for the long delay, I'm not professionally working on PPC64 since then.

@arichardson and all, thanks for the review. I updated the comment, please check if it's ok.

[MaskRay]

In D113443#4105131, @myfreeweb wrote:

Encountering this as well with llvm16, decided to research this bit:

Not handling R_X86_64_REX_GOTPCRELX is FreeBSD's limitation which should be fixed

..I can't find any dynamic loader that would handle GOTPCREL stuff? I've looked at glibc and the Linux kernel and it really doesn't seem like these are ever handled in a loader?

R_X86_64_REX_GOTPCRELX is a static relocation type. Only link editors need to inspect it, not loaders, so you don't find them in glibc/musl/FreeBSD rtld.

Referencing an undefined symbol directly (not through a GOT-generating or PLT-generating relocation type) is a bad idea. If the symbol ends up to be defined in a shared object, you need a copy relocation.
See https://maskray.me/blog/2021-08-29-all-about-global-offset-table#undefined-symbols

The PPC64 ELFv2 ABI AIUI tries hard to avoid copy relocations.

llvm/lib/CodeGen/TargetLoweringBase.cpp

                                   GlobalVariable::ExternalLinkage, nullptr,
                                   "__stack_chk_guard");
 
-    // FreeBSD has "__stack_chk_guard" defined externally on libc.so
+    // XXX this is a workaround for FreeBSD PPC64 stack protector codegen and
+    // requires more investigation. For unknown reasons set dso_local here
+    // breaks PPC64 userland binaries (see bug #51590).
+    // However, FreeBSD amd64 kernel and modules are broken if dso_local
+    // is not set here, while PCC64 kernel and modules are fine without it.
     if (TM.getRelocationModel() == Reloc::Static &&
         !TM.getTargetTriple().isWindowsGNUEnvironment() &&
-        !TM.getTargetTriple().isOSFreeBSD())
+        !(TM.getTargetTriple().isPPC64() && TM.getTargetTriple().isOSFreeBSD()))
       GV->setDSOLocal(true);
   }
 }