Skip to content

[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

Merged
merged 6 commits into from
Dec 10, 2024

Conversation

thurstond
Copy link
Contributor

@thurstond thurstond commented Dec 10, 2024

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.

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.
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:codegen IR generation bugs: mangling, exceptions, etc. labels Dec 10, 2024
@llvmbot
Copy link
Member

llvmbot commented Dec 10, 2024

@llvm/pr-subscribers-clang-codegen

Author: Thurston Dang (thurstond)

Changes

UBSan 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:

  • (modified) clang/lib/CodeGen/CGExpr.cpp (+6)
  • (modified) clang/test/CodeGen/ubsan-trap-merge.c (+1-1)
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 }

@llvmbot
Copy link
Member

llvmbot commented Dec 10, 2024

@llvm/pr-subscribers-clang

Author: Thurston Dang (thurstond)

Changes

UBSan 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:

  • (modified) clang/lib/CodeGen/CGExpr.cpp (+6)
  • (modified) clang/test/CodeGen/ubsan-trap-merge.c (+1-1)
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 }

Copy link

github-actions bot commented Dec 10, 2024

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

@thurstond thurstond changed the title [ubsan] Allow -ubsan-unique-traps option for ubsan min-rt [ubsan] Allow -ubsan-unique-traps option for non-trap handlers Dec 10, 2024
@thurstond thurstond requested a review from vitalybuka December 10, 2024 02:54
@thurstond thurstond changed the title [ubsan] Allow -ubsan-unique-traps option for non-trap handlers [ubsan] Don't merge non-trap handlers if -ubsan-unique-traps or not optimized Dec 10, 2024
@thurstond thurstond merged commit 67bd04f into llvm:main Dec 10, 2024
6 of 8 checks passed
thurstond added a commit to thurstond/llvm-project that referenced this pull request Dec 18, 2024
…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.
thurstond added a commit that referenced this pull request Dec 18, 2024
'-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.
thurstond added a commit to thurstond/llvm-project that referenced this pull request Dec 19, 2024
…#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.
thurstond added a commit that referenced this pull request Dec 19, 2024
…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.
github-actions bot pushed a commit to arm/arm-toolchain that referenced this pull request Jan 10, 2025
…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.
github-actions bot pushed a commit to arm/arm-toolchain that referenced this pull request Jan 10, 2025
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:codegen IR generation bugs: mangling, exceptions, etc. clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants