Skip to content

[ubsan] Don't merge non-trap handlers if -ubsan-unique-traps or not optimized#119302

Merged
thurstond merged 6 commits into
llvm:mainfrom
thurstond:ubsan_minrt_nomerge
Dec 10, 2024
Merged

[ubsan] Don't merge non-trap handlers if -ubsan-unique-traps or not optimized#119302
thurstond merged 6 commits into
llvm:mainfrom
thurstond:ubsan_minrt_nomerge

Conversation

@thurstond

@thurstond thurstond commented Dec 10, 2024

Copy link
Copy Markdown
Contributor

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

llvmbot commented Dec 10, 2024

Copy link
Copy Markdown
Member

@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

llvmbot commented Dec 10, 2024

Copy link
Copy Markdown
Member

@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

github-actions Bot commented Dec 10, 2024

Copy link
Copy Markdown

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

Comment thread clang/lib/CodeGen/CGExpr.cpp Outdated
@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
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