[codegen]Ensure __builtin_trap() has an unreachable - #197789
Conversation
|
@llvm/pr-subscribers-backend-amdgpu @llvm/pr-subscribers-clang Author: Vy Nguyen (oontvoo) ChangesPreviously, this would generate this: Now we'll have Full diff: https://github.com/llvm/llvm-project/pull/197789.diff 1 Files Affected:
diff --git a/clang/lib/CodeGen/CGBuiltin.cpp b/clang/lib/CodeGen/CGBuiltin.cpp
index 20f99f489c55f..678985ebb55e1 100644
--- a/clang/lib/CodeGen/CGBuiltin.cpp
+++ b/clang/lib/CodeGen/CGBuiltin.cpp
@@ -4022,6 +4022,12 @@ RValue CodeGenFunction::EmitBuiltinExpr(const GlobalDecl GD, unsigned BuiltinID,
}
case Builtin::BI__builtin_trap:
EmitTrapCall(Intrinsic::trap);
+ if (Builder.GetInsertBlock()) {
+ Builder.CreateUnreachable();
+ // Dummy block for the ret void - it'll be clened up
+ llvm::BasicBlock *DeadBB = createBasicBlock("dead.trap");
+ EmitBlock(DeadBB);
+ }
return RValue::get(nullptr);
case Builtin::BI__builtin_verbose_trap: {
llvm::DILocation *TrapLocation = Builder.getCurrentDebugLocation();
|
🐧 Linux x64 Test Results
✅ The build succeeded and all tests passed. |
🪟 Windows x64 Test Results
✅ The build succeeded and all tests passed. |
|
I guess we can do this... but what's the benefit? |
We are currently relying on the optimization pipeline to deduce that whenever a trap function is called there will be an unreachable following it. We already know from the get-go that after a trap comes an unreachable, so it would be nice to know this information when optimizations are disabled for analysis sake. |
At |
efriedma-quic
left a comment
There was a problem hiding this comment.
That doesn't really seem like much of a benefit, but the cost is also pretty small, so fine, I guess.
Regression tests are still failing.
| } | ||
|
|
||
| // CHECK-LABEL: define dso_local void @"?call_var_args@@YAXXZ"() | ||
| // FIXME all of these are after the trap so they should be gone. |
There was a problem hiding this comment.
I don't think this is something we plan to fix... not sure the FIXME is useful.
| // CHECK: call void @llvm.debugtrap() #[[ATTR0]] | ||
| // CHECK: call void @llvm.trap() #[[ATTR0]] | ||
| // CHECK: call void @_ZN1AD1Ev(ptr {{.*}}) #[[ATTR1]] | ||
| // CHECK: call void @llvm.trap() #[[ATTR8:[0-9]+]] |
There was a problem hiding this comment.
This breaks what this is supposed to be testing.
Please split the call to __builtin_trap() into a separate function.
ojhunt
left a comment
There was a problem hiding this comment.
Rather than special casing these I feel like we should have at least EmitTrapCall() automatically plant the unreachable if the requested intrinsic is a no return/no throw (and attach the various no throw, etc flags if they do need to be set directly on the call).
That would also catch the verbose traps, etc.
Honestly if we're doing this in debug builds I'd almost just have EmitCall plant a trap + unreachable after noreturn functions.
Done |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
@ojhunt I've made the requested change. Please have another look. Thank you. |
ojhunt
left a comment
There was a problem hiding this comment.
Just a minor renaming - I'm really surprised that there is not a single "builtin_trap" test case anywhere.
I think it would be worth it to add one, with different targets - thoughts are:
- -O0 vs -O1 (I think you get a debug? trap in some cases - it would be good to find such path and ensure that it does not get the unreachable, non-returning, etc attributes)
- Something like wasm (maybe?), or some other platform where trap is implemented as exceptions or something other than a hardware trap
done |
|
@ojhunt HI, ping? |
|
@ojhunt @efriedma-quic @jyknight Hi, friendly ping again? |
```
void test2() {
__builtin_trap();
}
```
Previously, this would generate this:
```
; Function Attrs: mustprogress noinline nounwind optnone uwtable
define dso_local void @test2()() llvm#2 !dbg !14 {
call void @llvm.trap(), !dbg !15
ret void, !dbg !16
}
```
Now we'll have `unreachable` after the trap
```
; Function Attrs: noinline nounwind optnone uwtable
define dso_local void @test2() #0 {
call void @llvm.trap()
unreachable
}
```
|
This is causing test failures in some build modes (I think release build modes hide it) for the attr-cpuspecific tests. ubsan explains it: w/o ubsan: Similar for attr-cpuspecific.cpp and attr-cpuspecific-outoflinedefs.cpp |
HLSL added a bunch of diagnostics for entry functions that messed with the matrix test, see here: #184892 The solution is to just mark the test as a library triple. Builtin-trap stopped translating in classic codegen, so the call after it is now missing (and there is an unreachable after it),w hich actually better matches our behavior see: #197789 This patch fixes both tests.
HLSL added a bunch of diagnostics for entry functions that messed with the matrix test, see here: llvm/llvm-project#184892 The solution is to just mark the test as a library triple. Builtin-trap stopped translating in classic codegen, so the call after it is now missing (and there is an unreachable after it),w hich actually better matches our behavior see: llvm/llvm-project#197789 This patch fixes both tests.
HLSL added a bunch of diagnostics for entry functions that messed with the matrix test, see here: llvm/llvm-project#184892 The solution is to just mark the test as a library triple. Builtin-trap stopped translating in classic codegen, so the call after it is now missing (and there is an unreachable after it),w hich actually better matches our behavior see: llvm/llvm-project#197789 This patch fixes both tests.
```
void test2() {
__builtin_trap();
}
```
Previously, this would generate this:
```
; Function Attrs: mustprogress noinline nounwind optnone uwtable
define dso_local void @test2()() #2 !dbg !14 {
call void @llvm.trap(), !dbg !15
ret void, !dbg !16
}
```
Now we'll have `unreachable` after the trap
```
; Function Attrs: noinline nounwind optnone uwtable
define dso_local void @test2() #0 {
call void @llvm.trap()
unreachable
}
```
HLSL added a bunch of diagnostics for entry functions that messed with the matrix test, see here: llvm#184892 The solution is to just mark the test as a library triple. Builtin-trap stopped translating in classic codegen, so the call after it is now missing (and there is an unreachable after it),w hich actually better matches our behavior see: llvm#197789 This patch fixes both tests.
Hmm... There's no reason that the resolver call CAN'T set the CurFn I think, and probably should. I'll work up a quick patch to do so, hopefully it won't have horrific impact on the cpu-specific tests. |
Commit llvm#197789 manages to try to reference CurFn while generating a trap, which some of the multiversion resolvers do. This patch makes sure we set it to the resolver so we are inserting stuff into the right places. This somewhat shockingly causes no tests to fail, but it SHOULD stop the UBSan failures.
Commit #197789 manages to try to reference CurFn while generating a trap, which some of the multiversion resolvers do. This patch makes sure we set it to the resolver so we are inserting stuff into the right places. This somewhat shockingly causes no tests to fail, but it SHOULD stop the UBSan failures.
Previously, this would generate this:
Now we'll have
unreachableafter the trap