Skip to content

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 }

@github-actions
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