-
Notifications
You must be signed in to change notification settings - Fork 13.5k
[ubsan] Don't merge non-trap handlers if -ubsan-unique-traps or not optimized #119302
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
UBSan handlers can sometimes be merged by the backend, which complicates debugging when a backtrace is not available (traps or min-rt handlers). -ubsan-unique-traps prevents merging for trap mode, but not for min-rt's (non-trap) handlers. This patch extends -ubsan-unique-traps to work for min-rt handlers. It does not change the behavior when using the regular ubsan runtime; the backtrace provides enough information for debugging, hence no-merge would simply increase code size with little benefit.
@llvm/pr-subscribers-clang-codegen Author: Thurston Dang (thurstond) ChangesUBSan handler calls can sometimes be merged by the backend, which complicates debugging when a backtrace is not available (trap or min-rt). -ubsan-unique-traps prevents merging for trap mode, but not for min-rt's handler calls. This patch extends -ubsan-unique-traps to work for min-rt handlers. It does not change the behavior when using the regular ubsan runtime; the backtrace provides enough information for debugging, hence no-merge would simply increase code size with little benefit. N.B. "-ubsan-unique-traps" becomes somewhat of a misnomer since it will now apply to min-rt handlers as well as traps. We keep the naming for backwards compatibility. Full diff: https://github.com/llvm/llvm-project/pull/119302.diff 2 Files Affected:
diff --git a/clang/lib/CodeGen/CGExpr.cpp b/clang/lib/CodeGen/CGExpr.cpp
index 0d16408aa4de9d..9a3f573fec7725 100644
--- a/clang/lib/CodeGen/CGExpr.cpp
+++ b/clang/lib/CodeGen/CGExpr.cpp
@@ -3581,6 +3581,12 @@ static void emitCheckHandlerCall(CodeGenFunction &CGF,
llvm::AttributeList::FunctionIndex, B),
/*Local=*/true);
llvm::CallInst *HandlerCall = CGF.EmitNounwindRuntimeCall(Fn, FnArgs);
+ bool NoMerge = ClSanitizeDebugDeoptimization ||
+ !CGF.CGM.getCodeGenOpts().OptimizationLevel ||
+ (CGF.CurCodeDecl && CGF.CurCodeDecl->hasAttr<OptimizeNoneAttr>());
+ // Regular runtime provides a backtrace, making NoMerge a waste of space
+ if (NoMerge && MinimalRuntime)
+ HandlerCall->addFnAttr(llvm::Attribute::NoMerge);
if (!MayReturn) {
HandlerCall->setDoesNotReturn();
CGF.Builder.CreateUnreachable();
diff --git a/clang/test/CodeGen/ubsan-trap-merge.c b/clang/test/CodeGen/ubsan-trap-merge.c
index e48681ce09377d..5c0760a539ec6a 100644
--- a/clang/test/CodeGen/ubsan-trap-merge.c
+++ b/clang/test/CodeGen/ubsan-trap-merge.c
@@ -266,4 +266,4 @@ int m(int x, int y) {
}
// TRAP: attributes #[[ATTR4]] = { nomerge noreturn nounwind }
// HANDLER: attributes #[[ATTR4]] = { noreturn nounwind }
-// MINRT: attributes #[[ATTR4]] = { noreturn nounwind }
+// MINRT: attributes #[[ATTR4]] = { nomerge noreturn nounwind }
|
@llvm/pr-subscribers-clang Author: Thurston Dang (thurstond) ChangesUBSan handler calls can sometimes be merged by the backend, which complicates debugging when a backtrace is not available (trap or min-rt). -ubsan-unique-traps prevents merging for trap mode, but not for min-rt's handler calls. This patch extends -ubsan-unique-traps to work for min-rt handlers. It does not change the behavior when using the regular ubsan runtime; the backtrace provides enough information for debugging, hence no-merge would simply increase code size with little benefit. N.B. "-ubsan-unique-traps" becomes somewhat of a misnomer since it will now apply to min-rt handlers as well as traps. We keep the naming for backwards compatibility. Full diff: https://github.com/llvm/llvm-project/pull/119302.diff 2 Files Affected:
diff --git a/clang/lib/CodeGen/CGExpr.cpp b/clang/lib/CodeGen/CGExpr.cpp
index 0d16408aa4de9d..9a3f573fec7725 100644
--- a/clang/lib/CodeGen/CGExpr.cpp
+++ b/clang/lib/CodeGen/CGExpr.cpp
@@ -3581,6 +3581,12 @@ static void emitCheckHandlerCall(CodeGenFunction &CGF,
llvm::AttributeList::FunctionIndex, B),
/*Local=*/true);
llvm::CallInst *HandlerCall = CGF.EmitNounwindRuntimeCall(Fn, FnArgs);
+ bool NoMerge = ClSanitizeDebugDeoptimization ||
+ !CGF.CGM.getCodeGenOpts().OptimizationLevel ||
+ (CGF.CurCodeDecl && CGF.CurCodeDecl->hasAttr<OptimizeNoneAttr>());
+ // Regular runtime provides a backtrace, making NoMerge a waste of space
+ if (NoMerge && MinimalRuntime)
+ HandlerCall->addFnAttr(llvm::Attribute::NoMerge);
if (!MayReturn) {
HandlerCall->setDoesNotReturn();
CGF.Builder.CreateUnreachable();
diff --git a/clang/test/CodeGen/ubsan-trap-merge.c b/clang/test/CodeGen/ubsan-trap-merge.c
index e48681ce09377d..5c0760a539ec6a 100644
--- a/clang/test/CodeGen/ubsan-trap-merge.c
+++ b/clang/test/CodeGen/ubsan-trap-merge.c
@@ -266,4 +266,4 @@ int m(int x, int y) {
}
// TRAP: attributes #[[ATTR4]] = { nomerge noreturn nounwind }
// HANDLER: attributes #[[ATTR4]] = { noreturn nounwind }
-// MINRT: attributes #[[ATTR4]] = { noreturn nounwind }
+// MINRT: attributes #[[ATTR4]] = { nomerge noreturn nounwind }
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
This reverts commit 1167d26.
…d-handlers) '-mllvm -ubsan-unique-traps' (llvm#65972) applies to all UBSan checks. This patch introduces -fsanitize-nonmerged-handlers and -fno-sanitize-nonmerged-handlers, which allows selectively applying non-merged handlers to a subset of UBSan checks. N.B. we use "non-merged handlers" instead of "unique traps", since llvm#119302 has generalized it to work for non-trap mode as well (min-rt and regular rt). This patch does not remove the -ubsan-unique-traps flag; that will override -f(no-)sanitize-non-merged-handlers.
'-mllvm -ubsan-unique-traps' (#65972) applies to all UBSan checks. This patch introduces -fsanitize-merge (defaults to on, maintaining the status quo behavior) and -fno-sanitize-merge (equivalent to '-mllvm -ubsan-unique-traps'), with the option to selectively applying non-merged handlers to a subset of UBSan checks (e.g., -fno-sanitize-merge=bool,enum). N.B. we do not use "trap" in the argument name since #119302 has generalized -ubsan-unique-traps to work for non-trap modes (min-rt and regular rt). This patch does not remove the -ubsan-unique-traps flag; that will override -f(no-)sanitize-merge.
…#120464)" This reverts commit 2691b96. This reapply fixes the buildbot breakage of the original patch, by updating clang/test/CodeGen/ubsan-trap-debugloc.c to specify -fsanitize-merge (the default, which is merge, is applied by the driver but not clang_cc1). This reapply also expands clang/test/CodeGen/ubsan-trap-merge.c. Original commit message: '-mllvm -ubsan-unique-traps' (llvm#65972) applies to all UBSan checks. This patch introduces -fsanitize-merge (defaults to on, maintaining the status quo behavior) and -fno-sanitize-merge (equivalent to '-mllvm -ubsan-unique-traps'), with the option to selectively applying non-merged handlers to a subset of UBSan checks (e.g., -fno-sanitize-merge=bool,enum). N.B. we do not use "trap" in the argument name since llvm#119302 has generalized -ubsan-unique-traps to work for non-trap modes (min-rt and regular rt). This patch does not remove the -ubsan-unique-traps flag; that will override -f(no-)sanitize-merge.
…464)" (#120511) This reverts commit 2691b96. This reapply fixes the buildbot breakage of the original patch, by updating clang/test/CodeGen/ubsan-trap-debugloc.c to specify -fsanitize-merge (the default, which is merge, is applied by the driver but not clang_cc1). This reapply also expands clang/test/CodeGen/ubsan-trap-merge.c. ---- Original commit message: '-mllvm -ubsan-unique-traps' (#65972) applies to all UBSan checks. This patch introduces -fsanitize-merge (defaults to on, maintaining the status quo behavior) and -fno-sanitize-merge (equivalent to '-mllvm -ubsan-unique-traps'), with the option to selectively applying non-merged handlers to a subset of UBSan checks (e.g., -fno-sanitize-merge=bool,enum). N.B. we do not use "trap" in the argument name since #119302 has generalized -ubsan-unique-traps to work for non-trap modes (min-rt and regular rt). This patch does not remove the -ubsan-unique-traps flag; that will override -f(no-)sanitize-merge.
…20464) '-mllvm -ubsan-unique-traps' (llvm/llvm-project#65972) applies to all UBSan checks. This patch introduces -fsanitize-merge (defaults to on, maintaining the status quo behavior) and -fno-sanitize-merge (equivalent to '-mllvm -ubsan-unique-traps'), with the option to selectively applying non-merged handlers to a subset of UBSan checks (e.g., -fno-sanitize-merge=bool,enum). N.B. we do not use "trap" in the argument name since llvm/llvm-project#119302 has generalized -ubsan-unique-traps to work for non-trap modes (min-rt and regular rt). This patch does not remove the -ubsan-unique-traps flag; that will override -f(no-)sanitize-merge.
…erge) (#120…464)" (#120511) This reverts commit 2691b96. This reapply fixes the buildbot breakage of the original patch, by updating clang/test/CodeGen/ubsan-trap-debugloc.c to specify -fsanitize-merge (the default, which is merge, is applied by the driver but not clang_cc1). This reapply also expands clang/test/CodeGen/ubsan-trap-merge.c. ---- Original commit message: '-mllvm -ubsan-unique-traps' (llvm/llvm-project#65972) applies to all UBSan checks. This patch introduces -fsanitize-merge (defaults to on, maintaining the status quo behavior) and -fno-sanitize-merge (equivalent to '-mllvm -ubsan-unique-traps'), with the option to selectively applying non-merged handlers to a subset of UBSan checks (e.g., -fno-sanitize-merge=bool,enum). N.B. we do not use "trap" in the argument name since llvm/llvm-project#119302 has generalized -ubsan-unique-traps to work for non-trap modes (min-rt and regular rt). This patch does not remove the -ubsan-unique-traps flag; that will override -f(no-)sanitize-merge.
UBSan handler calls can sometimes be merged by the backend, which complicates debugging. Merging is currently disabled for UBSan traps if -ubsan-unique-traps is specified or if optimization is disabled. This patch applies the same policy to non-trap handler calls.
N.B. "-ubsan-unique-traps" becomes somewhat of a misnomer since it will now apply to non-trap handler calls as well as traps. We keep the naming for backwards compatibility.