[Clang] Fix nomerge attribute not working with __builtin_trap(), __debugbreak(), __builtin_verbose_trap() #101549
Conversation
This file contains hidden or 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
…bugbreak(), __builtin_verbose_trap()
llvmbot
added
clang
|
@llvm/pr-subscribers-backend-x86 @llvm/pr-subscribers-llvm-selectiondag Author: Zequan Wu (ZequanWu) Changes
This is a copy of https://reviews.llvm.org/D146164. This only attempts to fixe Fixes #53011 Full diff: https://github.com/llvm/llvm-project/pull/101549.diff 4 Files Affected:
diff --git a/clang/lib/CodeGen/CGExpr.cpp b/clang/lib/CodeGen/CGExpr.cpp
index 5f58a64d8386c..9f18642ebbbf2 100644
--- a/clang/lib/CodeGen/CGExpr.cpp
+++ b/clang/lib/CodeGen/CGExpr.cpp
@@ -3883,6 +3883,8 @@ llvm::CallInst *CodeGenFunction::EmitTrapCall(llvm::Intrinsic::ID IntrID) {
TrapCall->addFnAttr(A);
}
+ if (InNoMergeAttributedStmt)
+ TrapCall->addFnAttr(llvm::Attribute::NoMerge);
return TrapCall;
}
diff --git a/clang/test/CodeGen/attr-nomerge.cpp b/clang/test/CodeGen/attr-nomerge.cpp
index 7305fb73cf1dc..b643920adde49 100644
--- a/clang/test/CodeGen/attr-nomerge.cpp
+++ b/clang/test/CodeGen/attr-nomerge.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -emit-llvm %s -triple x86_64-unknown-linux-gnu -o - | FileCheck %s
+// RUN: %clang_cc1 -emit-llvm %s -fms-extensions -o - | FileCheck %s
class A {
public:
@@ -42,6 +42,9 @@ void foo(int i, A *ap, B *bp) {
A *newA = new B();
delete newA;
+ [[clang::nomerge]] __builtin_trap();
+ [[clang::nomerge]] __debugbreak();
+ [[clang::nomerge]] __builtin_verbose_trap("check null", "Argument must not be null.");
}
int g(int i);
@@ -97,6 +100,9 @@ void something_else_again() {
// CHECK: load ptr, ptr
// CHECK: %[[AG:.*]] = load ptr, ptr
// CHECK-NEXT: call void %[[AG]](ptr {{.*}}) #[[ATTR1]]
+// CHECK: call void @llvm.trap() #[[ATTR0]]
+// CHECK: call void @llvm.debugtrap() #[[ATTR0]]
+// CHECK: call void @llvm.trap() #[[ATTR0]]
// CHECK: call void @_ZN1AD1Ev(ptr {{.*}}) #[[ATTR1]]
// CHECK-DAG: attributes #[[ATTR0]] = {{{.*}}nomerge{{.*}}}
diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
index 9f5e6466309e9..a035fee6aafca 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
@@ -7448,6 +7448,8 @@ void SelectionDAGBuilder::visitIntrinsicCall(const CallInst &I,
break;
default: llvm_unreachable("unknown trap intrinsic");
}
+ DAG.addNoMergeSiteInfo(DAG.getRoot().getNode(),
+ I.hasFnAttr(Attribute::NoMerge));
return;
}
TargetLowering::ArgListTy Args;
@@ -7464,7 +7466,7 @@ void SelectionDAGBuilder::visitIntrinsicCall(const CallInst &I,
DAG.getExternalSymbol(TrapFuncName.data(),
TLI.getPointerTy(DAG.getDataLayout())),
std::move(Args));
-
+ CLI.NoMerge = I.hasFnAttr(Attribute::NoMerge);
std::pair<SDValue, SDValue> Result = TLI.LowerCallTo(CLI);
DAG.setRoot(Result.second);
return;
diff --git a/llvm/test/CodeGen/X86/nomerge.ll b/llvm/test/CodeGen/X86/nomerge.ll
index efbadf5b6911f..d3e6feae87108 100644
--- a/llvm/test/CodeGen/X86/nomerge.ll
+++ b/llvm/test/CodeGen/X86/nomerge.ll
@@ -62,4 +62,96 @@ if.end:
declare dso_local void @bar()
+define void @nomerge_trap(i32 %i) {
+; CHECK-LABEL: nomerge_trap:
+; CHECK: # %bb.0: # %entry
+; CHECK: # %bb.1: # %entry
+; CHECK: # %bb.2: # %if.then
+; CHECK-NEXT: ud2
+; CHECK-NEXT: LBB1_3: # %if.then2
+; CHECK-NEXT: ud2
+; CHECK-NEXT: .LBB1_4: # %if.end3
+; CHECK-NEXT: ud2
+entry:
+ switch i32 %i, label %if.end3 [
+ i32 5, label %if.then
+ i32 7, label %if.then2
+ ]
+
+if.then:
+ tail call void @llvm.trap() #0
+ unreachable
+
+if.then2:
+ tail call void @llvm.trap() #0
+ unreachable
+
+if.end3:
+ tail call void @llvm.trap() #0
+ unreachable
+}
+
+declare dso_local void @llvm.trap()
+
+define void @nomerge_debugtrap(i32 %i) {
+; CHECK-LABEL: nomerge_debugtrap:
+; CHECK: # %bb.0: # %entry
+; CHECK: # %bb.1: # %entry
+; CHECK: # %bb.2: # %if.then
+; CHECK-NEXT: int3
+; CHECK-NEXT: LBB2_3: # %if.then2
+; CHECK-NEXT: int3
+; CHECK-NEXT: .LBB2_4: # %if.end3
+; CHECK-NEXT: int3
+entry:
+ switch i32 %i, label %if.end3 [
+ i32 5, label %if.then
+ i32 7, label %if.then2
+ ]
+
+if.then:
+ tail call void @llvm.debugtrap() #0
+ unreachable
+
+if.then2:
+ tail call void @llvm.debugtrap() #0
+ unreachable
+
+if.end3:
+ tail call void @llvm.debugtrap() #0
+ unreachable
+}
+
+define void @nomerge_named_debugtrap(i32 %i) {
+; CHECK-LABEL: nomerge_named_debugtrap:
+; CHECK: # %bb.0: # %entry
+; CHECK: # %bb.1: # %entry
+; CHECK: # %bb.2: # %if.then
+; CHECK-NEXT: callq trap_func@PLT
+; CHECK-NEXT: LBB3_3: # %if.then2
+; CHECK-NEXT: callq trap_func@PLT
+; CHECK-NEXT: .LBB3_4: # %if.end3
+; CHECK-NEXT: callq trap_func@PLT
+entry:
+ switch i32 %i, label %if.end3 [
+ i32 5, label %if.then
+ i32 7, label %if.then2
+ ]
+
+if.then:
+ tail call void @llvm.debugtrap() #1
+ unreachable
+
+if.then2:
+ tail call void @llvm.debugtrap() #1
+ unreachable
+
+if.end3:
+ tail call void @llvm.debugtrap() #1
+ unreachable
+}
+
+declare dso_local void @llvm.debugtrap()
+
attributes #0 = { nomerge }
+attributes #1 = { nomerge "trap-func-name"="trap_func" }
|
|
@llvm/pr-subscribers-clang-codegen Author: Zequan Wu (ZequanWu) Changes
This is a copy of https://reviews.llvm.org/D146164. This only attempts to fixe Fixes #53011 Full diff: https://github.com/llvm/llvm-project/pull/101549.diff 4 Files Affected:
diff --git a/clang/lib/CodeGen/CGExpr.cpp b/clang/lib/CodeGen/CGExpr.cpp
index 5f58a64d8386c..9f18642ebbbf2 100644
--- a/clang/lib/CodeGen/CGExpr.cpp
+++ b/clang/lib/CodeGen/CGExpr.cpp
@@ -3883,6 +3883,8 @@ llvm::CallInst *CodeGenFunction::EmitTrapCall(llvm::Intrinsic::ID IntrID) {
TrapCall->addFnAttr(A);
}
+ if (InNoMergeAttributedStmt)
+ TrapCall->addFnAttr(llvm::Attribute::NoMerge);
return TrapCall;
}
diff --git a/clang/test/CodeGen/attr-nomerge.cpp b/clang/test/CodeGen/attr-nomerge.cpp
index 7305fb73cf1dc..b643920adde49 100644
--- a/clang/test/CodeGen/attr-nomerge.cpp
+++ b/clang/test/CodeGen/attr-nomerge.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -emit-llvm %s -triple x86_64-unknown-linux-gnu -o - | FileCheck %s
+// RUN: %clang_cc1 -emit-llvm %s -fms-extensions -o - | FileCheck %s
class A {
public:
@@ -42,6 +42,9 @@ void foo(int i, A *ap, B *bp) {
A *newA = new B();
delete newA;
+ [[clang::nomerge]] __builtin_trap();
+ [[clang::nomerge]] __debugbreak();
+ [[clang::nomerge]] __builtin_verbose_trap("check null", "Argument must not be null.");
}
int g(int i);
@@ -97,6 +100,9 @@ void something_else_again() {
// CHECK: load ptr, ptr
// CHECK: %[[AG:.*]] = load ptr, ptr
// CHECK-NEXT: call void %[[AG]](ptr {{.*}}) #[[ATTR1]]
+// CHECK: call void @llvm.trap() #[[ATTR0]]
+// CHECK: call void @llvm.debugtrap() #[[ATTR0]]
+// CHECK: call void @llvm.trap() #[[ATTR0]]
// CHECK: call void @_ZN1AD1Ev(ptr {{.*}}) #[[ATTR1]]
// CHECK-DAG: attributes #[[ATTR0]] = {{{.*}}nomerge{{.*}}}
diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
index 9f5e6466309e9..a035fee6aafca 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
@@ -7448,6 +7448,8 @@ void SelectionDAGBuilder::visitIntrinsicCall(const CallInst &I,
break;
default: llvm_unreachable("unknown trap intrinsic");
}
+ DAG.addNoMergeSiteInfo(DAG.getRoot().getNode(),
+ I.hasFnAttr(Attribute::NoMerge));
return;
}
TargetLowering::ArgListTy Args;
@@ -7464,7 +7466,7 @@ void SelectionDAGBuilder::visitIntrinsicCall(const CallInst &I,
DAG.getExternalSymbol(TrapFuncName.data(),
TLI.getPointerTy(DAG.getDataLayout())),
std::move(Args));
-
+ CLI.NoMerge = I.hasFnAttr(Attribute::NoMerge);
std::pair<SDValue, SDValue> Result = TLI.LowerCallTo(CLI);
DAG.setRoot(Result.second);
return;
diff --git a/llvm/test/CodeGen/X86/nomerge.ll b/llvm/test/CodeGen/X86/nomerge.ll
index efbadf5b6911f..d3e6feae87108 100644
--- a/llvm/test/CodeGen/X86/nomerge.ll
+++ b/llvm/test/CodeGen/X86/nomerge.ll
@@ -62,4 +62,96 @@ if.end:
declare dso_local void @bar()
+define void @nomerge_trap(i32 %i) {
+; CHECK-LABEL: nomerge_trap:
+; CHECK: # %bb.0: # %entry
+; CHECK: # %bb.1: # %entry
+; CHECK: # %bb.2: # %if.then
+; CHECK-NEXT: ud2
+; CHECK-NEXT: LBB1_3: # %if.then2
+; CHECK-NEXT: ud2
+; CHECK-NEXT: .LBB1_4: # %if.end3
+; CHECK-NEXT: ud2
+entry:
+ switch i32 %i, label %if.end3 [
+ i32 5, label %if.then
+ i32 7, label %if.then2
+ ]
+
+if.then:
+ tail call void @llvm.trap() #0
+ unreachable
+
+if.then2:
+ tail call void @llvm.trap() #0
+ unreachable
+
+if.end3:
+ tail call void @llvm.trap() #0
+ unreachable
+}
+
+declare dso_local void @llvm.trap()
+
+define void @nomerge_debugtrap(i32 %i) {
+; CHECK-LABEL: nomerge_debugtrap:
+; CHECK: # %bb.0: # %entry
+; CHECK: # %bb.1: # %entry
+; CHECK: # %bb.2: # %if.then
+; CHECK-NEXT: int3
+; CHECK-NEXT: LBB2_3: # %if.then2
+; CHECK-NEXT: int3
+; CHECK-NEXT: .LBB2_4: # %if.end3
+; CHECK-NEXT: int3
+entry:
+ switch i32 %i, label %if.end3 [
+ i32 5, label %if.then
+ i32 7, label %if.then2
+ ]
+
+if.then:
+ tail call void @llvm.debugtrap() #0
+ unreachable
+
+if.then2:
+ tail call void @llvm.debugtrap() #0
+ unreachable
+
+if.end3:
+ tail call void @llvm.debugtrap() #0
+ unreachable
+}
+
+define void @nomerge_named_debugtrap(i32 %i) {
+; CHECK-LABEL: nomerge_named_debugtrap:
+; CHECK: # %bb.0: # %entry
+; CHECK: # %bb.1: # %entry
+; CHECK: # %bb.2: # %if.then
+; CHECK-NEXT: callq trap_func@PLT
+; CHECK-NEXT: LBB3_3: # %if.then2
+; CHECK-NEXT: callq trap_func@PLT
+; CHECK-NEXT: .LBB3_4: # %if.end3
+; CHECK-NEXT: callq trap_func@PLT
+entry:
+ switch i32 %i, label %if.end3 [
+ i32 5, label %if.then
+ i32 7, label %if.then2
+ ]
+
+if.then:
+ tail call void @llvm.debugtrap() #1
+ unreachable
+
+if.then2:
+ tail call void @llvm.debugtrap() #1
+ unreachable
+
+if.end3:
+ tail call void @llvm.debugtrap() #1
+ unreachable
+}
+
+declare dso_local void @llvm.debugtrap()
+
attributes #0 = { nomerge }
+attributes #1 = { nomerge "trap-func-name"="trap_func" }
|
rnk
approved these changes
Aug 1, 2024
There was a problem hiding this comment.
Richard had a comment on the original which I think it still relevant:
There are 496 calls to Builder.CreateCall in clang's CodeGen. Do they all need this change? If not, how can we be confident we've found all the ones that do? (From a quick check, at least MSVC's __fastfail builtin seems like it would also benefit from this handling.)
Would it be reasonable to make clang's CGBuilder do this for every call instruction we create?
I think we could still do this, but we should fix this long outstanding bug first.
Yeah, it would be easier if there's one place to attach statement attributes on all expressions inside that statement. |
|
Hi, Error message: and Failed builder task: https://ci.chromium.org/ui/p/fuchsia/builders/toolchain.ci/clang-base-linux-x64/b8740776112052324513/overview Could you revert your change and reland it after fixing it please? |
|
Thanks for reporting. Those tests are failed due to not updated properly. I relanded this change with updated tests: ae6dc64. |
Your updated change still seems to cause the test
Can you either fix or revert if you need time to investigate? |
Hopefully fixed by c5f1395. If not, perhaps a fixed |
Thanks for the fix! |
The test still appears to be failing after the fix. |
6c375ae should fix it. |
thurstond
added a commit
to thurstond/llvm-project
that referenced
this pull request
Nov 25, 2024
…e-trap.ll llvm/test/MC/AArch64/local-bounds-single-trap.ll was introduced in https://github.com/llvm/llvm-project/pull/65972/files to demonstrate that nomerge did not work properly, which is documented in the header comment. llvm#101549 fixed nomerge for trap builtins and llvm@ae6dc64 updated the test assertions, but not the header comment. This patch updates the header comment accordingly.
thurstond
added a commit
to thurstond/llvm-project
that referenced
this pull request
Nov 26, 2024
This test demonstrates that UBSan does not add the nomerge annotation. This is significant because it results in them being merged by the backend. N.B. llvm#65972 (continuation of https://reviews.llvm.org/D148654) had considered adding nomerge to ubsantrap, but did not proceed with that because of llvm#53011. llvm#101549 fixed that limitation ("It sets nomerge flag for the node if the instruction has nomerge arrtibute."); planned upcoming will add nomerge for ubsan.
thurstond
added a commit
to thurstond/llvm-project
that referenced
this pull request
Nov 26, 2024
llvm#65972 (continuation of https://reviews.llvm.org/D148654) had considered adding nomerge to ubsantrap, but did not proceed with that because of llvm#53011. Instead, it added a counter (based on TrapBB->getParent()->size()) to each ubsantrap call. However, this counter is not guaranteed to be unique after inlining, as shown by llvm#83470, which can result in ubsantraps being merged by the backend. llvm#101549 fixed has since fixed the nomerge limitation ("It sets nomerge flag for the node if the instruction has nomerge arrtibute."). This patch therefore takes advantage of nomerge instead of using the counter, guaranteeing that the ubsantraps are not merged. This patch is equivalent to llvm#83470 but also adds nomerge and updates the test that was precommitted in llvm#117649.
thurstond
added a commit
that referenced
this pull request
Nov 26, 2024
…single-trap.ll (#117642) llvm/test/MC/AArch64/local-bounds-single-trap.ll was introduced in #65972 to demonstrate that nomerge did not work properly, which is documented in the header comment. #101549 fixed nomerge for trap builtins and ae6dc64 updated the test assertions, but not the header comment. This patch updates the header comment accordingly.
thurstond
added a commit
that referenced
this pull request
Nov 26, 2024
…117649) This test (copied from #83470) demonstrates that UBSan does not add the nomerge annotation. This is significant because it can result in them being merged by the backend, even when -ubsan-unique-traps is enabled. N.B. #65972 (continuation of https://reviews.llvm.org/D148654) had considered adding nomerge to ubsantrap, but did not proceed with that because of #53011. #101549 fixed that limitation ("It sets nomerge flag for the node if the instruction has nomerge arrtibute."); planned upcoming work (#117651) will add nomerge for ubsan.
thurstond
added a commit
to thurstond/llvm-project
that referenced
this pull request
Nov 26, 2024
llvm#65972 (continuation of https://reviews.llvm.org/D148654) had considered adding nomerge to ubsantrap, but did not proceed with that because of llvm#53011. Instead, it added a counter (based on TrapBB->getParent()->size()) to each ubsantrap call. However, this counter is not guaranteed to be unique after inlining, as shown by llvm#83470, which can result in ubsantraps being merged by the backend. llvm#101549 fixed has since fixed the nomerge limitation ("It sets nomerge flag for the node if the instruction has nomerge arrtibute."). This patch therefore takes advantage of nomerge instead of using the counter, guaranteeing that the ubsantraps are not merged. This patch is equivalent to llvm#83470 but also adds nomerge and updates the test that was precommitted in llvm#117649.
thurstond
added a commit
to thurstond/llvm-project
that referenced
this pull request
Nov 26, 2024
…nomerge" (llvm#117804) This reverts commit c8bdb31. It was reverted because I forgot to update the auto-generated assertions after adding the target triple. Original commit message: This test (copied from llvm#83470) demonstrates that UBSan does not add the nomerge annotation. This is significant because it can result in them being merged by the backend, even when -ubsan-unique-traps is enabled. N.B. llvm#65972 (continuation of https://reviews.llvm.org/D148654) had considered adding nomerge to ubsantrap, but did not proceed with that because of llvm#53011. llvm#101549 fixed that limitation ("It sets nomerge flag for the node if the instruction has nomerge arrtibute."); planned upcoming work (llvm#117651) will add nomerge for ubsan.
thurstond
added a commit
that referenced
this pull request
Nov 26, 2024
…nomerge" (#117804) (#117805) This reverts commit c8bdb31. It was reverted because I forgot to update the auto-generated assertions after adding the target triple. Original commit message: This test (copied from #83470) demonstrates that UBSan does not add the nomerge annotation. This is significant because it can result in them being merged by the backend, even when -ubsan-unique-traps is enabled. N.B. #65972 (continuation of https://reviews.llvm.org/D148654) had considered adding nomerge to ubsantrap, but did not proceed with that because of #53011. #101549 fixed that limitation ("It sets nomerge flag for the node if the instruction has nomerge arrtibute."); planned upcoming work (#117651) will add nomerge for ubsan.
thurstond
added a commit
to thurstond/llvm-project
that referenced
this pull request
Nov 26, 2024
llvm#65972 (continuation of https://reviews.llvm.org/D148654) had considered adding nomerge to ubsantrap, but did not proceed with that because of llvm#53011. Instead, it added a counter (based on TrapBB->getParent()->size()) to each ubsantrap call. However, this counter is not guaranteed to be unique after inlining, as shown by llvm#83470, which can result in ubsantraps being merged by the backend. llvm#101549 fixed has since fixed the nomerge limitation ("It sets nomerge flag for the node if the instruction has nomerge arrtibute."). This patch therefore takes advantage of nomerge instead of using the counter, guaranteeing that the ubsantraps are not merged. This patch is equivalent to llvm#83470 but also adds nomerge and updates the test that was precommitted in llvm#117649.
thurstond
added a commit
that referenced
this pull request
Nov 27, 2024
…117651) #65972 (continuation of https://reviews.llvm.org/D148654) had considered adding nomerge to ubsantrap, but did not proceed with that because of #53011. Instead, it added a counter (based on TrapBB->getParent()->size()) to each ubsantrap call. However, this counter is not guaranteed to be unique after inlining, as shown by #83470, which can result in ubsantraps being merged by the backend. #101549 has since fixed the nomerge limitation ("It sets nomerge flag for the node if the instruction has nomerge arrtibute."). This patch therefore takes advantage of nomerge instead of using the counter, guaranteeing that the ubsantraps are not merged. This patch is equivalent to #83470 but also adds nomerge and updates tests (#117649: ubsan-trap-merge.c; #117657: ubsan-trap-merge.ll, ubsan-trap-nomerge.ll; catch-undef-behavior.c).
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.
This is a copy of https://reviews.llvm.org/D146164. This only attempts to fix
nomergefor__builtin_trap(),__debugbreak(),__builtin_verbose_trap(), not working for non-trap builtins.Fixes #53011