[clang] [UBSAN] Preserve ubsan code with ubsan-unique-traps (PR #83470)
https://github.com/vitalybuka closed https://github.com/llvm/llvm-project/pull/83470 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [UBSAN] Preserve ubsan code with ubsan-unique-traps (PR #83470)
vitalybuka wrote: abandoning https://github.com/llvm/llvm-project/pull/83470 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [UBSAN] Preserve ubsan code with ubsan-unique-traps (PR #83470)
oskarwirga wrote: > 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 :) https://github.com/llvm/llvm-project/pull/83470 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [UBSAN] Preserve ubsan code with ubsan-unique-traps (PR #83470)
vitalybuka wrote: I changed my design, so I don't need that change as is. Given https://godbolt.org/z/4KfEKq7zb, I can revert your patch, or just leave it as is. I have no preference. https://github.com/llvm/llvm-project/pull/83470 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [UBSAN] Preserve ubsan code with ubsan-unique-traps (PR #83470)
vitalybuka wrote: > 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 https://github.com/llvm/llvm-project/pull/83471 However I am considering introducing a special intrinsic for that, the this patch will not be needed. https://github.com/llvm/llvm-project/pull/83470 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [UBSAN] Preserve ubsan code with ubsan-unique-traps (PR #83470)
https://github.com/oskarwirga requested changes to this pull request. 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! :) https://github.com/llvm/llvm-project/pull/83470 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [UBSAN] Preserve ubsan code with ubsan-unique-traps (PR #83470)
vitalybuka wrote: > > It happens later, in LLVM backend, it needs to be fixed. > > From [#65972 > (comment)](https://github.com/llvm/llvm-project/pull/65972#issuecomment-1971855638) > > 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. https://github.com/llvm/llvm-project/pull/83470 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [UBSAN] Preserve ubsan code with ubsan-unique-traps (PR #83470)
oskarwirga wrote: > It happens later, in LLVM backend, it needs to be fixed. >From https://github.com/llvm/llvm-project/pull/65972#issuecomment-1971855638 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. https://github.com/llvm/llvm-project/pull/83470 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [UBSAN] Preserve ubsan code with ubsan-unique-traps (PR #83470)
llvmbot wrote:
@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
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';
}
``
https://github.com/llvm/llvm-project/pull/83470
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [UBSAN] Preserve ubsan code with ubsan-unique-traps (PR #83470)
https://github.com/vitalybuka created
https://github.com/llvm/llvm-project/pull/83470
Removing `TrapBB->getParent()->size()` added with #65972. Counter as-is
is not very unique after inlining https://godbolt.org/z/4KfEKq7zb (see
m()).
>From e44df1c386d96472614939658e496cf2a9643e05 Mon Sep 17 00:00:00 2001
From: Vitaly Buka
Date: Tue, 27 Feb 2024 11:13:11 -0800
Subject: [PATCH] [UBSAN] Preserve ubsan code with ubsan-unique-traps
Removing `TrapBB->getParent()->size()` added with #65972. Counter as-is
is not very unique after inlining https://godbolt.org/z/4KfEKq7zb (see
m()).
---
clang/lib/CodeGen/CGExpr.cpp | 8 +++-
clang/test/CodeGen/bounds-checking.c | 4 ++--
2 files changed, 5 insertions(+), 7 deletions(-)
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';
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
