[UBSAN] Preserve ubsan code with ubsan-unique-traps

[vitalybuka]

Removing TrapBB->getParent()->size() added with #65972. Counter as-is
is not very unique after inlining https://godbolt.org/z/4KfEKq7zb (see
m()).

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-codegen

Author: Vitaly Buka (vitalybuka)

Changes

Removing TrapBB->getParent()->size() added with #65972. Counter as-is
is not very unique after inlining https://godbolt.org/z/4KfEKq7zb (see
m()).

---

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

2 Files Affected:

• (modified) clang/lib/CodeGen/CGExpr.cpp (+3-5)
• (modified) clang/test/CodeGen/bounds-checking.c (+2-2)

diff --git a/clang/lib/CodeGen/CGExpr.cpp b/clang/lib/CodeGen/CGExpr.cpp
index 59a7fe8925001c..ee5f3a2786a627 100644
--- a/clang/lib/CodeGen/CGExpr.cpp
+++ b/clang/lib/CodeGen/CGExpr.cpp
@@ -3826,11 +3826,9 @@ void CodeGenFunction::EmitTrapCheck(llvm::Value *Checked,
     Builder.CreateCondBr(Checked, Cont, TrapBB);
     EmitBlock(TrapBB);
 
-    llvm::CallInst *TrapCall = Builder.CreateCall(
-        CGM.getIntrinsic(llvm::Intrinsic::ubsantrap),
-        llvm::ConstantInt::get(CGM.Int8Ty, ClSanitizeDebugDeoptimization
-                                               ? TrapBB->getParent()->size()
-                                               : CheckHandlerID));
+    llvm::CallInst *TrapCall =
+        Builder.CreateCall(CGM.getIntrinsic(llvm::Intrinsic::ubsantrap),
+                           llvm::ConstantInt::get(CGM.Int8Ty, CheckHandlerID));
 
     if (!CGM.getCodeGenOpts().TrapFuncName.empty()) {
       auto A = llvm::Attribute::get(getLLVMContext(), "trap-func-name",
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';
 }
 

[oskarwirga]

It happens later, in LLVM backend, it needs to be fixed.

From #65972 (comment)

Is this something you have planned to fix? If not would replacing the .size() counter with perhaps a seeded random uint8 be acceptable?

My use case prevents me from shipping the minimal runtime alongside the instrumentation so my goal was to create an improvement (definitely imperfect!) to the debugability of a production deployment of BoundsSan. This PR as is would revert that behavior entirely.

[vitalybuka]

It happens later, in LLVM backend, it needs to be fixed.

From #65972 (comment)

Is this something you have planned to fix? If not would replacing the .size() counter with perhaps a seeded random uint8 be acceptable?

My use case prevents me from shipping the minimal runtime alongside the instrumentation so my goal was to create an improvement (definitely imperfect!) to the debugability of a production deployment of BoundsSan. This PR as is would revert that behavior entirely.

I don't plan to do anything about it.
My point that it does not work even on trivial example as in description.
Unless you/someone else is willing to work on real fix, this behavior is not worse of preserving.

[oskarwirga]

If you are going to remove this feature, I would rather you simply revert the old commit. There is no point leaving the flag in at this point.

I had explored earlier dealing with the optimization at a later time in the compilation pipeline, but got nowhere and this solution was ideal for my use case, binary size constraints limited any inlining so I never ran into the issue. I appreciate you cleaning this up! :)

[vitalybuka]

If you are going to remove this feature, I would rather you simply revert the old commit. There is no point leaving the flag in at this point.

I had explored earlier dealing with the optimization at a later time in the compilation pipeline, but got nowhere and this solution was ideal for my use case, binary size constraints limited any inlining so I never ran into the issue. I appreciate you cleaning this up! :)

Actually I am here because I want to use the flag to avoid merging basic blocks.
We are going to opt-out ubsan checks based on PGO, per basic block hotness #83471

However I am considering introducing a special intrinsic for that, the this patch will not be needed.

[vitalybuka]

I changed my design, so I don't need this patch.
Given https://godbolt.org/z/4KfEKq7zb, I can revert your patch, or just leave it as is. I have no preference.

[oskarwirga]

I changed my design, so I don't need this patch. Given https://godbolt.org/z/4KfEKq7zb, I can revert your patch, or just leave it as is. I have no preference.

I would prefer leaving it as is, I will make a note to revisit this pending further testing on my end to see how useful it really is. Thank you :)

[vitalybuka]

abandoning