[CFI] Fix Direct Call Issues in CFI Dispatch Table #69663
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-llvm-transforms Author: Oskar Wirga (oskarwirga) ChangesI discovered two issues for when a CFI dispatch table entry is used as a direct call. InliningThere is the possibility that the dispatch table entry contains only a single function pointer: If this function is inlined, the unreachable follows and ruins the containing function. Exception HandlingThe dispatch table is always marked NoUnwind. This is fine if the entries are never used directly, but if a direct call is used which the containing function expects to throw, it will no longer throw and the exception handling code will be lost. Full diff: https://github.com/llvm/llvm-project/pull/69663.diff 5 Files Affected:
diff --git a/llvm/lib/Transforms/IPO/LowerTypeTests.cpp b/llvm/lib/Transforms/IPO/LowerTypeTests.cpp
index 2bf0fc2a4a84459..ad6495a5669c9b6 100644
--- a/llvm/lib/Transforms/IPO/LowerTypeTests.cpp
+++ b/llvm/lib/Transforms/IPO/LowerTypeTests.cpp
@@ -500,7 +500,7 @@ class LowerTypeTestsModule {
Type *getJumpTableEntryType();
void createJumpTableEntry(raw_ostream &AsmOS, raw_ostream &ConstraintOS,
Triple::ArchType JumpTableArch,
- SmallVectorImpl<Value *> &AsmArgs, Function *Dest);
+ SmallVectorImpl<Value *> &AsmArgs, Function *Dest, bool& doesEntryHaveNoUnwind);
void verifyTypeMDNode(GlobalObject *GO, MDNode *Type);
void buildBitSetsFromFunctions(ArrayRef<Metadata *> TypeIds,
ArrayRef<GlobalTypeMember *> Functions);
@@ -1254,7 +1254,10 @@ unsigned LowerTypeTestsModule::getJumpTableEntrySize() {
void LowerTypeTestsModule::createJumpTableEntry(
raw_ostream &AsmOS, raw_ostream &ConstraintOS,
Triple::ArchType JumpTableArch, SmallVectorImpl<Value *> &AsmArgs,
- Function *Dest) {
+ Function *Dest, bool& doesEntryHaveNoUnwind) {
+ if (Dest->hasFnAttribute(llvm::Attribute::NoUnwind))
+ doesEntryHaveNoUnwind = true;
+
unsigned ArgIndex = AsmArgs.size();
if (JumpTableArch == Triple::x86 || JumpTableArch == Triple::x86_64) {
@@ -1470,10 +1473,11 @@ void LowerTypeTestsModule::createJumpTable(
raw_string_ostream AsmOS(AsmStr), ConstraintOS(ConstraintStr);
SmallVector<Value *, 16> AsmArgs;
AsmArgs.reserve(Functions.size() * 2);
+ bool doesEntryHaveNoUnwind = false;
for (GlobalTypeMember *GTM : Functions)
createJumpTableEntry(AsmOS, ConstraintOS, JumpTableArch, AsmArgs,
- cast<Function>(GTM->getGlobal()));
+ cast<Function>(GTM->getGlobal()), doesEntryHaveNoUnwind);
// Align the whole table by entry size.
F->setAlignment(Align(getJumpTableEntrySize()));
@@ -1516,8 +1520,13 @@ void LowerTypeTestsModule::createJumpTable(
// -fcf-protection=.
if (JumpTableArch == Triple::x86 || JumpTableArch == Triple::x86_64)
F->addFnAttr(Attribute::NoCfCheck);
- // Make sure we don't emit .eh_frame for this function.
- F->addFnAttr(Attribute::NoUnwind);
+
+ // Make sure we don't emit .eh_frame for this function if it isn't needed.
+ if(doesEntryHaveNoUnwind)
+ F->addFnAttr(Attribute::NoUnwind);
+
+ // Make sure we do not inline any calls to the cfi.jumptable.
+ F->addFnAttr(Attribute::NoInline);
BasicBlock *BB = BasicBlock::Create(M.getContext(), "entry", F);
IRBuilder<> IRB(BB);
diff --git a/llvm/test/Transforms/LowerTypeTests/aarch64-jumptable.ll b/llvm/test/Transforms/LowerTypeTests/aarch64-jumptable.ll
index 139df60a7b916f4..3464a748778b668 100644
--- a/llvm/test/Transforms/LowerTypeTests/aarch64-jumptable.ll
+++ b/llvm/test/Transforms/LowerTypeTests/aarch64-jumptable.ll
@@ -1,3 +1,4 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --check-attributes --include-generated-funcs --version 2
; RUN: opt -S -passes=lowertypetests -mtriple=aarch64-unknown-linux-gnu %s | FileCheck --check-prefixes=AARCH64 %s
; Test for the jump table generation with branch protection on AArch64
@@ -6,7 +7,6 @@ target datalayout = "e-p:64:64"
@0 = private unnamed_addr constant [2 x ptr] [ptr @f, ptr @g], align 16
-; AARCH64: @f = alias void (), ptr @[[JT:.*]]
define void @f() !type !0 {
ret void
@@ -29,11 +29,30 @@ define i1 @foo(ptr %p) {
!1 = !{i32 4, !"branch-target-enforcement", i32 1}
-; AARCH64: define private void @[[JT]]() #[[ATTR:.*]] align 8 {
-; AARCH64: bti c
-; AARCH64-SAME: b $0
-; AARCH64-SAME: bti c
-; AARCH64-SAME: b $1
-
-; AARCH64: attributes #[[ATTR]] = { naked nounwind "branch-target-enforcement"="false" "sign-return-address"="none"
+; AARCH64-LABEL: define hidden void @f.cfi() !type !1 {
+; AARCH64-NEXT: ret void
+;
+;
+; AARCH64-LABEL: define internal void @g.cfi() !type !1 {
+; AARCH64-NEXT: ret void
+;
+;
+; AARCH64-LABEL: define i1 @foo
+; AARCH64-SAME: (ptr [[P:%.*]]) {
+; AARCH64-NEXT: [[TMP1:%.*]] = ptrtoint ptr [[P]] to i64
+; AARCH64-NEXT: [[TMP2:%.*]] = sub i64 [[TMP1]], ptrtoint (ptr @.cfi.jumptable to i64)
+; AARCH64-NEXT: [[TMP3:%.*]] = lshr i64 [[TMP2]], 3
+; AARCH64-NEXT: [[TMP4:%.*]] = shl i64 [[TMP2]], 61
+; AARCH64-NEXT: [[TMP5:%.*]] = or i64 [[TMP3]], [[TMP4]]
+; AARCH64-NEXT: [[TMP6:%.*]] = icmp ule i64 [[TMP5]], 1
+; AARCH64-NEXT: ret i1 [[TMP6]]
+;
+;
+; AARCH64: Function Attrs: naked noinline
+; AARCH64-LABEL: define private void @.cfi.jumptable
+; AARCH64-SAME: () #[[ATTR1:[0-9]+]] align 8 {
+; AARCH64-NEXT: entry:
+; AARCH64-NEXT: call void asm sideeffect "bti c\0Ab $0\0Abti c\0Ab $1\0A", "s,s"(ptr @f.cfi, ptr @g.cfi)
+; AARCH64-NEXT: unreachable
+;
diff --git a/llvm/test/Transforms/LowerTypeTests/cfi-unwind-direct-call.ll b/llvm/test/Transforms/LowerTypeTests/cfi-unwind-direct-call.ll
new file mode 100644
index 000000000000000..d729f9bfaf20fdf
--- /dev/null
+++ b/llvm/test/Transforms/LowerTypeTests/cfi-unwind-direct-call.ll
@@ -0,0 +1,217 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 2
+; RUN: opt < %s -passes='lowertypetests,default<O3>' -S | FileCheck %s
+
+; This IR is based of the following C++
+; which was compiled with:
+; clang -cc1 -fexceptions -fcxx-exceptions \
+; -std=c++11 -internal-isystem llvm-project/build/lib/clang/17/include \
+; -nostdsysteminc -triple x86_64-unknown-linux -fsanitize=cfi-icall \
+; -fsanitize-cfi-cross-dso -fsanitize-trap=cfi-icall -Oz -S -emit-llvm
+;
+; void (*catch_ptr)(int);
+;
+; void throw_e (int num) {
+; if (num) throw 20;
+; }
+;
+; void call_catch(int num) {
+; catch_ptr = &throw_e;
+; try{
+; catch_ptr(num);
+; } catch (int i) {
+; }
+; }
+
+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"
+
+@catch_ptr = local_unnamed_addr global ptr null, align 8
+@_ZTIi = external constant ptr
+@llvm.used = appending global [1 x ptr] [ptr @__cfi_check_fail], section "llvm.metadata"
+
+; Function Attrs: minsize mustprogress optsize
+define dso_local void @_Z7throw_ei(i32 noundef %num) #0 !type !4 !type !5 !type !6 {
+; CHECK-LABEL: define dso_local void @_Z7throw_ei
+; CHECK-SAME: (i32 noundef [[NUM:%.*]]) #[[ATTR0:[0-9]+]] !type !4 !type !5 !type !6 {
+; CHECK-NEXT: entry:
+; CHECK-NEXT: [[TOBOOL_NOT:%.*]] = icmp eq i32 [[NUM]], 0
+; CHECK-NEXT: br i1 [[TOBOOL_NOT]], label [[IF_END:%.*]], label [[IF_THEN:%.*]]
+; CHECK: if.then:
+; CHECK-NEXT: [[EXCEPTION:%.*]] = tail call ptr @__cxa_allocate_exception(i64 4) #[[ATTR5:[0-9]+]]
+; CHECK-NEXT: store i32 20, ptr [[EXCEPTION]], align 16, !tbaa [[TBAA7:![0-9]+]]
+; CHECK-NEXT: tail call void @__cxa_throw(ptr nonnull [[EXCEPTION]], ptr nonnull @_ZTIi, ptr null) #[[ATTR6:[0-9]+]]
+; CHECK-NEXT: unreachable
+; CHECK: if.end:
+; CHECK-NEXT: ret void
+;
+entry:
+ %tobool.not = icmp eq i32 %num, 0
+ br i1 %tobool.not, label %if.end, label %if.then
+
+if.then: ; preds = %entry
+ %exception = tail call ptr @__cxa_allocate_exception(i64 4) #5
+ store i32 20, ptr %exception, align 16, !tbaa !7
+ tail call void @__cxa_throw(ptr nonnull %exception, ptr nonnull @_ZTIi, ptr null) #6
+ unreachable
+
+if.end: ; preds = %entry
+ ret void
+}
+
+declare ptr @__cxa_allocate_exception(i64) local_unnamed_addr
+
+declare void @__cxa_throw(ptr, ptr, ptr) local_unnamed_addr
+
+; Function Attrs: minsize mustprogress optsize
+define dso_local void @_Z10call_catchi(i32 noundef %num) local_unnamed_addr #0 personality ptr @__gxx_personality_v0 !type !4 !type !5 !type !6 {
+; CHECK-LABEL: define dso_local void @_Z10call_catchi
+; CHECK-SAME: (i32 noundef [[NUM:%.*]]) local_unnamed_addr #[[ATTR0]] personality ptr @__gxx_personality_v0 !type !4 !type !5 !type !6 {
+; CHECK-NEXT: entry:
+; CHECK-NEXT: store ptr @_Z7throw_ei.cfi_jt, ptr @catch_ptr, align 8, !tbaa [[TBAA11:![0-9]+]]
+; CHECK-NEXT: invoke void @_Z7throw_ei.cfi_jt() #[[ATTR7:[0-9]+]]
+; CHECK-NEXT: to label [[TRY_CONT:%.*]] unwind label [[LPAD:%.*]]
+; CHECK: lpad:
+; CHECK-NEXT: [[TMP0:%.*]] = landingpad { ptr, i32 }
+; CHECK-NEXT: catch ptr @_ZTIi
+; CHECK-NEXT: [[TMP1:%.*]] = extractvalue { ptr, i32 } [[TMP0]], 1
+; CHECK-NEXT: [[TMP2:%.*]] = tail call i32 @llvm.eh.typeid.for(ptr nonnull @_ZTIi) #[[ATTR5]]
+; CHECK-NEXT: [[MATCHES:%.*]] = icmp eq i32 [[TMP1]], [[TMP2]]
+; CHECK-NEXT: br i1 [[MATCHES]], label [[CATCH:%.*]], label [[EH_RESUME:%.*]]
+; CHECK: catch:
+; CHECK-NEXT: [[TMP3:%.*]] = extractvalue { ptr, i32 } [[TMP0]], 0
+; CHECK-NEXT: [[TMP4:%.*]] = tail call ptr @__cxa_begin_catch(ptr [[TMP3]]) #[[ATTR5]]
+; CHECK-NEXT: tail call void @__cxa_end_catch() #[[ATTR5]]
+; CHECK-NEXT: br label [[TRY_CONT]]
+; CHECK: try.cont:
+; CHECK-NEXT: ret void
+; CHECK: eh.resume:
+; CHECK-NEXT: resume { ptr, i32 } [[TMP0]]
+;
+entry:
+ store ptr @_Z7throw_ei, ptr @catch_ptr, align 8, !tbaa !11
+ %0 = tail call i1 @llvm.type.test(ptr nonnull @_Z7throw_ei, metadata !"_ZTSFviE"), !nosanitize !13
+ br i1 %0, label %cfi.cont, label %cfi.slowpath, !prof !14, !nosanitize !13
+
+cfi.slowpath: ; preds = %entry
+ tail call void @__cfi_slowpath(i64 -8738933900360652027, ptr nonnull @_Z7throw_ei) #5, !nosanitize !13
+ br label %cfi.cont, !nosanitize !13
+
+cfi.cont: ; preds = %cfi.slowpath, %entry
+ invoke void @_Z7throw_ei(i32 noundef %num) #7
+ to label %try.cont unwind label %lpad
+
+lpad: ; preds = %cfi.cont
+ %1 = landingpad { ptr, i32 }
+ catch ptr @_ZTIi
+ %2 = extractvalue { ptr, i32 } %1, 1
+ %3 = tail call i32 @llvm.eh.typeid.for(ptr nonnull @_ZTIi) #5
+ %matches = icmp eq i32 %2, %3
+ br i1 %matches, label %catch, label %eh.resume
+
+catch: ; preds = %lpad
+ %4 = extractvalue { ptr, i32 } %1, 0
+ %5 = tail call ptr @__cxa_begin_catch(ptr %4) #5
+ tail call void @__cxa_end_catch() #5
+ br label %try.cont
+
+try.cont: ; preds = %cfi.cont, %catch
+ ret void
+
+eh.resume: ; preds = %lpad
+ resume { ptr, i32 } %1
+}
+
+; Function Attrs: mustprogress nocallback nofree nosync nounwind speculatable willreturn memory(none)
+declare i1 @llvm.type.test(ptr, metadata) #1
+
+declare void @__cfi_slowpath(i64, ptr) local_unnamed_addr
+
+declare i32 @__gxx_personality_v0(...)
+
+; Function Attrs: nofree nosync nounwind memory(none)
+declare i32 @llvm.eh.typeid.for(ptr) #2
+
+declare ptr @__cxa_begin_catch(ptr) local_unnamed_addr
+
+declare void @__cxa_end_catch() local_unnamed_addr
+
+; Function Attrs: minsize optsize
+define weak_odr hidden void @__cfi_check_fail(ptr noundef %0, ptr noundef %1) #3 {
+; CHECK-LABEL: define weak_odr hidden void @__cfi_check_fail
+; CHECK-SAME: (ptr noundef [[TMP0:%.*]], ptr noundef [[TMP1:%.*]]) #[[ATTR2:[0-9]+]] {
+; CHECK-NEXT: entry:
+; CHECK-NEXT: [[DOTNOT:%.*]] = icmp eq ptr [[TMP0]], null, !nosanitize !13
+; CHECK-NEXT: br i1 [[DOTNOT]], label [[TRAP:%.*]], label [[CONT:%.*]], !nosanitize !13
+; CHECK: trap:
+; CHECK-NEXT: tail call void @llvm.ubsantrap(i8 2) #[[ATTR8:[0-9]+]], !nosanitize !13
+; CHECK-NEXT: unreachable, !nosanitize !13
+; CHECK: cont:
+; CHECK-NEXT: [[TMP2:%.*]] = load i8, ptr [[TMP0]], align 4, !nosanitize !13
+; CHECK-NEXT: [[SWITCH:%.*]] = icmp ult i8 [[TMP2]], 5
+; CHECK-NEXT: br i1 [[SWITCH]], label [[TRAP]], label [[CONT6:%.*]]
+; CHECK: cont6:
+; CHECK-NEXT: ret void, !nosanitize !13
+;
+entry:
+ %.not = icmp eq ptr %0, null, !nosanitize !13
+ br i1 %.not, label %trap, label %cont, !nosanitize !13
+
+trap: ; preds = %cont, %entry
+ tail call void @llvm.ubsantrap(i8 2) #8, !nosanitize !13
+ unreachable, !nosanitize !13
+
+cont: ; preds = %entry
+ %2 = load i8, ptr %0, align 4, !nosanitize !13
+ %switch = icmp ult i8 %2, 5
+ br i1 %switch, label %trap, label %cont6
+
+cont6: ; preds = %cont
+ ret void, !nosanitize !13
+}
+
+; Function Attrs: cold noreturn nounwind
+declare void @llvm.ubsantrap(i8 immarg) #4
+
+define weak void @__cfi_check(i64 %0, ptr %1, ptr %2) local_unnamed_addr {
+; CHECK-LABEL: define weak void @__cfi_check
+; CHECK-SAME: (i64 [[TMP0:%.*]], ptr [[TMP1:%.*]], ptr [[TMP2:%.*]]) local_unnamed_addr {
+; CHECK-NEXT: entry:
+; CHECK-NEXT: tail call void @llvm.trap()
+; CHECK-NEXT: unreachable
+;
+entry:
+ tail call void @llvm.trap()
+ unreachable
+}
+
+; Function Attrs: cold noreturn nounwind
+declare void @llvm.trap() #4
+
+attributes #0 = { minsize mustprogress optsize "min-legal-vector-width"="0" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-features"="+cx8,+mmx,+sse,+sse2,+x87" }
+attributes #1 = { mustprogress nocallback nofree nosync nounwind speculatable willreturn memory(none) }
+attributes #2 = { nofree nosync nounwind memory(none) }
+attributes #3 = { minsize optsize "min-legal-vector-width"="0" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-features"="+cx8,+mmx,+sse,+sse2,+x87" }
+attributes #4 = { cold noreturn nounwind }
+attributes #5 = { nounwind }
+attributes #6 = { noreturn }
+attributes #7 = { minsize optsize }
+attributes #8 = { noreturn nounwind }
+
+!llvm.module.flags = !{!0, !1, !2}
+!llvm.ident = !{!3}
+
+!0 = !{i32 1, !"wchar_size", i32 4}
+!1 = !{i32 4, !"Cross-DSO CFI", i32 1}
+!2 = !{i32 4, !"CFI Canonical Jump Tables", i32 0}
+!3 = !{!"clang version 17.0.2"}
+!4 = !{i64 0, !"_ZTSFviE"}
+!5 = !{i64 0, !"_ZTSFviE.generalized"}
+!6 = !{i64 0, i64 -8738933900360652027}
+!7 = !{!8, !8, i64 0}
+!8 = !{!"int", !9, i64 0}
+!9 = !{!"omnipotent char", !10, i64 0}
+!10 = !{!"Simple C++ TBAA"}
+!11 = !{!12, !12, i64 0}
+!12 = !{!"any pointer", !9, i64 0}
+!13 = !{}
+!14 = !{!"branch_weights", i32 1048575, i32 1}
diff --git a/llvm/test/Transforms/LowerTypeTests/function.ll b/llvm/test/Transforms/LowerTypeTests/function.ll
index 5ba69e236e41355..a858aace834d6b3 100644
--- a/llvm/test/Transforms/LowerTypeTests/function.ll
+++ b/llvm/test/Transforms/LowerTypeTests/function.ll
@@ -51,7 +51,7 @@ define internal void @g() !type !0 {
!0 = !{i32 0, !"typeid1"}
-declare i1 @llvm.type.test(ptr %ptr, metadata %bitset) nounwind readnone
+declare i1 @llvm.type.test(ptr %ptr, metadata %bitset) noinline readnone
define i1 @foo(ptr %p) {
; NATIVE: sub i64 {{.*}}, ptrtoint (ptr @[[JT]] to i64)
@@ -109,13 +109,13 @@ define i1 @foo(ptr %p) {
; NATIVE-SAME: "s,s"(ptr @f.cfi, ptr @g.cfi)
-; X86-LINUX: attributes #[[ATTR]] = { naked nocf_check nounwind }
-; X86-WIN32: attributes #[[ATTR]] = { nocf_check nounwind }
-; ARM: attributes #[[ATTR]] = { naked nounwind
-; THUMB: attributes #[[ATTR]] = { naked nounwind "branch-target-enforcement"="false" "sign-return-address"="none" "target-cpu"="cortex-a8" "target-features"="+thumb-mode" }
-; THUMBV6M: attributes #[[ATTR]] = { naked nounwind "branch-target-enforcement"="false" "sign-return-address"="none" "target-features"="+thumb-mode" }
-; RISCV: attributes #[[ATTR]] = { naked nounwind "target-features"="-c,-relax" }
-; LOONGARCH64: attributes #[[ATTR]] = { naked nounwind }
+; X86-LINUX: attributes #[[ATTR]] = { naked nocf_check noinline }
+; X86-WIN32: attributes #[[ATTR]] = { nocf_check noinline }
+; ARM: attributes #[[ATTR]] = { naked noinline
+; THUMB: attributes #[[ATTR]] = { naked noinline "branch-target-enforcement"="false" "sign-return-address"="none" "target-cpu"="cortex-a8" "target-features"="+thumb-mode" }
+; THUMBV6M: attributes #[[ATTR]] = { naked noinline "branch-target-enforcement"="false" "sign-return-address"="none" "target-features"="+thumb-mode" }
+; RISCV: attributes #[[ATTR]] = { naked noinline "target-features"="-c,-relax" }
+; LOONGARCH64: attributes #[[ATTR]] = { naked noinline }
; WASM32: ![[I0]] = !{i64 1}
; WASM32: ![[I1]] = !{i64 2}
diff --git a/llvm/test/Transforms/LowerTypeTests/x86-jumptable.ll b/llvm/test/Transforms/LowerTypeTests/x86-jumptable.ll
index a88a45a65d59c28..f56d30be37959ff 100644
--- a/llvm/test/Transforms/LowerTypeTests/x86-jumptable.ll
+++ b/llvm/test/Transforms/LowerTypeTests/x86-jumptable.ll
@@ -28,4 +28,4 @@ define i1 @foo(ptr %p) {
; X86_32-NEXT: call void asm sideeffect "endbr32\0Ajmp ${0:c}@plt\0A.balign 16, 0xcc\0Aendbr32\0Ajmp ${1:c}@plt\0A.balign 16, 0xcc\0A", "s,s"(ptr @f.cfi, ptr @g.cfi)
; X86_64-NEXT: call void asm sideeffect "endbr64\0Ajmp ${0:c}@plt\0A.balign 16, 0xcc\0Aendbr64\0Ajmp ${1:c}@plt\0A.balign 16, 0xcc\0A", "s,s"(ptr @f.cfi, ptr @g.cfi)
-; X86_64: attributes #[[#ATTR]] = { naked nocf_check nounwind }
+; X86_64: attributes #[[#ATTR]] = { naked nocf_check noinline }
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
oskarwirga
force-pushed
the
ltt-dispatch-opt
branch
2 times, most recently
from
0bccb7c
to
7bc809e
Compare
|
Ping :) |
oskarwirga
commented
Oct 27, 2023
oskarwirga
force-pushed
the
ltt-dispatch-opt
branch
2 times, most recently
from
eb399e1
to
8173ccd
Compare
oskarwirga
requested review from
MaskRay,
statham-arm and
Ami-zhang
and removed request for
drodriguez
|
ping |
statham-arm
reviewed
Nov 8, 2023
|
Sorry – I'm confused. You've apparently requested that I review the change again, but it still seems to be in the same state as last time? There's some code that looks at only the first element of a jump table, and doesn't explain why, and I think it should either check the whole table or have a comment explaining why it doesn't need to. In its current state, it's confusing to readers. |
My apologies, still getting used to Github reviewing, I wanted to get consensus on my proposal here: #69663 (comment) I probably should have CC'd both of you on that comment rather than request review again. |
oskarwirga
force-pushed
the
ltt-dispatch-opt
branch
from
8173ccd
to
79b21ab
Compare
|
ping :) |
statham-arm
reviewed
Dec 5, 2023
There was a problem hiding this comment.
I'm happy with this version of the code, assuming that what we're trying to achieve is sensible. But I'm curious about @efriedma-quic 's point about tail calls. Why do we ever need EH info on a jump table at all, since it doesn't leave a trace on the call stack that needs to be unwound through?
Can you add a bit more detail to the commit message explaining what specifically goes wrong if you leave the jump table as NoUnwind?
It is because with the NoUnwind attribute, we remove exception handling code in the containing function which then results in uncaught exception crashes.
Yes absolutely! |
This commit addresses two issues related to the use of Control Flow Integrity (CFI) dispatch table entries as direct calls. Issue with Inlining: When a dispatch table entry contains only a single function pointer, it can be replaced with a direct call to the jump table. However, if this function is inlined, the unreachable instruction that follows can disrupt the control flow of the containing function. This is illustrated in the following code snippet: ```llvm ; Function Attrs: naked nocf_check define private void @.cfi.jumptable() llvm#6 align 8 { entry: call void asm sideeffect "jmp ${0:c}@plt\0Aint3\0Aint3\0Aint3\0A", "s"(ptr @_Z7throw_ei) unreachable } ``` Issue with Unwinding: When unwinding, a direct call of the jump table can cause all exception handling code of the containing function to be dropped. This occurs even if the indirectly dispatched function throws an exception which then results in unhandled exception crashes.
oskarwirga
force-pushed
the
ltt-dispatch-opt
branch
from
79b21ab
to
bea52ed
Compare
statham-arm
approved these changes
Dec 6, 2023
|
LGTM as far as I'm concerned |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I discovered two issues for when a CFI dispatch table entry is used as a direct call.
Inlining
There is the possibility that the dispatch table entry contains only a single function pointer:
If this function is inlined, the unreachable follows and ruins the containing function.
Exception Handling
The dispatch table is always marked NoUnwind. This is fine if the entries are never used directly, but if a direct call is used which the containing function expects to throw, it will no longer throw and the exception handling code will be lost.