[FreeBSD] avoid marking __stack_chk_guard symbol as dso_local on FreeBSD

[adalava]

Summary

Binaries compiled with any optization model plus -fstack-protector-strong crash with segmentation on llvm12 and later due to bad pointer to "__stack_chk_guard" . This is known to affect powerpc64 (both BE and LE) on FreeBSD 13 and later.

Source of regression was traced to 2518433f861fcb877d0a7bdd9aec1aec1f77505a but it's not clear that this commit is the "real root cause", it probably uncovered a bug in PowerPC64 backend since other platforms generate a working binary. In any case on FreeBSD the "__stack_chk_guard" symbol is defined on external DSO (libc.so.7), so make sense not marking it as dso_local..

Linux doesn't follow the same path since it uses a different approach (LOAD_STACK_GUARD)

*Reported in bugzilla: https://bugs.llvm.org/show_bug.cgi?id=51590
*Similar patch was merged to FreeBSD source tree: https://cgit.freebsd.org/src/commit/?h=stable/13&id=e8e5d75e6a9676e76c3bfd6d1d52561ffbb40846

Test case:

int main(int argc, char *argv[]) 
{ 
  char name[10]; 
  if (ttyname_r(0, name, 10)) 
    err(1, "capsicum"); return 0;
};

build parameters:

-target powerpc64-unknown-freebsd13 -O2 -fstack-protector-strong

[adalava]

rebased from stable/13.x to main branch

[arichardson]

This does not seem like the right approach to me. Have you looked into why the pointer is not correct. Does it affect static binaries or dynamic ones? Maybe this can be fixed in rtld instead? As it works for all other architectures (and PPC Linux), I feel like that is the more likely cause

[adalava]

In D109090#2978298, @arichardson wrote:

This does not seem like the right approach to me. Have you looked into why the pointer is not correct. Does it affect static binaries or dynamic ones? Maybe this can be fixed in rtld instead? As it works for all other architectures (and PPC Linux), I feel like that is the more likely cause

Agreed, it looks like "GV->setDSOLocal(true)" is only exposing the problem. I confirmed it affects only dynamic binaries, static ones are fine. On clang11 it points to something like 0x81030fad8, and on clang12 0xffffffffffff8ee8. I noticed PPC64LE Linux binaries are using a different approach since "stack_chk_guard" isn't defined on glibc, according to https://reviews.llvm.org/D17740. I used the patch bellow to force the Linux approach and it builds sane binaries but it's not correct since FreeBSD has "stack_chk_guard" defined on libc.

diff --git a/llvm/lib/Target/PowerPC/PPCISelLowering.cpp b/llvm/lib/Target/PowerPC/PPCISelLowering.cpp
index 7be3115bb7db..cb88af4c0821 100644
--- a/llvm/lib/Target/PowerPC/PPCISelLowering.cpp
+++ b/llvm/lib/Target/PowerPC/PPCISelLowering.cpp
@@ -16633,8 +16633,8 @@ SDValue PPCTargetLowering::getNegatedExpression(SDValue Op, SelectionDAG &DAG,

 // Override to enable LOAD_STACK_GUARD lowering on Linux.
 bool PPCTargetLowering::useLoadStackGuardNode() const {
-  if (!Subtarget.isTargetLinux())
-    return TargetLowering::useLoadStackGuardNode();
+  //if (!Subtarget.isTargetLinux())
+  //  return TargetLowering::useLoadStackGuardNode();
   return true;
 }

diff --git a/llvm/lib/Target/PowerPC/PPCInstrInfo.cpp b/llvm/lib/Target/PowerPC/PPCInstrInfo.cpp
index c74b6bcf4287..d92ea1053201 100644
--- a/llvm/lib/Target/PowerPC/PPCInstrInfo.cpp
+++ b/llvm/lib/Target/PowerPC/PPCInstrInfo.cpp
@@ -3022,8 +3022,8 @@ bool PPCInstrInfo::expandPostRAPseudo(MachineInstr &MI) const {
     return true;
   }
   case TargetOpcode::LOAD_STACK_GUARD: {
-    assert(Subtarget.isTargetLinux() &&
-           "Only Linux target is expected to contain LOAD_STACK_GUARD");
+    //assert(Subtarget.isTargetLinux() &&
+    //       "Only Linux target is expected to contain LOAD_STACK_GUARD");
     const int64_t Offset = Subtarget.isPPC64() ? -0x7010 : -0x7008;
     const unsigned Reg = Subtarget.isPPC64() ? PPC::X13 : PPC::R2;
     MI.setDesc(get(Subtarget.isPPC64() ? PPC::LD : PPC::LWZ));

[emaste]

It looks like this worked prior to 2518433f861f because shouldAssumeDSOLocal has this case:

// PPC has no copy relocations and cannot use a plt entry as a symbol address.
llvm::Triple::ArchType Arch = TT.getArch();
if (Arch == llvm::Triple::ppc || Arch == llvm::Triple::ppc64 ||
    Arch == llvm::Triple::ppc64le)
  return false;

(later changed to TT.isPPC64() but same effect)

[MaskRay]

In D109090#3004618, @emaste wrote:

It looks like this worked prior to 2518433f861f because shouldAssumeDSOLocal has this case:

// PPC has no copy relocations and cannot use a plt entry as a symbol address.
llvm::Triple::ArchType Arch = TT.getArch();
if (Arch == llvm::Triple::ppc || Arch == llvm::Triple::ppc64 ||
    Arch == llvm::Triple::ppc64le)
  return false;

(later changed to TT.isPPC64() but same effect)

The ppc part was incorrect. ppc32 has copy relocations and GCC does leverage it. isPPC64 is because ELF v2 prefers TOC indirection for access to non-preemptible definitions, but the special condition was still spurious.

I am afraid the root cause is still unknown and this patch just papers over the issue.
I hope we have a good understanding why having dso_local will cause trouble on FreeBSD powerpc64's -fstack-protector* and fix the root cause instead.

[adalava]

In D109090#3007009, @MaskRay wrote:

I am afraid the root cause is still unknown and this patch just papers over the issue.
I hope we have a good understanding why having dso_local will cause trouble on FreeBSD powerpc64's -fstack-protector* and fix the root cause instead.

@MaskRay , since the binary works when compiled with -O0 and doesn't work with -O1, I used '-mllvm -opt-bisect-limit=NNN' as clang parameter and found that the binary stops working at the following optimization pass:

BISECT: running pass (80) PowerPC DAG->DAG Pattern Instruction Selection on function (main)

May it give some light?

[nemanjai]

@MaskRay According to @adalava the symbol __stack_chk_guard resides in libc.so on FreeBSD (i.e. a different DSO from the compiled program). Can you please elaborate on why you feel that marking such a symbol dso_local is the correct thing to do and there is an underlying problem elsewhere? Once we understand the logic there, we can dig into any deeper problems.

[nemanjai]

@sfertile Just wanted to add you here as well as you are quite familiar with dso_local and the various PPC relocations.

[sfertile]

In D109090#3018978, @nemanjai wrote:

@adalava the symbol __stack_chk_guard resides in libc.so on FreeBSD (i.e. a different DSO from the compiled program).

If that is the case then isn't DSO local. Was the idea behind Make __stack_chk_guard dso_local if Reloc::Static that the linker would emit a copy-relocation in the case we don't statically link against libc?

llvm/lib/CodeGen/TargetLoweringBase.cpp

1989

Obviously fix the formatting, but otherwise this looks like the right fix to me.

[adalava]

Update patch to avoid setting dso_local for any FreeBSD target since symbol is defined on libc.so.7.

[adalava]

It's now avoiding a copy relocation for '__stack_chk_guard':

Relocation section with addend (.rela.dyn):
r_offset     r_info       r_type              st_value         st_name + r_addend
000010020f88 000100000026 R_PPC64_ADDR64      0000000000000000 _init_tls + 0
000010020f80 000200000026 R_PPC64_ADDR64      0000000000000000 atexit + 0
000010020f98 000300000026 R_PPC64_ADDR64      0000000000000000 exit + 0
000010020fa8 000800000026 R_PPC64_ADDR64      0000000000000000 __stack_chk_guard + 0
..
..
Symbol table (.dynsym) contains 11 entries:
   Num:    Value          Size Type    Bind   Vis      Ndx Name
     0: 0000000000000000     0 NOTYPE  LOCAL  DEFAULT  UND 
     1: 0000000000000000     0 FUNC    GLOBAL DEFAULT  UND _init_tls@FBSD_1.0 (2)
     2: 0000000000000000     0 FUNC    GLOBAL DEFAULT  UND atexit@FBSD_1.0 (2)
     3: 0000000000000000     0 FUNC    GLOBAL DEFAULT  UND exit@FBSD_1.0 (2)
     4: 0000000000000000     0 NOTYPE  WEAK   DEFAULT  UND _Jv_RegisterClasses
     5: 0000000000000000     0 FUNC    GLOBAL DEFAULT  UND ttyname_r@FBSD_1.0 (2)
     6: 0000000000000000     0 FUNC    GLOBAL DEFAULT  UND err@FBSD_1.0 (2)
     7: 0000000000000000     0 FUNC    GLOBAL DEFAULT  UND __stack_chk_fail@FBSD_1.0 (2)
     8: 0000000000000000     0 OBJECT  GLOBAL DEFAULT  UND __stack_chk_guard@FBSD_1.0 (2)
     9: 0000000010030fb0     8 OBJECT  GLOBAL DEFAULT   24 __progname
    10: 0000000010030fc0     8 OBJECT  GLOBAL DEFAULT   26 environ

[adalava]

[adalava]

add tests for FreeBSD

[adalava]

@sfertile is this last version still good for you? (after removing isPPC64 from if clause)

[MaskRay]

llvm/test/CodeGen/PowerPC/stack-protector.ll

14

Can be simplified as [[#]]

[adalava]

• Simplified register number check as suggested by @MaskRay
• Added test for powerpcle (32 bits LE)

[nemanjai]

LGTM.

[dim]

Hmm this seems to cause some trouble when building kernels and kernel modules on FreeBSD, which are built in -ffreestanding mode, and then you get weird relocations in kernel modules:

clang13  -O2 -pipe -fno-common  -fno-strict-aliasing -Werror -D_KERNEL -DKLD_MODULE -DKLD_TIED -nostdinc   -DHAVE_KERNEL_OPTION_HEADERS -include /usr/home/kostik/work/DEV/src/sys/amd64/compile/X/opt_global.h -I. -I/usr/home/kostik/work/DEV/src/sys -I/usr/home/kostik/work/DEV/src/sys/contrib/ck/include -fno-common -gdwarf-2 -fno-omit-frame-pointer -mno-omit-leaf-frame-pointer -fdebug-prefix-map=./machine=/usr/home/kostik/work/DEV/src/sys/amd64/include -fdebug-prefix-map=./x86=/usr/home/kostik/work/DEV/src/sys/x86/include -I/usr/home/kostik/work/DEV/src/sys/amd64/compile/X     -MD  -MF.depend.ufs_vnops.o -MTufs_vnops.o -mcmodel=kernel -mno-red-zone -mno-mmx -mno-sse -msoft-float  -fno-asynchronous-unwind-tables -ffreestanding -fwrapv -fstack-protector -Wall -Wredundant-decls -Wnested-externs -Wstrict-prototypes -Wmissing-prototypes -Wpointer-arith -Wcast-qual -Wundef -Wno-pointer-sign -D__printf__=__freebsd_kprintf__ -Wmissing-include-dirs -fdiagnostics-show-option -Wno-unknown-pragmas -Wno-error=tautological-compare -Wno-error=empty-body -Wno-error=parentheses-equality -Wno-error=unused-function -Wno-error=pointer-sign -Wno-error=shift-negative-value -Wno-address-of-packed-member -Wno-error=unused-but-set-variable -Wno-format-zero-length   -mno-aes -mno-avx  -std=iso9899:1999 -c /usr/home/kostik/work/DEV/src/sys/ufs/ufs/ufs_vnops.c -o ufs_vnops.o

% readelf -a ../src/sys/amd64/compile/X/modules/usr/home/kostik/work/DEV/src/sys/modules/ufs/ufs_vnops.o | grep GOT | head -3
0000000001f7  004a0000002a R_X86_64_REX_GOTP 0000000000000000 __stack_chk_guard - 4
00000000060d  004a0000002a R_X86_64_REX_GOTP 0000000000000000 __stack_chk_guard - 4
0000000007c8  004a0000002a R_X86_64_REX_GOTP 0000000000000000 __stack_chk_guard - 4

Maybe this should have an additional check for freestanding mode?

[nemanjai]

In D109090#3113552, @dim wrote:

Hmm this seems to cause some trouble when building kernels and kernel modules on FreeBSD, which are built in -ffreestanding mode, and then you get weird relocations in kernel modules:

% readelf -a ../src/sys/amd64/compile/X/modules/usr/home/kostik/work/DEV/src/sys/modules/ufs/ufs_vnops.o | grep GOT | head -3
0000000001f7 004a0000002a R_X86_64_REX_GOTP 0000000000000000 stack_chk_guard - 4
00000000060d 004a0000002a R_X86_64_REX_GOTP 0000000000000000 stack_chk_guard - 4
0000000007c8 004a0000002a R_X86_64_REX_GOTP 0000000000000000 __stack_chk_guard - 4

Maybe this should have an additional check for freestanding mode?

Hmm, something seems odd here. I don't claim to really understand what is going on or why these relocations are a problem, but it seems bad if there is a requirement for dso_local for correctness.
The way I understand this attribute is that it is useful for optimization only - i.e. it is not required for anything, but if something is actually local to a DSO, a more optimal code sequence is possible. Shouldn't it be conservatively correct to just omit dso_local from everything?

[MaskRay]

In D109090#3113552, @dim wrote:

Hmm this seems to cause some trouble when building kernels and kernel modules on FreeBSD, which are built in -ffreestanding mode, and then you get weird relocations in kernel modules:

clang13  -O2 -pipe -fno-common  -fno-strict-aliasing -Werror -D_KERNEL -DKLD_MODULE -DKLD_TIED -nostdinc   -DHAVE_KERNEL_OPTION_HEADERS -include /usr/home/kostik/work/DEV/src/sys/amd64/compile/X/opt_global.h -I. -I/usr/home/kostik/work/DEV/src/sys -I/usr/home/kostik/work/DEV/src/sys/contrib/ck/include -fno-common -gdwarf-2 -fno-omit-frame-pointer -mno-omit-leaf-frame-pointer -fdebug-prefix-map=./machine=/usr/home/kostik/work/DEV/src/sys/amd64/include -fdebug-prefix-map=./x86=/usr/home/kostik/work/DEV/src/sys/x86/include -I/usr/home/kostik/work/DEV/src/sys/amd64/compile/X     -MD  -MF.depend.ufs_vnops.o -MTufs_vnops.o -mcmodel=kernel -mno-red-zone -mno-mmx -mno-sse -msoft-float  -fno-asynchronous-unwind-tables -ffreestanding -fwrapv -fstack-protector -Wall -Wredundant-decls -Wnested-externs -Wstrict-prototypes -Wmissing-prototypes -Wpointer-arith -Wcast-qual -Wundef -Wno-pointer-sign -D__printf__=__freebsd_kprintf__ -Wmissing-include-dirs -fdiagnostics-show-option -Wno-unknown-pragmas -Wno-error=tautological-compare -Wno-error=empty-body -Wno-error=parentheses-equality -Wno-error=unused-function -Wno-error=pointer-sign -Wno-error=shift-negative-value -Wno-address-of-packed-member -Wno-error=unused-but-set-variable -Wno-format-zero-length   -mno-aes -mno-avx  -std=iso9899:1999 -c /usr/home/kostik/work/DEV/src/sys/ufs/ufs/ufs_vnops.c -o ufs_vnops.o

% readelf -a ../src/sys/amd64/compile/X/modules/usr/home/kostik/work/DEV/src/sys/modules/ufs/ufs_vnops.o | grep GOT | head -3
0000000001f7  004a0000002a R_X86_64_REX_GOTP 0000000000000000 __stack_chk_guard - 4
00000000060d  004a0000002a R_X86_64_REX_GOTP 0000000000000000 __stack_chk_guard - 4
0000000007c8  004a0000002a R_X86_64_REX_GOTP 0000000000000000 __stack_chk_guard - 4

Maybe this should have an additional check for freestanding mode?

readelf -Wr/llvm-readobj -r prints the full name.

The relocation type name is R_X86_64_REX_GOTPCRELX, which is correct (if the definition resides in libc.so) to avoid copy relocations.
Ideally the relevant FreeBSD code should handle the relocation type. It can be handled the same way as R_X86_64_GOTPCREL if no optimization is planned.
-fno-pic programs may use GOT-generating relocations as well (https://maskray.me/blog/2021-08-29-all-about-global-offset-table#undefined%20symbols).

[MaskRay]

In D109090#3115210, @nemanjai wrote:

In D109090#3113552, @dim wrote:

Hmm this seems to cause some trouble when building kernels and kernel modules on FreeBSD, which are built in -ffreestanding mode, and then you get weird relocations in kernel modules:

% readelf -a ../src/sys/amd64/compile/X/modules/usr/home/kostik/work/DEV/src/sys/modules/ufs/ufs_vnops.o | grep GOT | head -3
0000000001f7 004a0000002a R_X86_64_REX_GOTP 0000000000000000 stack_chk_guard - 4
00000000060d 004a0000002a R_X86_64_REX_GOTP 0000000000000000 stack_chk_guard - 4
0000000007c8 004a0000002a R_X86_64_REX_GOTP 0000000000000000 __stack_chk_guard - 4

Maybe this should have an additional check for freestanding mode?

Hmm, something seems odd here. I don't claim to really understand what is going on or why these relocations are a problem, but it seems bad if there is a requirement for dso_local for correctness.
The way I understand this attribute is that it is useful for optimization only - i.e. it is not required for anything, but if something is actually local to a DSO, a more optimal code sequence is possible. Shouldn't it be conservatively correct to just omit dso_local from everything?

This is also a good choice. Most popular targets (OS x arch) have switched TLS for the stack canary. Targets using the global variable may get a minor code size hit for -fno-pic mode, but that may be acceptable.
In the long term, this choice will also benefit elimination of copy relocations which some folks are fond of.

[adalava]

In D109090#3113552, @dim wrote:

Hmm this seems to cause some trouble when building kernels and kernel modules on FreeBSD, which are built in -ffreestanding mode, and then you get weird relocations in kernel modules:

@dim , I created https://reviews.llvm.org/D113443 to restricting the logic to PPC64, where original regression was confirmed and proved a safe workaround. With this amend the logic will be the same as the temporary patch added to FreeBSD 14-CURRENT in August.

llvm/lib/CodeGen/TargetLoweringBase.cpp

 
 // Currently only support "standard" __stack_chk_guard.
 // TODO: add LOAD_STACK_GUARD support.
 void TargetLoweringBase::insertSSPDeclarations(Module &M) const {
   if (!M.getNamedValue("__stack_chk_guard")) {
     auto *GV = new GlobalVariable(M, Type::getInt8PtrTy(M.getContext()), false,
                                   GlobalVariable::ExternalLinkage, nullptr,
                                   "__stack_chk_guard");
+
+    // FreeBSD has "__stack_chk_guard" defined externally on libc.so
     if (TM.getRelocationModel() == Reloc::Static &&
-        !TM.getTargetTriple().isWindowsGNUEnvironment())
+        !TM.getTargetTriple().isWindowsGNUEnvironment() &&
+        !TM.getTargetTriple().isOSFreeBSD())
       GV->setDSOLocal(true);
   }
 }
 
 // Currently only support "standard" __stack_chk_guard.
 // TODO: add LOAD_STACK_GUARD support.
 Value *TargetLoweringBase::getSDagStackGuard(const Module &M) const {
   return M.getNamedValue("__stack_chk_guard");

llvm/test/CodeGen/PowerPC/stack-protector.ll

 ; RUN: llc -verify-machineinstrs -mtriple=powerpc-unknown-linux < %s | FileCheck -check-prefix=LINUX32 %s
 ; RUN: llc -verify-machineinstrs -mtriple=powerpc64-unknown-linux < %s | FileCheck -check-prefix=LINUX64 %s
 ; RUN: llc -verify-machineinstrs -mtriple=powerpc64le-unknown-linux < %s | FileCheck -check-prefix=LINUX64 %s
 ; RUN: llc -verify-machineinstrs -mtriple=powerpc-ibm-aix-xcoff < %s | FileCheck -check-prefix=AIX32 %s
 ; RUN: llc -verify-machineinstrs -mtriple=powerpc64-ibm-aix-xcoff < %s | FileCheck -check-prefix=AIX64 %s
+; RUN: llc -verify-machineinstrs -mtriple=powerpc-unknown-freebsd13 < %s | FileCheck -check-prefix=FREEBSD32 %s
+; RUN: llc -verify-machineinstrs -mtriple=powerpc64-unknown-freebsd13 < %s | FileCheck -check-prefix=FREEBSD64 %s
+; RUN: llc -verify-machineinstrs -mtriple=powerpc64le-unknown-freebsd13 < %s | FileCheck -check-prefix=FREEBSD64 %s
 
 ; LINUX32: lwz {{[0-9]+}}, -28680(2)
 ; LINUX64: ld {{[0-9]+}}, -28688(13)
 ; AIX32: lwz {{.*}}__ssp_canary_word
 ; AIX64: ld {{.*}}__ssp_canary_word
+; FREEBSD32: lwz {{[0-9]+}}, __stack_chk_guard@l(29)
+; FREEBSD64: ld {{[0-9]+}}, .LC0@toc@l(4)
 
 ; LINUX32: __stack_chk_fail
 ; LINUX64: __stack_chk_fail
 ; AIX32: __stack_chk_fail
 ; AIX64: __stack_chk_fail
+; FREEBSD32: bl __stack_chk_fail
+; FREEBSD64: bl __stack_chk_fail
 
 @"\01LC" = internal constant [11 x i8] c"buf == %s\0A\00"    ; <[11 x i8]*> [#uses=1]
 
 define void @test(i8* %a) nounwind ssp {
 entry:
   %a_addr = alloca i8*    ; <i8**> [#uses=2]
   %buf = alloca [8 x i8]    ; <[8 x i8]*> [#uses=2]
   %"alloca point" = bitcast i32 0 to i32    ; <i32> [#uses=0]