[IRSymTab] Mark __stack_chk_guard used

[ychen]

Summary

__stack_chk_guard is a global variable that has no uses before the LLVM code generation phase (how it is defined is platform-dependent). LTO needs to preserve this symbol for that reason. Currently, legacy LTO API preserves it by hardcoding the logic in Internalizer, but this symbol is not preserved by regular LTO API in thinlink phase. This patch marks __stack_chk_guard used during IR symbol table writing since this is how builtin functions are preserved by thinlink by using RuntimeLibcalls.def.

[MaskRay]

Can __stack_chk_fail be removed from llvm/lib/Transforms/IPO/Internalize.cpp?

[ychen]

In D112595#3089372, @MaskRay wrote:

Can __stack_chk_fail be removed from llvm/lib/Transforms/IPO/Internalize.cpp?

It would not be removed there. Internalize.cpp preserves both __stack_chk_fail and the guard symbol.

[tejohnson]

In D112595#3089374, @ychen wrote:

In D112595#3089372, @MaskRay wrote:

Can __stack_chk_fail be removed from llvm/lib/Transforms/IPO/Internalize.cpp?

It would not be removed there. Internalize.cpp preserves both __stack_chk_fail and the guard symbol.

I'm confused after reading the referenced bug, which is about doing something to be able to remove this value's hardcoding from Internalize.cpp. Does the newly added test case fail without the IRSymtab change in this patch? There are a couple other symbols handled specially in Internalize.cpp (stack_chk_fail, and ssp_canary_word for AIX), should the all be handled uniformly?

[MaskRay]

LG

llvm/lib/Object/IRSymtab.cpp

51

Add a comma so that a follow-up doesn't need to patch this line.

llvm/test/ThinLTO/X86/builtin-nostrip.ll

31

llvm-nm sorts the symbols alphabetically by default. No need for -DAG

[ychen]

handle __ssp_canary_word

[MaskRay]

__stack_chk_guard is a user-defined variable

Can be more accurate. For some architectures glibc define the variable (https://sourceware.org/bugzilla/show_bug.cgi?id=26817)

There are a couple other symbols handled specially in Internalize.cpp (stack_chk_fail, and ssp_canary_word for AIX), should the all be handled uniformly?

Echo this request. Would'be great to make the logic centralized, but may not be in this patch if you think not fit.

[ychen]

• Address comments

[ychen]

In D112595#3090548, @tejohnson wrote:

In D112595#3089374, @ychen wrote:

In D112595#3089372, @MaskRay wrote:

Can __stack_chk_fail be removed from llvm/lib/Transforms/IPO/Internalize.cpp?

It would not be removed there. Internalize.cpp preserves both __stack_chk_fail and the guard symbol.

I'm confused after reading the referenced bug, which is about doing something to be able to remove this value's hardcoding from Internalize.cpp.

Apology for the confusion. I was not clear. Looking at PR28061 again, I think this patch does *NOT* fix PR28061: they are related but separate issues. (patch summary updated)
UpdateCompilerUsed/Internalizer deals with builtin symbols preservation for regular LTO using legacy LTO API; "RuntimeLibcalls.def" deals with builtin function symbols preservation for the new LTO API, but not builtin global variable like __stack_chk_guard.

Does the newly added test case fail without the IRSymtab change in this patch?

Yes, it does fail without this patch.

There are a couple other symbols handled specially in Internalize.cpp (stack_chk_fail, and ssp_canary_word for AIX), should the all be handled uniformly?

__stack_chk_fail is listed in "RuntimeLibcalls.def". All symbols in "RuntimeLibcalls.def" is marked used already. Just added handling&test for __ssp_canary_word.

[ychen]

In D112595#3091582, @MaskRay wrote:

__stack_chk_guard is a user-defined variable

Can be more accurate. For some architectures glibc define the variable (https://sourceware.org/bugzilla/show_bug.cgi?id=26817)

Yes, I've updated the summary.

There are a couple other symbols handled specially in Internalize.cpp (stack_chk_fail, and ssp_canary_word for AIX), should the all be handled uniformly?

Echo this request. Would'be great to make the logic centralized, but may not be in this patch if you think not fit.

Hmm, I think the only place that needs this logic is IRSymTab; "RuntimeLibcalls.def" and Internalizer cover the rest of the use cases (ThinLTO, legacy LTO API).

[tejohnson]

In D112595#3092240, @ychen wrote:

In D112595#3091582, @MaskRay wrote:

__stack_chk_guard is a user-defined variable

Can be more accurate. For some architectures glibc define the variable (https://sourceware.org/bugzilla/show_bug.cgi?id=26817)

Yes, I've updated the summary.

There are a couple other symbols handled specially in Internalize.cpp (stack_chk_fail, and ssp_canary_word for AIX), should the all be handled uniformly?

Echo this request. Would'be great to make the logic centralized, but may not be in this patch if you think not fit.

Hmm, I think the only place that needs this logic is IRSymTab; "RuntimeLibcalls.def" and Internalizer cover the rest of the use cases (ThinLTO, legacy LTO API).

Ok I follow now. The reason why legacy (Thin)LTO and new ThinLTO work is that they invoke Internalize.cpp to perform the internalization. However, regular LTO with the new LTO API performs internalization itself based solely on the symbol resolutions which derive from IRSymTab info.

llvm/lib/Object/IRSymtab.cpp

44

I think perhaps the name of this array and the variable it is used to set (IsBuiltinFunc) ought to change.

[ychen]

• Address Teresa's comment.

[ychen]

• fix comment

[tejohnson]

lgtm

[ychen]

Thanks for the review!

llvm/lib/Object/IRSymtab.cpp

 #include <cassert>
 #include <string>
 #include <utility>
 #include <vector>
 
 using namespace llvm;
 using namespace irsymtab;
 
-static const char *LibcallRoutineNames[] = {
+static const char *PreservedSymbols[] = {
 #define HANDLE_LIBCALL(code, name) name,
 #include "llvm/IR/RuntimeLibcalls.def"
 #undef HANDLE_LIBCALL
+    // There are global variables, so put it here instead of in
+    // RuntimeLibcalls.def.
+    // TODO: Are there similar such variables?
+    "__ssp_canary_word",
+    "__stack_chk_guard",
 };
 
 namespace {
 
 const char *getExpectedProducerName() {
   static char DefaultName[] = LLVM_VERSION_STRING
 #ifdef LLVM_REVISION
       " " LLVM_REVISION
if (!GV) {
     if (Flags & object::BasicSymbolRef::SF_Undefined)
       Sym.Flags |= 1 << storage::Symbol::FB_used;
     setStr(Sym.IRName, "");
     return Error::success();
   }
 
   setStr(Sym.IRName, GV->getName());
 
-  bool IsBuiltinFunc = llvm::is_contained(LibcallRoutineNames, GV->getName());
+  bool IsPreservedSymbol = llvm::is_contained(PreservedSymbols, GV->getName());
 
-  if (Used.count(GV) || IsBuiltinFunc)
+  if (Used.count(GV) || IsPreservedSymbol)
     Sym.Flags |= 1 << storage::Symbol::FB_used;
   if (GV->isThreadLocal())
     Sym.Flags |= 1 << storage::Symbol::FB_tls;
   if (GV->hasGlobalUnnamedAddr())
     Sym.Flags |= 1 << storage::Symbol::FB_unnamed_addr;
   if (GV->canBeOmittedFromSymbolTable())
     Sym.Flags |= 1 << storage::Symbol::FB_may_omit;
   Sym.Flags |= unsigned(GV->getVisibility()) << storage::Symbol::FB_visibility;

llvm/test/ThinLTO/X86/builtin-nostrip-aix.ll

-; REQUIRES: x86-registered-target
+; REQUIRES: powerpc-registered-target
 ; Compile with thinlto indices, to enable thinlto.
 ; RUN: opt -module-summary %s -o %t1.bc
 
 ; Test old lto interface with thinlto.
 ; RUN: llvm-lto -exported-symbol=main -thinlto-action=run %t1.bc
-; RUN: llvm-nm %t1.bc.thinlto.o | FileCheck %s --check-prefix=CHECK-NM
+; RUN: llvm-nm %t1.bc | FileCheck %s --check-prefix=CHECK-NM
 
 ; Test new lto interface with thinlto.
 ; RUN: llvm-lto2 run %t1.bc -o %t.out -save-temps \
 ; RUN:   -r %t1.bc,bar,pl \
+; RUN:   -r %t1.bc,__ssp_canary_word,pl \
 ; RUN:   -r %t1.bc,__stack_chk_fail,pl
-; RUN: llvm-nm %t.out.1 | FileCheck %s --check-prefix=CHECK-NM
+; RUN: llvm-nm %t.out.1.2.internalize.bc | FileCheck %s --check-prefix=CHECK-NM
 
 ; Re-compile, this time without the thinlto indices.
 ; RUN: opt %s -o %t4.bc
 
 ; Test the new lto interface without thinlto.
 ; RUN: llvm-lto2 run %t4.bc -o %t5.out -save-temps \
 ; RUN:   -r %t4.bc,bar,pl \
+; RUN:   -r %t4.bc,__ssp_canary_word,pl \
 ; RUN:   -r %t4.bc,__stack_chk_fail,pl
-; RUN: llvm-nm %t5.out.0 | FileCheck %s --check-prefix=CHECK-NM
+; RUN: llvm-nm %t5.out.0.2.internalize.bc | FileCheck %s --check-prefix=CHECK-NM
 
 ; Test the old lto interface without thinlto.
-; RUN: llvm-lto -exported-symbol=main %t4.bc -o %t6
-; RUN: llvm-nm %t6 | FileCheck %s --check-prefix=CHECK-NM
+; RUN: llvm-lto -exported-symbol=main -save-merged-module %t4.bc -o %t6
+; RUN: llvm-nm %t6.merged.bc | FileCheck %s --check-prefix=CHECK-NM
 
-; CHECK-NM-NOT: bar
+; CHECK-NM: D __ssp_canary_word
 ; CHECK-NM: T __stack_chk_fail
-; CHECK-NM-NOT: bar
 
-target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
-target triple = "x86_64-unknown-linux-gnu"
+target datalayout = "E-m:a-p:32:32-i64:64-n32"
+target triple = "powerpc-ibm-aix-xcoff"
 
 define void @bar() {
     ret void
 }
 
+@__ssp_canary_word = dso_local global i64 1, align 8
+
 define void @__stack_chk_fail() {
     ret void
 }

llvm/test/ThinLTO/X86/builtin-nostrip.ll

 ; REQUIRES: x86-registered-target
 ; Compile with thinlto indices, to enable thinlto.
 ; RUN: opt -module-summary %s -o %t1.bc
 
 ; Test old lto interface with thinlto.
 ; RUN: llvm-lto -exported-symbol=main -thinlto-action=run %t1.bc
 ; RUN: llvm-nm %t1.bc.thinlto.o | FileCheck %s --check-prefix=CHECK-NM
 
 ; Test new lto interface with thinlto.
 ; RUN: llvm-lto2 run %t1.bc -o %t.out -save-temps \
 ; RUN:   -r %t1.bc,bar,pl \
+; RUN:   -r %t1.bc,__stack_chk_guard,pl \
 ; RUN:   -r %t1.bc,__stack_chk_fail,pl
 ; RUN: llvm-nm %t.out.1 | FileCheck %s --check-prefix=CHECK-NM
 
 ; Re-compile, this time without the thinlto indices.
 ; RUN: opt %s -o %t4.bc
 
 ; Test the new lto interface without thinlto.
 ; RUN: llvm-lto2 run %t4.bc -o %t5.out -save-temps \
 ; RUN:   -r %t4.bc,bar,pl \
+; RUN:   -r %t4.bc,__stack_chk_guard,pl \
 ; RUN:   -r %t4.bc,__stack_chk_fail,pl
 ; RUN: llvm-nm %t5.out.0 | FileCheck %s --check-prefix=CHECK-NM
 
 ; Test the old lto interface without thinlto.
 ; RUN: llvm-lto -exported-symbol=main %t4.bc -o %t6
 ; RUN: llvm-nm %t6 | FileCheck %s --check-prefix=CHECK-NM
 
 ; CHECK-NM-NOT: bar
 ; CHECK-NM: T __stack_chk_fail
+; CHECK-NM: D __stack_chk_guard
 ; CHECK-NM-NOT: bar
 
 target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
 
 define void @bar() {
     ret void
 }
 
+@__stack_chk_guard = dso_local global i64 1, align 8
+
 define void @__stack_chk_fail() {
     ret void
 }