[LowerTypeTests] Emit cfi_jt aliases regardless of function export

[nickdesaulniers]

Summary

A constant complaint we get is that the typeid symbols in the CFI
jump tables causes confusing stack traces in applications. Emit the more
readable cfi_jt aliases regardless of function export (LTO vs Thin LTO).

[samitolvanen]

Thanks, Nick. I tested a kernel build with both ThinLTO and regular LTO + CFI, and can confirm that with this patch vmlinux has the previously missing .cfi_jt symbols again.

[tejohnson]

llvm/lib/Transforms/IPO/LowerTypeTests.cpp

1566

Where and how does the new alias symbol get used in the LTO case? I only see it used when importing in the backend in the ThinLTO case.

[pcc]

compiler-rt/test/cfi/cfi_jt_aliases.c

1 ↗

(On Diff #365863)

This isn't an execution test so I don't think it should be together with the other execution tests. Also I think it will fail on non-ELF platforms because you're using llvm-readelf etc. It seems better to make this a test of the LTO pipeline (see e.g. llvm/test/ThinLTO/X86/cache-icall.ll).

llvm/lib/Transforms/IPO/LowerTypeTests.cpp

1566

I think the idea is that these symbols only exist for debugging purposes.

I think this could be cleaned up a bit though to ensure that the symbols aren't being exported unnecessarily. Can we set the linkage to internal for non-exported symbols and add to llvm.used to ensure that the symbols aren't dropped?

[samitolvanen]

causes confusing stack traces in applications

Peter also pointed out that jump table symbols don't actually show up in stack traces, but in the kernel's case, everywhere else where we need to print out a function name, including ftrace.

[kees]

Ping; Nick, do you have a moment to rework this patch? AIUI, this is still important to getting CFI more sane for Android.

[nickdesaulniers]

• rebase, convert compiler-rt test to ThinLTO test. TODO: llvm.used and linkage

[nickdesaulniers]

• emit cfi_jt symbols as InternalLinkage and add them to llvm.used

[nickdesaulniers]

llvm/lib/Transforms/IPO/LowerTypeTests.cpp

1566

I've added this; I've fixed up one of the regressions it causes, llvm/test/Transforms/LowerTypeTests/export-icall.ll, but I'm not sure about the other two:

1. compiler-rt/test/cfi/icall/external-call.c
2. compiler-rt/test/cfi/icall/weak.c

[nickdesaulniers]

llvm/lib/Transforms/IPO/LowerTypeTests.cpp

1566

Specifically, it seems that the use of GlobalValue::InternalLinkage rather than GlobalValue::ExternalLinkage is what causes those two tests to regress.

[nickdesaulniers]

• change linkage to internal only if !exported, as @pcc said

[nickdesaulniers]

• use M rather than *F->getParent(), store Functions[I]->isExported() in local var

[samitolvanen]

I tested the latest version with a CFI kernel build, and everything still looks good. I see the same number of .cfi_jt symbols with ThinLTO and full LTO.

[tejohnson]

llvm/lib/Transforms/IPO/LowerTypeTests.cpp

1575

This should only be needed if it is internal linkage, to prevent the dropping. With external linkage it can't be dropped anyway.

[tejohnson]

llvm/test/ThinLTO/X86/cfi_jt_aliases.ll

1 ↗

(On Diff #379572)

I can't quite tell - is this checking both the exported and non-exported cases? Comments would be helpful, and maybe explicitly checking llvm.used contents in the IR for the non-exported case.

[nickdesaulniers]

• only retain used for internal linkage, add LTO test for @llvm.used

llvm/test/ThinLTO/X86/cfi_jt_aliases.ll

1 ↗

(On Diff #379572)

The exported case should be full LTO, while the non-exported case should be thin LTO. I've copied this test to llvm/test/LTO/... as well, and added the test for @llvm.used there. PTAL.

[tejohnson]

llvm/test/ThinLTO/X86/cfi_jt_aliases.ll

1 ↗

(On Diff #379572)

The exported case should be full LTO, while the non-exported case should be thin LTO. I've copied this test to llvm/test/LTO/... as well, and added the test for @llvm.used there. PTAL.

Did you state this backwards, since the full LTO case is checking for the llvm.used which must mean it is non-exported? Or am I misinterpreting?

[nickdesaulniers]

llvm/test/ThinLTO/X86/cfi_jt_aliases.ll

1 ↗

(On Diff #379572)

*egg on face*

Yes; you're right.

1 ↗

(On Diff #379572)

ie.

The exported case should be thin LTO, while the non-exported case should be full LTO. I've copied this test to llvm/test/LTO/... as well, and added the test for @llvm.used there. PTAL.

[tejohnson]

lgtm

[pcc]

llvm/test/LTO/X86/cfi_jt_aliases.ll

2

Can this file and the ThinLTO one be combined into a single file?

17

Shouldn't it be checking for a relocation pointing to a? (Same below.)

[nickdesaulniers]

• add check next lines to check the relocations

llvm/test/LTO/X86/cfi_jt_aliases.ll

2

Is there prior art for tests shared between llvm/test/LTO/ and llvm/test/ThinLTO/?

The IR is identical between the two.

The RUN lines differ slightly in the following ways:

• additional opt flags for thin LTO
• -save-temps for full LTO's invocation of llvm-lto2
• additional llvm-dis invocation for full LTO checking one of those saved temporaries.

Perhaps I could put the IR in an Inputs/ dir somewhere, then use that (not use %s) as input to opt?

[nickdesaulniers]

@pcc mentioned offline that I should combine test files, and double up the RUN lines.

[nickdesaulniers]

• combine test files into one source

[pcc]

LGTM

llvm/lib/Transforms/IPO/LowerTypeTests.cpp

for (unsigned I = 0; I != Functions.size(); ++I) {
       bool IsJumpTableCanonical = Functions[I]->isJumpTableCanonical();
 
       Constant *CombinedGlobalElemPtr = ConstantExpr::getBitCast(
           ConstantExpr::getInBoundsGetElementPtr(
               JumpTableType, JumpTable,
               ArrayRef<Constant *>{ConstantInt::get(IntPtrTy, 0),
                                    ConstantInt::get(IntPtrTy, I)}),
           F->getType());
-      if (Functions[I]->isExported()) {
-        if (IsJumpTableCanonical) {
-          ExportSummary->cfiFunctionDefs().insert(std::string(F->getName()));
-        } else {
-          GlobalAlias *JtAlias = GlobalAlias::create(
-              F->getValueType(), 0, GlobalValue::ExternalLinkage,
-              F->getName() + ".cfi_jt", CombinedGlobalElemPtr, &M);
+
+      const bool IsExported = Functions[I]->isExported();
+      if (!IsJumpTableCanonical) {
+        GlobalValue::LinkageTypes LT = IsExported
+                                           ? GlobalValue::ExternalLinkage
+                                           : GlobalValue::InternalLinkage;
+        GlobalAlias *JtAlias = GlobalAlias::create(F->getValueType(), 0, LT,
+                                                   F->getName() + ".cfi_jt",
+                                                   CombinedGlobalElemPtr, &M);
+        if (IsExported)
           JtAlias->setVisibility(GlobalValue::HiddenVisibility);
-          ExportSummary->cfiFunctionDecls().insert(std::string(F->getName()));
-        }
+        else
+          appendToUsed(M, {JtAlias});
+
+      if (IsExported) {
+        if (IsJumpTableCanonical)
+          ExportSummary->cfiFunctionDefs().insert(std::string(F->getName()));
+        else
+          ExportSummary->cfiFunctionDecls().insert(std::string(F->getName()));
       }
+
       if (!IsJumpTableCanonical) {
         if (F->hasExternalWeakLinkage())
           replaceWeakDeclarationWithJumpTablePtr(F, CombinedGlobalElemPtr,
                                                  IsJumpTableCanonical);
         else
           replaceCfiUses(F, CombinedGlobalElemPtr, IsJumpTableCanonical);
       } else {
         assert(F->getType()->getAddressSpace() == 0);

llvm/test/LTO/X86/cfi_jt_aliases.ll

+; Full LTO test
+; RUN: opt %s -o %t.bc
+; RUN: llvm-lto2 run -o %t.o %t.bc -save-temps \
+; RUN:   -r=%t.bc,a,px \
+; RUN:   -r=%t.bc,b,px \
+; RUN:   -r=%t.bc,c,px \
+; RUN:   -r=%t.bc,d,px
+; RUN: llvm-readelf --symbols %t.o.0 | grep \.cfi_jt | FileCheck --check-prefix=CHECK-FULL-RE %s
+; RUN: llvm-objdump -dr %t.o.0 | FileCheck --check-prefix=CHECK-FULL-OD %s
+; RUN: llvm-dis %t.o.0.4.opt.bc -o - | FileCheck --check-prefix=CHECK-USED %s
+; Thin LTO test
+; RUN: opt -thinlto-bc -thinlto-split-lto-unit %s -o %t.bc
+; RUN: llvm-lto2 run -o %t.o %t.bc \
+; RUN:   -r=%t.bc,a,px \
+; RUN:   -r=%t.bc,b,px \
+; RUN:   -r=%t.bc,c,px \
+; RUN:   -r=%t.bc,d,px
+; RUN: llvm-readelf --symbols %t.o.0 | grep \.cfi_jt | FileCheck --check-prefix=CHECK-THIN-RE %s
+; RUN: llvm-objdump -dr %t.o.0 | FileCheck --check-prefix=CHECK-THIN-OD %s
+
+; CHECK-FULL-RE:      FUNC LOCAL DEFAULT {{[0-9]+}} a.cfi_jt
+; CHECK-FULL-RE-NEXT: FUNC LOCAL DEFAULT {{[0-9]+}} b.cfi_jt
+; CHECK-FULL-RE-NEXT: FUNC LOCAL DEFAULT {{[0-9]+}} c.cfi_jt
+
+; CHECK-THIN-RE:      FUNC GLOBAL HIDDEN {{[0-9]+}} b.cfi_jt
+; CHECK-THIN-RE-NEXT: FUNC GLOBAL HIDDEN {{[0-9]+}} c.cfi_jt
+; CHECK-THIN-RE-NEXT: FUNC GLOBAL HIDDEN {{[0-9]+}} a.cfi_jt
+
+; CHECK-FULL-OD:      a.cfi_jt>:
+; CHECK-FULL-OD:      jmp {{.*}} <a.cfi_jt
+; CHECK-FULL-OD-NEXT: R_X86_64_PLT32 .La$local
+; CHECK-FULL-OD:      b.cfi_jt>:
+; CHECK-FULL-OD:      jmp {{.*}} <b.cfi_jt
+; CHECK-FULL-OD-NEXT: R_X86_64_PLT32 .Lb$local
+; CHECK-FULL-OD:      c.cfi_jt>:
+; CHECK-FULL-OD:      jmp {{.*}} <c.cfi_jt
+; CHECK-FULL-OD-NEXT: R_X86_64_PLT32 .Lc$local
+
+; CHECK-THIN-OD:      b.cfi_jt>:
+; CHECK-THIN-OD:      jmp {{.*}} <b.cfi_jt
+; CHECK-THIN-OD-NEXT: R_X86_64_PLT32 b
+; CHECK-THIN-OD:      c.cfi_jt>:
+; CHECK-THIN-OD:      jmp {{.*}} <c.cfi_jt
+; CHECK-THIN-OD-NEXT: R_X86_64_PLT32 c
+; CHECK-THIN-OD:      a.cfi_jt>:
+; CHECK-THIN-OD:      jmp {{.*}} <a.cfi_jt
+; CHECK-THIN-OD-NEXT: R_X86_64_PLT32 a
+
+; CHECK-USED: @llvm.used = appending global [3 x i8*] [i8* bitcast (void ()* @a.cfi_jt to i8*), i8* bitcast (void ()* @b.cfi_jt to i8*), i8* bitcast (void ()* @c.cfi_jt to i8*)], section "llvm.metadata"
+
+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"
+
+@f = internal global [4 x void ()*] [void ()* @a, void ()* @b, void ()* @c, void ()* null], align 16
+
+define dso_local void @a() !type !5 !type !6 { ret void }
+define dso_local void @b() !type !5 !type !6 { ret void }
+define dso_local void @c() !type !5 !type !6 { ret void }
+
+define dso_local void @d() !type !5 !type !6 {
+entry:
+  %i = alloca i32, align 4
+  store i32 0, i32* %i, align 4
+  br label %for.cond
+
+for.cond:                                         ; preds = %for.inc, %entry
+  %0 = load i32, i32* %i, align 4
+  %idxprom = sext i32 %0 to i64
+  %arrayidx = getelementptr inbounds [4 x void ()*], [4 x void ()*]* @f, i64 0, i64 %idxprom
+  %1 = load void ()*, void ()** %arrayidx, align 8
+  %tobool = icmp ne void ()* %1, null
+  br i1 %tobool, label %for.body, label %for.end
+
+for.body:                                         ; preds = %for.cond
+  %2 = load i32, i32* %i, align 4
+  %idxprom1 = sext i32 %2 to i64
+  %arrayidx2 = getelementptr inbounds [4 x void ()*], [4 x void ()*]* @f, i64 0, i64 %idxprom1
+  %3 = load void ()*, void ()** %arrayidx2, align 8
+  %4 = bitcast void ()* %3 to i8*, !nosanitize !7
+  %5 = call i1 @llvm.type.test(i8* %4, metadata !"_ZTSFvvE"), !nosanitize !7
+  br i1 %5, label %cont, label %trap, !nosanitize !7
+
+trap:                                             ; preds = %for.body
+  call void @llvm.ubsantrap(i8 2), !nosanitize !7
+  unreachable, !nosanitize !7
+
+cont:                                             ; preds = %for.body
+  call void %3()
+  br label %for.inc
+
+for.inc:                                          ; preds = %cont
+  %6 = load i32, i32* %i, align 4
+  %inc = add nsw i32 %6, 1
+  store i32 %inc, i32* %i, align 4
+  br label %for.cond
+
+for.end:                                          ; preds = %for.cond
+  ret void
+}
+
+declare i1 @llvm.type.test(i8*, metadata)
+declare void @llvm.ubsantrap(i8 immarg)
+
+!llvm.module.flags = !{!0, !1, !2, !3}
+
+!0 = !{i32 1, !"wchar_size", i32 4}
+!1 = !{i32 4, !"CFI Canonical Jump Tables", i32 0}
+!2 = !{i32 7, !"uwtable", i32 1}
+!3 = !{i32 7, !"frame-pointer", i32 2}
+!5 = !{i64 0, !"_ZTSFvvE"}
+!6 = !{i64 0, !"_ZTSFvvE.generalized"}
+!7 = !{}