[ubsan] Change ubsan-unique-traps to use nomerge instead of counter #117651

thurstond · 2024-11-26T00:39:20Z

#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).

llvmbot · 2024-11-26T00:39:54Z

@llvm/pr-subscribers-mc
@llvm/pr-subscribers-backend-x86

@llvm/pr-subscribers-clang

Author: Thurston Dang (thurstond)

Changes

#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 the test that was precommitted in #117649.

Full diff: https://github.com/llvm/llvm-project/pull/117651.diff

3 Files Affected:

(modified) clang/lib/CodeGen/CGExpr.cpp (+2-4)
(modified) clang/test/CodeGen/bounds-checking.c (+2-2)
(added) clang/test/CodeGen/ubsan-trap-merge.c (+106)

diff --git a/clang/lib/CodeGen/CGExpr.cpp b/clang/lib/CodeGen/CGExpr.cpp
index d3f470d401b3d4..f8c1e1cd7a4d68 100644
--- a/clang/lib/CodeGen/CGExpr.cpp
+++ b/clang/lib/CodeGen/CGExpr.cpp
@@ -3921,16 +3921,14 @@ void CodeGenFunction::EmitTrapCheck(llvm::Value *Checked,
 
     llvm::CallInst *TrapCall = Builder.CreateCall(
         CGM.getIntrinsic(llvm::Intrinsic::ubsantrap),
-        llvm::ConstantInt::get(CGM.Int8Ty,
-                               ClSanitizeDebugDeoptimization
-                                   ? TrapBB->getParent()->size()
-                                   : static_cast<uint64_t>(CheckHandlerID)));
+        llvm::ConstantInt::get(CGM.Int8Ty, CheckHandlerID));
 
     if (!CGM.getCodeGenOpts().TrapFuncName.empty()) {
       auto A = llvm::Attribute::get(getLLVMContext(), "trap-func-name",
                                     CGM.getCodeGenOpts().TrapFuncName);
       TrapCall->addFnAttr(A);
     }
+    TrapCall->addFnAttr(llvm::Attribute::NoMerge);
     TrapCall->setDoesNotReturn();
     TrapCall->setDoesNotThrow();
     Builder.CreateUnreachable();
diff --git a/clang/test/CodeGen/bounds-checking.c b/clang/test/CodeGen/bounds-checking.c
index 8100e30d0650ad..f6c4880e70a150 100644
--- a/clang/test/CodeGen/bounds-checking.c
+++ b/clang/test/CodeGen/bounds-checking.c
@@ -74,11 +74,11 @@ char B2[10];
 // CHECK-LABEL: @f8
 void f8(int i, int k) {
   // NOOPTLOCAL: call void @llvm.ubsantrap(i8 3)
-  // NOOPTARRAY: call void @llvm.ubsantrap(i8 2)
+  // NOOPTARRAY: call void @llvm.ubsantrap(i8 18)
   B[i] = '\0';
 
   // NOOPTLOCAL: call void @llvm.ubsantrap(i8 5)
-  // NOOPTARRAY: call void @llvm.ubsantrap(i8 4)
+  // NOOPTARRAY: call void @llvm.ubsantrap(i8 18)
   B2[k] = '\0';
 }
 
diff --git a/clang/test/CodeGen/ubsan-trap-merge.c b/clang/test/CodeGen/ubsan-trap-merge.c
new file mode 100644
index 00000000000000..e6aa7902262813
--- /dev/null
+++ b/clang/test/CodeGen/ubsan-trap-merge.c
@@ -0,0 +1,106 @@
+// NOTE: Assertions have mostly been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --version 5
+// The most important assertion is the attributes at the end of the file, which
+// shows that ubsan attaches 'nomerge' to each ubsantrap intrinsic.
+//
+// RUN: %clang -fsanitize=signed-integer-overflow -S -emit-llvm -fsanitize-trap=all -O3 -mllvm -ubsan-unique-traps %s -o - \
+// RUN:     | FileCheck %s
+
+#include <stdio.h>
+#include <stdlib.h>
+
+// CHECK-LABEL: define dso_local range(i32 -2147483523, -2147483648) i32 @f(
+// CHECK-SAME: i32 noundef [[X:%.*]]) local_unnamed_addr #[[ATTR0:[0-9]+]] {
+// CHECK-NEXT:  [[ENTRY:.*:]]
+// CHECK-NEXT:    [[TMP0:%.*]] = tail call { i32, i1 } @llvm.sadd.with.overflow.i32(i32 [[X]], i32 125), !nosanitize [[META5:![0-9]+]]
+// CHECK-NEXT:    [[TMP1:%.*]] = extractvalue { i32, i1 } [[TMP0]], 1, !nosanitize [[META5]]
+// CHECK-NEXT:    br i1 [[TMP1]], label %[[TRAP:.*]], label %[[CONT:.*]], !nosanitize [[META5]]
+// CHECK:       [[TRAP]]:
+// CHECK-NEXT:    tail call void @llvm.ubsantrap(i8 0) #[[ATTR4:[0-9]+]], !nosanitize [[META5]]
+// CHECK-NEXT:    unreachable, !nosanitize [[META5]]
+// CHECK:       [[CONT]]:
+// CHECK-NEXT:    [[TMP2:%.*]] = extractvalue { i32, i1 } [[TMP0]], 0, !nosanitize [[META5]]
+// CHECK-NEXT:    ret i32 [[TMP2]]
+//
+int f(int x) {
+  return x + 125;
+}
+
+// CHECK-LABEL: define dso_local range(i32 -2147483521, -2147483648) i32 @g(
+// CHECK-SAME: i32 noundef [[X:%.*]]) local_unnamed_addr #[[ATTR0]] {
+// CHECK-NEXT:  [[ENTRY:.*:]]
+// CHECK-NEXT:    [[TMP0:%.*]] = tail call { i32, i1 } @llvm.sadd.with.overflow.i32(i32 [[X]], i32 127), !nosanitize [[META5]]
+// CHECK-NEXT:    [[TMP1:%.*]] = extractvalue { i32, i1 } [[TMP0]], 1, !nosanitize [[META5]]
+// CHECK-NEXT:    br i1 [[TMP1]], label %[[TRAP:.*]], label %[[CONT:.*]], !nosanitize [[META5]]
+// CHECK:       [[TRAP]]:
+// CHECK-NEXT:    tail call void @llvm.ubsantrap(i8 0) #[[ATTR4]], !nosanitize [[META5]]
+// CHECK-NEXT:    unreachable, !nosanitize [[META5]]
+// CHECK:       [[CONT]]:
+// CHECK-NEXT:    [[TMP2:%.*]] = extractvalue { i32, i1 } [[TMP0]], 0, !nosanitize [[META5]]
+// CHECK-NEXT:    ret i32 [[TMP2]]
+//
+int g(int x) {
+  return x + 127;
+}
+
+// CHECK-LABEL: define dso_local range(i32 -2147483521, -2147483648) i32 @h(
+// CHECK-SAME: i32 noundef [[X:%.*]], i32 noundef [[Y:%.*]]) local_unnamed_addr #[[ATTR0]] {
+// CHECK-NEXT:  [[ENTRY:.*:]]
+// CHECK-NEXT:    [[TMP0:%.*]] = tail call { i32, i1 } @llvm.sadd.with.overflow.i32(i32 [[X]], i32 127), !nosanitize [[META5]]
+// CHECK-NEXT:    [[TMP1:%.*]] = extractvalue { i32, i1 } [[TMP0]], 1, !nosanitize [[META5]]
+// CHECK-NEXT:    br i1 [[TMP1]], label %[[TRAP:.*]], label %[[CONT:.*]], !nosanitize [[META5]]
+// CHECK:       [[TRAP]]:
+// CHECK-NEXT:    tail call void @llvm.ubsantrap(i8 0) #[[ATTR4]], !nosanitize [[META5]]
+// CHECK-NEXT:    unreachable, !nosanitize [[META5]]
+// CHECK:       [[CONT]]:
+// CHECK-NEXT:    [[TMP2:%.*]] = tail call { i32, i1 } @llvm.sadd.with.overflow.i32(i32 [[Y]], i32 129), !nosanitize [[META5]]
+// CHECK-NEXT:    [[TMP3:%.*]] = extractvalue { i32, i1 } [[TMP2]], 1, !nosanitize [[META5]]
+// CHECK-NEXT:    br i1 [[TMP3]], label %[[TRAP1:.*]], label %[[CONT2:.*]], !nosanitize [[META5]]
+// CHECK:       [[TRAP1]]:
+// CHECK-NEXT:    tail call void @llvm.ubsantrap(i8 0) #[[ATTR4]], !nosanitize [[META5]]
+// CHECK-NEXT:    unreachable, !nosanitize [[META5]]
+// CHECK:       [[CONT2]]:
+// CHECK-NEXT:    [[TMP4:%.*]] = extractvalue { i32, i1 } [[TMP2]], 0, !nosanitize [[META5]]
+// CHECK-NEXT:    [[TMP5:%.*]] = extractvalue { i32, i1 } [[TMP0]], 0, !nosanitize [[META5]]
+// CHECK-NEXT:    [[COND:%.*]] = tail call i32 @llvm.smin.i32(i32 [[TMP5]], i32 [[TMP4]])
+// CHECK-NEXT:    ret i32 [[COND]]
+//
+int h(int x, int y) {
+  x += 127;
+  y += 129;
+  return x < y ? x : y;
+}
+
+// CHECK-LABEL: define dso_local noundef i32 @m(
+// CHECK-SAME: i32 noundef [[X:%.*]], i32 noundef [[Y:%.*]]) local_unnamed_addr #[[ATTR0]] {
+// CHECK-NEXT:  [[ENTRY:.*:]]
+// CHECK-NEXT:    [[TMP0:%.*]] = tail call { i32, i1 } @llvm.sadd.with.overflow.i32(i32 [[X]], i32 125), !nosanitize [[META5]]
+// CHECK-NEXT:    [[TMP1:%.*]] = extractvalue { i32, i1 } [[TMP0]], 1, !nosanitize [[META5]]
+// CHECK-NEXT:    br i1 [[TMP1]], label %[[TRAP_I:.*]], label %[[F_EXIT:.*]], !nosanitize [[META5]]
+// CHECK:       [[TRAP_I]]:
+// CHECK-NEXT:    tail call void @llvm.ubsantrap(i8 0) #[[ATTR4]], !nosanitize [[META5]]
+// CHECK-NEXT:    unreachable, !nosanitize [[META5]]
+// CHECK:       [[F_EXIT]]:
+// CHECK-NEXT:    [[TMP2:%.*]] = tail call { i32, i1 } @llvm.sadd.with.overflow.i32(i32 [[Y]], i32 127), !nosanitize [[META5]]
+// CHECK-NEXT:    [[TMP3:%.*]] = extractvalue { i32, i1 } [[TMP2]], 1, !nosanitize [[META5]]
+// CHECK-NEXT:    br i1 [[TMP3]], label %[[TRAP_I2:.*]], label %[[G_EXIT:.*]], !nosanitize [[META5]]
+// CHECK:       [[TRAP_I2]]:
+// CHECK-NEXT:    tail call void @llvm.ubsantrap(i8 0) #[[ATTR4]], !nosanitize [[META5]]
+// CHECK-NEXT:    unreachable, !nosanitize [[META5]]
+// CHECK:       [[G_EXIT]]:
+// CHECK-NEXT:    [[TMP4:%.*]] = extractvalue { i32, i1 } [[TMP0]], 0, !nosanitize [[META5]]
+// CHECK-NEXT:    [[TMP5:%.*]] = extractvalue { i32, i1 } [[TMP2]], 0, !nosanitize [[META5]]
+// CHECK-NEXT:    [[TMP6:%.*]] = tail call { i32, i1 } @llvm.sadd.with.overflow.i32(i32 [[TMP4]], i32 [[TMP5]]), !nosanitize [[META5]]
+// CHECK-NEXT:    [[TMP7:%.*]] = extractvalue { i32, i1 } [[TMP6]], 1, !nosanitize [[META5]]
+// CHECK-NEXT:    br i1 [[TMP7]], label %[[TRAP:.*]], label %[[CONT:.*]], !nosanitize [[META5]]
+// CHECK:       [[TRAP]]:
+// CHECK-NEXT:    tail call void @llvm.ubsantrap(i8 0) #[[ATTR4]], !nosanitize [[META5]]
+// CHECK-NEXT:    unreachable, !nosanitize [[META5]]
+// CHECK:       [[CONT]]:
+// CHECK-NEXT:    [[TMP8:%.*]] = extractvalue { i32, i1 } [[TMP6]], 0, !nosanitize [[META5]]
+// CHECK-NEXT:    ret i32 [[TMP8]]
+//
+int m(int x, int y) {
+  return f(x) + g(y);
+}
+//.
+// CHECK: attributes #[[ATTR4]] = { nomerge noreturn nounwind }

llvmbot · 2024-11-26T00:39:55Z

@llvm/pr-subscribers-clang-codegen

Author: Thurston Dang (thurstond)

Changes

#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 the test that was precommitted in #117649.

Full diff: https://github.com/llvm/llvm-project/pull/117651.diff

3 Files Affected:

(modified) clang/lib/CodeGen/CGExpr.cpp (+2-4)
(modified) clang/test/CodeGen/bounds-checking.c (+2-2)
(added) clang/test/CodeGen/ubsan-trap-merge.c (+106)

diff --git a/clang/lib/CodeGen/CGExpr.cpp b/clang/lib/CodeGen/CGExpr.cpp
index d3f470d401b3d4..f8c1e1cd7a4d68 100644
--- a/clang/lib/CodeGen/CGExpr.cpp
+++ b/clang/lib/CodeGen/CGExpr.cpp
@@ -3921,16 +3921,14 @@ void CodeGenFunction::EmitTrapCheck(llvm::Value *Checked,
 
     llvm::CallInst *TrapCall = Builder.CreateCall(
         CGM.getIntrinsic(llvm::Intrinsic::ubsantrap),
-        llvm::ConstantInt::get(CGM.Int8Ty,
-                               ClSanitizeDebugDeoptimization
-                                   ? TrapBB->getParent()->size()
-                                   : static_cast<uint64_t>(CheckHandlerID)));
+        llvm::ConstantInt::get(CGM.Int8Ty, CheckHandlerID));
 
     if (!CGM.getCodeGenOpts().TrapFuncName.empty()) {
       auto A = llvm::Attribute::get(getLLVMContext(), "trap-func-name",
                                     CGM.getCodeGenOpts().TrapFuncName);
       TrapCall->addFnAttr(A);
     }
+    TrapCall->addFnAttr(llvm::Attribute::NoMerge);
     TrapCall->setDoesNotReturn();
     TrapCall->setDoesNotThrow();
     Builder.CreateUnreachable();
diff --git a/clang/test/CodeGen/bounds-checking.c b/clang/test/CodeGen/bounds-checking.c
index 8100e30d0650ad..f6c4880e70a150 100644
--- a/clang/test/CodeGen/bounds-checking.c
+++ b/clang/test/CodeGen/bounds-checking.c
@@ -74,11 +74,11 @@ char B2[10];
 // CHECK-LABEL: @f8
 void f8(int i, int k) {
   // NOOPTLOCAL: call void @llvm.ubsantrap(i8 3)
-  // NOOPTARRAY: call void @llvm.ubsantrap(i8 2)
+  // NOOPTARRAY: call void @llvm.ubsantrap(i8 18)
   B[i] = '\0';
 
   // NOOPTLOCAL: call void @llvm.ubsantrap(i8 5)
-  // NOOPTARRAY: call void @llvm.ubsantrap(i8 4)
+  // NOOPTARRAY: call void @llvm.ubsantrap(i8 18)
   B2[k] = '\0';
 }
 
diff --git a/clang/test/CodeGen/ubsan-trap-merge.c b/clang/test/CodeGen/ubsan-trap-merge.c
new file mode 100644
index 00000000000000..e6aa7902262813
--- /dev/null
+++ b/clang/test/CodeGen/ubsan-trap-merge.c
@@ -0,0 +1,106 @@
+// NOTE: Assertions have mostly been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --version 5
+// The most important assertion is the attributes at the end of the file, which
+// shows that ubsan attaches 'nomerge' to each ubsantrap intrinsic.
+//
+// RUN: %clang -fsanitize=signed-integer-overflow -S -emit-llvm -fsanitize-trap=all -O3 -mllvm -ubsan-unique-traps %s -o - \
+// RUN:     | FileCheck %s
+
+#include <stdio.h>
+#include <stdlib.h>
+
+// CHECK-LABEL: define dso_local range(i32 -2147483523, -2147483648) i32 @f(
+// CHECK-SAME: i32 noundef [[X:%.*]]) local_unnamed_addr #[[ATTR0:[0-9]+]] {
+// CHECK-NEXT:  [[ENTRY:.*:]]
+// CHECK-NEXT:    [[TMP0:%.*]] = tail call { i32, i1 } @llvm.sadd.with.overflow.i32(i32 [[X]], i32 125), !nosanitize [[META5:![0-9]+]]
+// CHECK-NEXT:    [[TMP1:%.*]] = extractvalue { i32, i1 } [[TMP0]], 1, !nosanitize [[META5]]
+// CHECK-NEXT:    br i1 [[TMP1]], label %[[TRAP:.*]], label %[[CONT:.*]], !nosanitize [[META5]]
+// CHECK:       [[TRAP]]:
+// CHECK-NEXT:    tail call void @llvm.ubsantrap(i8 0) #[[ATTR4:[0-9]+]], !nosanitize [[META5]]
+// CHECK-NEXT:    unreachable, !nosanitize [[META5]]
+// CHECK:       [[CONT]]:
+// CHECK-NEXT:    [[TMP2:%.*]] = extractvalue { i32, i1 } [[TMP0]], 0, !nosanitize [[META5]]
+// CHECK-NEXT:    ret i32 [[TMP2]]
+//
+int f(int x) {
+  return x + 125;
+}
+
+// CHECK-LABEL: define dso_local range(i32 -2147483521, -2147483648) i32 @g(
+// CHECK-SAME: i32 noundef [[X:%.*]]) local_unnamed_addr #[[ATTR0]] {
+// CHECK-NEXT:  [[ENTRY:.*:]]
+// CHECK-NEXT:    [[TMP0:%.*]] = tail call { i32, i1 } @llvm.sadd.with.overflow.i32(i32 [[X]], i32 127), !nosanitize [[META5]]
+// CHECK-NEXT:    [[TMP1:%.*]] = extractvalue { i32, i1 } [[TMP0]], 1, !nosanitize [[META5]]
+// CHECK-NEXT:    br i1 [[TMP1]], label %[[TRAP:.*]], label %[[CONT:.*]], !nosanitize [[META5]]
+// CHECK:       [[TRAP]]:
+// CHECK-NEXT:    tail call void @llvm.ubsantrap(i8 0) #[[ATTR4]], !nosanitize [[META5]]
+// CHECK-NEXT:    unreachable, !nosanitize [[META5]]
+// CHECK:       [[CONT]]:
+// CHECK-NEXT:    [[TMP2:%.*]] = extractvalue { i32, i1 } [[TMP0]], 0, !nosanitize [[META5]]
+// CHECK-NEXT:    ret i32 [[TMP2]]
+//
+int g(int x) {
+  return x + 127;
+}
+
+// CHECK-LABEL: define dso_local range(i32 -2147483521, -2147483648) i32 @h(
+// CHECK-SAME: i32 noundef [[X:%.*]], i32 noundef [[Y:%.*]]) local_unnamed_addr #[[ATTR0]] {
+// CHECK-NEXT:  [[ENTRY:.*:]]
+// CHECK-NEXT:    [[TMP0:%.*]] = tail call { i32, i1 } @llvm.sadd.with.overflow.i32(i32 [[X]], i32 127), !nosanitize [[META5]]
+// CHECK-NEXT:    [[TMP1:%.*]] = extractvalue { i32, i1 } [[TMP0]], 1, !nosanitize [[META5]]
+// CHECK-NEXT:    br i1 [[TMP1]], label %[[TRAP:.*]], label %[[CONT:.*]], !nosanitize [[META5]]
+// CHECK:       [[TRAP]]:
+// CHECK-NEXT:    tail call void @llvm.ubsantrap(i8 0) #[[ATTR4]], !nosanitize [[META5]]
+// CHECK-NEXT:    unreachable, !nosanitize [[META5]]
+// CHECK:       [[CONT]]:
+// CHECK-NEXT:    [[TMP2:%.*]] = tail call { i32, i1 } @llvm.sadd.with.overflow.i32(i32 [[Y]], i32 129), !nosanitize [[META5]]
+// CHECK-NEXT:    [[TMP3:%.*]] = extractvalue { i32, i1 } [[TMP2]], 1, !nosanitize [[META5]]
+// CHECK-NEXT:    br i1 [[TMP3]], label %[[TRAP1:.*]], label %[[CONT2:.*]], !nosanitize [[META5]]
+// CHECK:       [[TRAP1]]:
+// CHECK-NEXT:    tail call void @llvm.ubsantrap(i8 0) #[[ATTR4]], !nosanitize [[META5]]
+// CHECK-NEXT:    unreachable, !nosanitize [[META5]]
+// CHECK:       [[CONT2]]:
+// CHECK-NEXT:    [[TMP4:%.*]] = extractvalue { i32, i1 } [[TMP2]], 0, !nosanitize [[META5]]
+// CHECK-NEXT:    [[TMP5:%.*]] = extractvalue { i32, i1 } [[TMP0]], 0, !nosanitize [[META5]]
+// CHECK-NEXT:    [[COND:%.*]] = tail call i32 @llvm.smin.i32(i32 [[TMP5]], i32 [[TMP4]])
+// CHECK-NEXT:    ret i32 [[COND]]
+//
+int h(int x, int y) {
+  x += 127;
+  y += 129;
+  return x < y ? x : y;
+}
+
+// CHECK-LABEL: define dso_local noundef i32 @m(
+// CHECK-SAME: i32 noundef [[X:%.*]], i32 noundef [[Y:%.*]]) local_unnamed_addr #[[ATTR0]] {
+// CHECK-NEXT:  [[ENTRY:.*:]]
+// CHECK-NEXT:    [[TMP0:%.*]] = tail call { i32, i1 } @llvm.sadd.with.overflow.i32(i32 [[X]], i32 125), !nosanitize [[META5]]
+// CHECK-NEXT:    [[TMP1:%.*]] = extractvalue { i32, i1 } [[TMP0]], 1, !nosanitize [[META5]]
+// CHECK-NEXT:    br i1 [[TMP1]], label %[[TRAP_I:.*]], label %[[F_EXIT:.*]], !nosanitize [[META5]]
+// CHECK:       [[TRAP_I]]:
+// CHECK-NEXT:    tail call void @llvm.ubsantrap(i8 0) #[[ATTR4]], !nosanitize [[META5]]
+// CHECK-NEXT:    unreachable, !nosanitize [[META5]]
+// CHECK:       [[F_EXIT]]:
+// CHECK-NEXT:    [[TMP2:%.*]] = tail call { i32, i1 } @llvm.sadd.with.overflow.i32(i32 [[Y]], i32 127), !nosanitize [[META5]]
+// CHECK-NEXT:    [[TMP3:%.*]] = extractvalue { i32, i1 } [[TMP2]], 1, !nosanitize [[META5]]
+// CHECK-NEXT:    br i1 [[TMP3]], label %[[TRAP_I2:.*]], label %[[G_EXIT:.*]], !nosanitize [[META5]]
+// CHECK:       [[TRAP_I2]]:
+// CHECK-NEXT:    tail call void @llvm.ubsantrap(i8 0) #[[ATTR4]], !nosanitize [[META5]]
+// CHECK-NEXT:    unreachable, !nosanitize [[META5]]
+// CHECK:       [[G_EXIT]]:
+// CHECK-NEXT:    [[TMP4:%.*]] = extractvalue { i32, i1 } [[TMP0]], 0, !nosanitize [[META5]]
+// CHECK-NEXT:    [[TMP5:%.*]] = extractvalue { i32, i1 } [[TMP2]], 0, !nosanitize [[META5]]
+// CHECK-NEXT:    [[TMP6:%.*]] = tail call { i32, i1 } @llvm.sadd.with.overflow.i32(i32 [[TMP4]], i32 [[TMP5]]), !nosanitize [[META5]]
+// CHECK-NEXT:    [[TMP7:%.*]] = extractvalue { i32, i1 } [[TMP6]], 1, !nosanitize [[META5]]
+// CHECK-NEXT:    br i1 [[TMP7]], label %[[TRAP:.*]], label %[[CONT:.*]], !nosanitize [[META5]]
+// CHECK:       [[TRAP]]:
+// CHECK-NEXT:    tail call void @llvm.ubsantrap(i8 0) #[[ATTR4]], !nosanitize [[META5]]
+// CHECK-NEXT:    unreachable, !nosanitize [[META5]]
+// CHECK:       [[CONT]]:
+// CHECK-NEXT:    [[TMP8:%.*]] = extractvalue { i32, i1 } [[TMP6]], 0, !nosanitize [[META5]]
+// CHECK-NEXT:    ret i32 [[TMP8]]
+//
+int m(int x, int y) {
+  return f(x) + g(y);
+}
+//.
+// CHECK: attributes #[[ATTR4]] = { nomerge noreturn nounwind }

github-actions · 2024-11-26T00:43:26Z

✅ With the latest revision this PR passed the C/C++ code formatter.

MaskRay · 2024-11-26T05:45:29Z

llvm/test/MC/X86/ubsan-trap-nomerge.ll

@@ -0,0 +1,185 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 5


Tests with IR input should live in test/CodeGen instead of test/MC

Thanks, moved!

MaskRay · 2024-11-26T05:51:36Z

clang/test/CodeGen/ubsan-trap-merge.c

+// The most important assertion is the attributes at the end of the file, which
+// shows that ubsan attaches 'nomerge' to each ubsantrap intrinsic.
+//
+// RUN: %clang -fsanitize=signed-integer-overflow -S -emit-llvm -fsanitize-trap=all -O3 -mllvm -ubsan-unique-traps %s -o - \


should use clang_cc1 and an explicit triple

Done! I'd overlooked the clang driver distinction - thanks for your electrifying comment!

clang/lib/CodeGen/CGExpr.cpp

…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.

…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.

…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.

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.

backend.

…ttps://github.com/llvm/llvm-project/pull/117657/files

Vitaly's feedback

This reverts commit aafe571.

…e, per" This reverts commit 5107ce1. Doesn't work because CodeGen happens before inlining, so the ubsantraps in functions f(), g() and m() will not have nomerge.

oskarwirga · 2024-12-04T21:06:40Z

Sorry for the lack of review, I was sick but I just wanted to express my gratitude for fixing my hacky approach :)

… applicable llvm#65972 introduced -ubsan-unique-traps and -bounds-checking-unique-traps, which attach the function size to the ubsantrap intrinsic. llvm#117651 changed ubsan-unique-traps to use nomerge instead of the function size, but did not update -bounds-checking-unique-traps. This patch adds nomerge to bounds-checking-unique-traps.

… applicable (#120620) #65972 introduced -ubsan-unique-traps and -bounds-checking-unique-traps, which attach the function size to the ubsantrap intrinsic. #117651 changed ubsan-unique-traps to use nomerge instead of the function size, but did not update -bounds-checking-unique-traps. This patch adds nomerge to bounds-checking-unique-traps.

…ibute where applicable (#120620) llvm/llvm-project#65972 introduced -ubsan-unique-traps and -bounds-checking-unique-traps, which attach the function size to the ubsantrap intrinsic. llvm/llvm-project#117651 changed ubsan-unique-traps to use nomerge instead of the function size, but did not update -bounds-checking-unique-traps. This patch adds nomerge to bounds-checking-unique-traps.

thurstond requested review from vitalybuka, oskarwirga and ZequanWu November 26, 2024 00:39

llvmbot added clang Clang issues not falling into any other category clang:codegen labels Nov 26, 2024

llvmbot added backend:X86 mc Machine (object) code labels Nov 26, 2024

MaskRay reviewed Nov 26, 2024

View reviewed changes

vitalybuka reviewed Nov 26, 2024

View reviewed changes

clang/lib/CodeGen/CGExpr.cpp Outdated Show resolved Hide resolved

thurstond mentioned this pull request Nov 26, 2024

[NFC][clang] Add ubsan-trap-merge.c test to show absence of nomerge #117649

Merged

thurstond marked this pull request as draft November 26, 2024 21:12

thurstond marked this pull request as ready for review November 26, 2024 21:47

thurstond requested a review from vitalybuka November 26, 2024 21:47

thurstond force-pushed the ubsan_trap_nomerge branch from 4c72167 to 7fec45e Compare November 26, 2024 22:40

thurstond mentioned this pull request Nov 26, 2024

Reapply "[NFC][clang] Add ubsan-trap-merge.c test to show absence of nomerge" (#117804) #117805

Merged

vitalybuka approved these changes Nov 26, 2024

View reviewed changes

vitalybuka requested a review from MaskRay November 26, 2024 22:58

thurstond added 6 commits November 26, 2024 23:00

Add test to show that ubsantrap with nomerge is correctly lowered by

8e8d8fd

backend.

clang-format

0536132

Separate ubsan-trap-merge.ll and ubsan-trap-nomerge.ll test cases per h…

e1e7db6

…ttps://github.com/llvm/llvm-project/pull/117657/files

Use clang_cc1 and explicit triple.

bcf1ba2

Move .ll tests to CodeGen directory

363ca32

thurstond added 9 commits November 26, 2024 23:00

Update catch-undef-behavior.c

178b0a9

Remove duplicate header comment

8c7eb2b

Don't apply nomerge for the first instance of each TrapBB type, per

09252c7

Vitaly's feedback

clang-format

ead3fea

Revert "clang-format"

9650683

This reverts commit aafe571.

Revert "Don't apply nomerge for the first instance of each TrapBB typ…

e752957

…e, per" This reverts commit 5107ce1. Doesn't work because CodeGen happens before inlining, so the ubsantraps in functions f(), g() and m() will not have nomerge.

Add comment why we unconditionally add nomerge

2389667

Use NoMerge condition per Vitaly's feedback

0e8a414

Replace repo name

7812ff5

thurstond force-pushed the ubsan_trap_nomerge branch from ee1c299 to 7812ff5 Compare November 26, 2024 23:08

Clarify test comment

8d1de2b

MaskRay approved these changes Nov 27, 2024

View reviewed changes

thurstond merged commit 0d15d46 into llvm:main Nov 27, 2024
8 checks passed

thurstond mentioned this pull request Dec 19, 2024

[BoundsSan] Update BoundsChecking.cpp to use no-merge attribute where applicable #120620

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ubsan] Change ubsan-unique-traps to use nomerge instead of counter #117651

[ubsan] Change ubsan-unique-traps to use nomerge instead of counter #117651

thurstond commented Nov 26, 2024 •

edited

Loading

llvmbot commented Nov 26, 2024 •

edited

Loading

llvmbot commented Nov 26, 2024

github-actions bot commented Nov 26, 2024 •

edited

Loading

MaskRay Nov 26, 2024

thurstond Nov 26, 2024

MaskRay Nov 26, 2024

thurstond Nov 26, 2024

oskarwirga commented Dec 4, 2024 •

edited

Loading

		@@ -0,0 +1,185 @@
		; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 5

[ubsan] Change ubsan-unique-traps to use nomerge instead of counter #117651

[ubsan] Change ubsan-unique-traps to use nomerge instead of counter #117651

Conversation

thurstond commented Nov 26, 2024 • edited Loading

llvmbot commented Nov 26, 2024 • edited Loading

llvmbot commented Nov 26, 2024

github-actions bot commented Nov 26, 2024 • edited Loading

MaskRay Nov 26, 2024

Choose a reason for hiding this comment

thurstond Nov 26, 2024

Choose a reason for hiding this comment

MaskRay Nov 26, 2024

Choose a reason for hiding this comment

thurstond Nov 26, 2024

Choose a reason for hiding this comment

oskarwirga commented Dec 4, 2024 • edited Loading

thurstond commented Nov 26, 2024 •

edited

Loading

llvmbot commented Nov 26, 2024 •

edited

Loading

github-actions bot commented Nov 26, 2024 •

edited

Loading

oskarwirga commented Dec 4, 2024 •

edited

Loading