Skip to content

Reapply "[ubsan] Add -fsanitize-merge (and -fno-sanitize-merge) (#120…464)" #120511

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 2 commits into from
Dec 19, 2024

Conversation

thurstond
Copy link
Contributor

@thurstond thurstond commented Dec 19, 2024

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.

…#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.
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:codegen IR generation bugs: mangling, exceptions, etc. labels Dec 19, 2024
@llvmbot
Copy link
Member

llvmbot commented Dec 19, 2024

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-driver

Author: Thurston Dang (thurstond)

Changes

…464)"

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.


Patch is 70.66 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/120511.diff

10 Files Affected:

  • (modified) clang/include/clang/Basic/CodeGenOptions.h (+4)
  • (modified) clang/include/clang/Driver/Options.td (+15)
  • (modified) clang/include/clang/Driver/SanitizerArgs.h (+1)
  • (modified) clang/lib/CodeGen/CGExpr.cpp (+17-13)
  • (modified) clang/lib/CodeGen/CodeGenFunction.h (+2-1)
  • (modified) clang/lib/Driver/SanitizerArgs.cpp (+24-7)
  • (modified) clang/lib/Frontend/CompilerInvocation.cpp (+7)
  • (modified) clang/test/CodeGen/ubsan-trap-debugloc.c (+1-1)
  • (modified) clang/test/CodeGen/ubsan-trap-merge.c (+500-29)
  • (modified) clang/test/Driver/fsanitize.c (+53)
diff --git a/clang/include/clang/Basic/CodeGenOptions.h b/clang/include/clang/Basic/CodeGenOptions.h
index 2dcf98b465661e..8097c9ef772bc7 100644
--- a/clang/include/clang/Basic/CodeGenOptions.h
+++ b/clang/include/clang/Basic/CodeGenOptions.h
@@ -380,6 +380,10 @@ class CodeGenOptions : public CodeGenOptionsBase {
   /// Set of sanitizer checks that trap rather than diagnose.
   SanitizerSet SanitizeTrap;
 
+  /// Set of sanitizer checks that can merge handlers (smaller code size at
+  /// the expense of debuggability).
+  SanitizerSet SanitizeMergeHandlers;
+
   /// List of backend command-line options for -fembed-bitcode.
   std::vector<uint8_t> CmdArgs;
 
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index 14e47f083ecec4..638f8c52053ec5 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -2555,6 +2555,21 @@ def fno_sanitize_trap : Flag<["-"], "fno-sanitize-trap">, Group<f_clang_Group>,
                         Alias<fno_sanitize_trap_EQ>, AliasArgs<["all"]>,
                         Visibility<[ClangOption, CLOption]>,
                         HelpText<"Disable trapping for all sanitizers">;
+def fsanitize_merge_handlers_EQ
+    : CommaJoined<["-"], "fsanitize-merge=">,
+      Group<f_clang_Group>,
+      HelpText<"Allow compiler to merge handlers for specified sanitizers">;
+def fno_sanitize_merge_handlers_EQ
+    : CommaJoined<["-"], "fno-sanitize-merge=">,
+      Group<f_clang_Group>,
+      HelpText<"Do not allow compiler to merge handlers for specified sanitizers">;
+def fsanitize_merge_handlers : Flag<["-"], "fsanitize-merge">, Group<f_clang_Group>,
+                     Alias<fsanitize_merge_handlers_EQ>, AliasArgs<["all"]>,
+                     HelpText<"Allow compiler to merge handlers for all sanitizers">;
+def fno_sanitize_merge_handlers : Flag<["-"], "fno-sanitize-merge">, Group<f_clang_Group>,
+                        Alias<fno_sanitize_merge_handlers_EQ>, AliasArgs<["all"]>,
+                        Visibility<[ClangOption, CLOption]>,
+                        HelpText<"Do not allow compiler to merge handlers for any sanitizers">;
 def fsanitize_undefined_trap_on_error
     : Flag<["-"], "fsanitize-undefined-trap-on-error">, Group<f_clang_Group>,
       Alias<fsanitize_trap_EQ>, AliasArgs<["undefined"]>;
diff --git a/clang/include/clang/Driver/SanitizerArgs.h b/clang/include/clang/Driver/SanitizerArgs.h
index 4f08ea2b260179..7410ad4303011c 100644
--- a/clang/include/clang/Driver/SanitizerArgs.h
+++ b/clang/include/clang/Driver/SanitizerArgs.h
@@ -25,6 +25,7 @@ class SanitizerArgs {
   SanitizerSet Sanitizers;
   SanitizerSet RecoverableSanitizers;
   SanitizerSet TrapSanitizers;
+  SanitizerSet MergeHandlers;
 
   std::vector<std::string> UserIgnorelistFiles;
   std::vector<std::string> SystemIgnorelistFiles;
diff --git a/clang/lib/CodeGen/CGExpr.cpp b/clang/lib/CodeGen/CGExpr.cpp
index 79955f55714164..d3fa5be6777ef4 100644
--- a/clang/lib/CodeGen/CGExpr.cpp
+++ b/clang/lib/CodeGen/CGExpr.cpp
@@ -3546,7 +3546,7 @@ static void emitCheckHandlerCall(CodeGenFunction &CGF,
                                  ArrayRef<llvm::Value *> FnArgs,
                                  SanitizerHandler CheckHandler,
                                  CheckRecoverableKind RecoverKind, bool IsFatal,
-                                 llvm::BasicBlock *ContBB) {
+                                 llvm::BasicBlock *ContBB, bool NoMerge) {
   assert(IsFatal || RecoverKind != CheckRecoverableKind::Unrecoverable);
   std::optional<ApplyDebugLocation> DL;
   if (!CGF.Builder.getCurrentDebugLocation()) {
@@ -3581,10 +3581,9 @@ 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>());
+  NoMerge = NoMerge || ClSanitizeDebugDeoptimization ||
+            !CGF.CGM.getCodeGenOpts().OptimizationLevel ||
+            (CGF.CurCodeDecl && CGF.CurCodeDecl->hasAttr<OptimizeNoneAttr>());
   if (NoMerge)
     HandlerCall->addFnAttr(llvm::Attribute::NoMerge);
   if (!MayReturn) {
@@ -3608,6 +3607,7 @@ void CodeGenFunction::EmitCheck(
   llvm::Value *FatalCond = nullptr;
   llvm::Value *RecoverableCond = nullptr;
   llvm::Value *TrapCond = nullptr;
+  bool NoMerge = false;
   for (int i = 0, n = Checked.size(); i < n; ++i) {
     llvm::Value *Check = Checked[i].first;
     // -fsanitize-trap= overrides -fsanitize-recover=.
@@ -3618,6 +3618,9 @@ void CodeGenFunction::EmitCheck(
                   ? RecoverableCond
                   : FatalCond;
     Cond = Cond ? Builder.CreateAnd(Cond, Check) : Check;
+
+    if (!CGM.getCodeGenOpts().SanitizeMergeHandlers.has(Checked[i].second))
+      NoMerge = true;
   }
 
   if (ClSanitizeGuardChecks) {
@@ -3632,7 +3635,7 @@ void CodeGenFunction::EmitCheck(
   }
 
   if (TrapCond)
-    EmitTrapCheck(TrapCond, CheckHandler);
+    EmitTrapCheck(TrapCond, CheckHandler, NoMerge);
   if (!FatalCond && !RecoverableCond)
     return;
 
@@ -3698,7 +3701,7 @@ void CodeGenFunction::EmitCheck(
     // Simple case: we need to generate a single handler call, either
     // fatal, or non-fatal.
     emitCheckHandlerCall(*this, FnType, Args, CheckHandler, RecoverKind,
-                         (FatalCond != nullptr), Cont);
+                         (FatalCond != nullptr), Cont, NoMerge);
   } else {
     // Emit two handler calls: first one for set of unrecoverable checks,
     // another one for recoverable.
@@ -3708,10 +3711,10 @@ void CodeGenFunction::EmitCheck(
     Builder.CreateCondBr(FatalCond, NonFatalHandlerBB, FatalHandlerBB);
     EmitBlock(FatalHandlerBB);
     emitCheckHandlerCall(*this, FnType, Args, CheckHandler, RecoverKind, true,
-                         NonFatalHandlerBB);
+                         NonFatalHandlerBB, NoMerge);
     EmitBlock(NonFatalHandlerBB);
     emitCheckHandlerCall(*this, FnType, Args, CheckHandler, RecoverKind, false,
-                         Cont);
+                         Cont, NoMerge);
   }
 
   EmitBlock(Cont);
@@ -3901,7 +3904,8 @@ void CodeGenFunction::EmitUnreachable(SourceLocation Loc) {
 }
 
 void CodeGenFunction::EmitTrapCheck(llvm::Value *Checked,
-                                    SanitizerHandler CheckHandlerID) {
+                                    SanitizerHandler CheckHandlerID,
+                                    bool NoMerge) {
   llvm::BasicBlock *Cont = createBasicBlock("cont");
 
   // If we're optimizing, collapse all calls to trap down to just one per
@@ -3911,9 +3915,9 @@ void CodeGenFunction::EmitTrapCheck(llvm::Value *Checked,
 
   llvm::BasicBlock *&TrapBB = TrapBBs[CheckHandlerID];
 
-  bool NoMerge = ClSanitizeDebugDeoptimization ||
-                 !CGM.getCodeGenOpts().OptimizationLevel ||
-                 (CurCodeDecl && CurCodeDecl->hasAttr<OptimizeNoneAttr>());
+  NoMerge = NoMerge || ClSanitizeDebugDeoptimization ||
+            !CGM.getCodeGenOpts().OptimizationLevel ||
+            (CurCodeDecl && CurCodeDecl->hasAttr<OptimizeNoneAttr>());
 
   if (TrapBB && !NoMerge) {
     auto Call = TrapBB->begin();
diff --git a/clang/lib/CodeGen/CodeGenFunction.h b/clang/lib/CodeGen/CodeGenFunction.h
index 847999cf1f28a0..4d4139180e100f 100644
--- a/clang/lib/CodeGen/CodeGenFunction.h
+++ b/clang/lib/CodeGen/CodeGenFunction.h
@@ -5171,7 +5171,8 @@ class CodeGenFunction : public CodeGenTypeCache {
 
   /// Create a basic block that will call the trap intrinsic, and emit a
   /// conditional branch to it, for the -ftrapv checks.
-  void EmitTrapCheck(llvm::Value *Checked, SanitizerHandler CheckHandlerID);
+  void EmitTrapCheck(llvm::Value *Checked, SanitizerHandler CheckHandlerID,
+                     bool NoMerge = false);
 
   /// Emit a call to trap or debugtrap and attach function attribute
   /// "trap-func-name" if specified.
diff --git a/clang/lib/Driver/SanitizerArgs.cpp b/clang/lib/Driver/SanitizerArgs.cpp
index 0edfe641416129..727974833e6d1e 100644
--- a/clang/lib/Driver/SanitizerArgs.cpp
+++ b/clang/lib/Driver/SanitizerArgs.cpp
@@ -68,6 +68,7 @@ static const SanitizerMask TrappingSupported =
     SanitizerKind::ImplicitConversion | SanitizerKind::Nullability |
     SanitizerKind::LocalBounds | SanitizerKind::CFI |
     SanitizerKind::FloatDivideByZero | SanitizerKind::ObjCCast;
+static const SanitizerMask MergeDefault = SanitizerKind::Undefined;
 static const SanitizerMask TrappingDefault = SanitizerKind::CFI;
 static const SanitizerMask CFIClasses =
     SanitizerKind::CFIVCall | SanitizerKind::CFINVCall |
@@ -696,6 +697,13 @@ SanitizerArgs::SanitizerArgs(const ToolChain &TC,
   TrappingKinds &= Kinds;
   RecoverableKinds &= ~TrappingKinds;
 
+  // Parse -f(no-)?sanitize-nonmerged-handlers flags
+  SanitizerMask MergeKinds =
+      parseSanitizeArgs(D, Args, DiagnoseErrors, MergeDefault, {}, {},
+                        options::OPT_fsanitize_merge_handlers_EQ,
+                        options::OPT_fno_sanitize_merge_handlers_EQ);
+  MergeKinds &= Kinds;
+
   // Setup ignorelist files.
   // Add default ignorelist from resource directory for activated sanitizers,
   // and validate special case lists format.
@@ -1114,6 +1122,8 @@ SanitizerArgs::SanitizerArgs(const ToolChain &TC,
   TrapSanitizers.Mask |= TrappingKinds;
   assert(!(RecoverableKinds & TrappingKinds) &&
          "Overlap between recoverable and trapping sanitizers");
+
+  MergeHandlers.Mask |= MergeKinds;
 }
 
 static std::string toString(const clang::SanitizerSet &Sanitizers) {
@@ -1275,6 +1285,10 @@ void SanitizerArgs::addArgs(const ToolChain &TC, const llvm::opt::ArgList &Args,
     CmdArgs.push_back(
         Args.MakeArgString("-fsanitize-trap=" + toString(TrapSanitizers)));
 
+  if (!MergeHandlers.empty())
+    CmdArgs.push_back(
+        Args.MakeArgString("-fsanitize-merge=" + toString(MergeHandlers)));
+
   addSpecialCaseListOpt(Args, CmdArgs,
                         "-fsanitize-ignorelist=", UserIgnorelistFiles);
   addSpecialCaseListOpt(Args, CmdArgs,
@@ -1442,13 +1456,16 @@ void SanitizerArgs::addArgs(const ToolChain &TC, const llvm::opt::ArgList &Args,
 
 SanitizerMask parseArgValues(const Driver &D, const llvm::opt::Arg *A,
                              bool DiagnoseErrors) {
-  assert((A->getOption().matches(options::OPT_fsanitize_EQ) ||
-          A->getOption().matches(options::OPT_fno_sanitize_EQ) ||
-          A->getOption().matches(options::OPT_fsanitize_recover_EQ) ||
-          A->getOption().matches(options::OPT_fno_sanitize_recover_EQ) ||
-          A->getOption().matches(options::OPT_fsanitize_trap_EQ) ||
-          A->getOption().matches(options::OPT_fno_sanitize_trap_EQ)) &&
-         "Invalid argument in parseArgValues!");
+  assert(
+      (A->getOption().matches(options::OPT_fsanitize_EQ) ||
+       A->getOption().matches(options::OPT_fno_sanitize_EQ) ||
+       A->getOption().matches(options::OPT_fsanitize_recover_EQ) ||
+       A->getOption().matches(options::OPT_fno_sanitize_recover_EQ) ||
+       A->getOption().matches(options::OPT_fsanitize_trap_EQ) ||
+       A->getOption().matches(options::OPT_fno_sanitize_trap_EQ) ||
+       A->getOption().matches(options::OPT_fsanitize_merge_handlers_EQ) ||
+       A->getOption().matches(options::OPT_fno_sanitize_merge_handlers_EQ)) &&
+      "Invalid argument in parseArgValues!");
   SanitizerMask Kinds;
   for (int i = 0, n = A->getNumValues(); i != n; ++i) {
     const char *Value = A->getValue(i);
diff --git a/clang/lib/Frontend/CompilerInvocation.cpp b/clang/lib/Frontend/CompilerInvocation.cpp
index 298fafc21588a1..348c56cc37da3f 100644
--- a/clang/lib/Frontend/CompilerInvocation.cpp
+++ b/clang/lib/Frontend/CompilerInvocation.cpp
@@ -1792,6 +1792,10 @@ void CompilerInvocationBase::GenerateCodeGenArgs(const CodeGenOptions &Opts,
   for (StringRef Sanitizer : serializeSanitizerKinds(Opts.SanitizeTrap))
     GenerateArg(Consumer, OPT_fsanitize_trap_EQ, Sanitizer);
 
+  for (StringRef Sanitizer :
+       serializeSanitizerKinds(Opts.SanitizeMergeHandlers))
+    GenerateArg(Consumer, OPT_fsanitize_merge_handlers_EQ, Sanitizer);
+
   if (!Opts.EmitVersionIdentMetadata)
     GenerateArg(Consumer, OPT_Qn);
 
@@ -2269,6 +2273,9 @@ bool CompilerInvocation::ParseCodeGenArgs(CodeGenOptions &Opts, ArgList &Args,
   parseSanitizerKinds("-fsanitize-trap=",
                       Args.getAllArgValues(OPT_fsanitize_trap_EQ), Diags,
                       Opts.SanitizeTrap);
+  parseSanitizerKinds("-fsanitize-merge=",
+                      Args.getAllArgValues(OPT_fsanitize_merge_handlers_EQ),
+                      Diags, Opts.SanitizeMergeHandlers);
 
   Opts.EmitVersionIdentMetadata = Args.hasFlag(OPT_Qy, OPT_Qn, true);
 
diff --git a/clang/test/CodeGen/ubsan-trap-debugloc.c b/clang/test/CodeGen/ubsan-trap-debugloc.c
index 4cad708b0ca50b..87cbfadec7d30c 100644
--- a/clang/test/CodeGen/ubsan-trap-debugloc.c
+++ b/clang/test/CodeGen/ubsan-trap-debugloc.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -emit-llvm -disable-llvm-passes -O -fsanitize=signed-integer-overflow -fsanitize-trap=signed-integer-overflow %s -o - -debug-info-kind=line-tables-only | FileCheck %s
+// RUN: %clang_cc1 -emit-llvm -disable-llvm-passes -O -fsanitize=signed-integer-overflow -fsanitize-trap=signed-integer-overflow -fsanitize-merge=signed-integer-overflow %s -o - -debug-info-kind=line-tables-only | FileCheck %s
 
 
 void foo(volatile int a) {
diff --git a/clang/test/CodeGen/ubsan-trap-merge.c b/clang/test/CodeGen/ubsan-trap-merge.c
index 412ec7b09744ef..f211150a09cb67 100644
--- a/clang/test/CodeGen/ubsan-trap-merge.c
+++ b/clang/test/CodeGen/ubsan-trap-merge.c
@@ -1,10 +1,26 @@
 // NOTE: Assertions have mostly been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --version 5
-// The most important assertion is the attributes at the end of the file, which
-// shows whether -ubsan-unique-traps attaches 'nomerge' to each ubsan call.
+// The most important assertions are the attributes at the end of the file, which
+// show whether -ubsan-unique-traps and -fno-sanitize-merge attach 'nomerge'
+// to each ubsan call.
 //
-// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -fsanitize=signed-integer-overflow -O3 -mllvm -ubsan-unique-traps %s -o - -fsanitize-trap=signed-integer-overflow | FileCheck %s --check-prefix=TRAP
-// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -fsanitize=signed-integer-overflow -O3 -mllvm -ubsan-unique-traps %s -o -                                         | FileCheck %s --check-prefix=HANDLER
-// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -fsanitize=signed-integer-overflow -O3 -mllvm -ubsan-unique-traps %s -o - -fsanitize-minimal-runtime              | FileCheck %s --check-prefix=MINRT
+// N.B. although the clang driver defaults to -fsanitize-merge=undefined,
+// clang_cc1 defaults to non-merge. (This is similar to -fsanitize-recover, for
+// which the default is also applied at the driver level only.)
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -fsanitize=signed-integer-overflow -O3 %s -o - -fsanitize-trap=signed-integer-overflow | FileCheck %s --check-prefixes=TRAP-NOMERGE
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -fsanitize=signed-integer-overflow -O3 %s -o -                                         | FileCheck %s --check-prefixes=HANDLER-NOMERGE
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -fsanitize=signed-integer-overflow -O3 %s -o - -fsanitize-minimal-runtime              | FileCheck %s --check-prefixes=MINRT-NOMERGE
+//
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -fsanitize=signed-integer-overflow -O3 -mllvm -ubsan-unique-traps %s -o - -fsanitize-trap=signed-integer-overflow | FileCheck %s --check-prefixes=TRAP-NOMERGE
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -fsanitize=signed-integer-overflow -O3 -mllvm -ubsan-unique-traps %s -o -                                         | FileCheck %s --check-prefixes=HANDLER-NOMERGE
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -fsanitize=signed-integer-overflow -O3 -mllvm -ubsan-unique-traps %s -o - -fsanitize-minimal-runtime              | FileCheck %s --check-prefixes=MINRT-NOMERGE
+//
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -fsanitize=signed-integer-overflow -O3 -fno-sanitize-merge=signed-integer-overflow  %s -o - -fsanitize-trap=signed-integer-overflow | FileCheck %s --check-prefixes=TRAP-NOMERGE
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -fsanitize=signed-integer-overflow -O3 -fno-sanitize-merge=signed-integer-overflow  %s -o -                                         | FileCheck %s --check-prefixes=HANDLER-NOMERGE
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -fsanitize=signed-integer-overflow -O3 -fno-sanitize-merge=signed-integer-overflow  %s -o - -fsanitize-minimal-runtime              | FileCheck %s --check-prefixes=MINRT-NOMERGE
+//
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -fsanitize=signed-integer-overflow -O3 -fsanitize-merge=signed-integer-overflow %s -o - -fsanitize-trap=signed-integer-overflow | FileCheck %s --check-prefixes=TRAP-MERGE
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -fsanitize=signed-integer-overflow -O3 -fsanitize-merge=signed-integer-overflow %s -o -                                         | FileCheck %s --check-prefixes=HANDLER-MERGE
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -fsanitize=signed-integer-overflow -O3 -fsanitize-merge=signed-integer-overflow %s -o - -fsanitize-minimal-runtime              | FileCheck %s --check-prefixes=MINRT-MERGE
 //
 // REQUIRES: x86-registered-target
 
@@ -47,6 +63,85 @@
 // MINRT:       [[CONT]]:
 // MINRT-NEXT:    [[TMP2:%.*]] = extractvalue { i32, i1 } [[TMP0]], 0, !nosanitize [[META2]]
 // MINRT-NEXT:    ret i32 [[TMP2]]
+// TRAP-NOMERGE-LABEL: define dso_local range(i32 -2147483523, -2147483648) i32 @f(
+// TRAP-NOMERGE-SAME: i32 noundef [[X:%.*]]) local_unnamed_addr #[[ATTR0:[0-9]+]] {
+// TRAP-NOMERGE-NEXT:  [[ENTRY:.*:]]
+// TRAP-NOMERGE-NEXT:    [[TMP0:%.*]] = tail call { i32, i1 } @llvm.sadd.with.overflow.i32(i32 [[X]], i32 125), !nosanitize [[META2:![0-9]+]]
+// TRAP-NOMERGE-NEXT:    [[TMP1:%.*]] = extractvalue { i32, i1 } [[TMP0]], 1, !nosanitize [[META2]]
+// TRAP-NOMERGE-NEXT:    br i1 [[TMP1]], label %[[TRAP:.*]], label %[[CONT:.*]], !nosanitize [[META2]]
+// TRAP-NOMERGE:       [[TRAP]]:
+// TRAP-NOMERGE-NEXT:    tail call void @llvm.ubsantrap(i8 0) #[[ATTR4:[0-9]+]], !nosanitize [[META2]]
+// TRAP-NOMERGE-NEXT:    unreachable, !nosanitize [[META2]]
+// TRAP-NOMERGE:       [[CONT]]:
+// TRAP-NOMERGE-NEXT:    [[TMP2:%.*]] = extractvalue { i32, i1 } [[TMP0]], 0, !nosanitize [[META2]]
+// TRAP-NOMERGE-NEXT:    ret i32 [[TMP2]]
+//
+// HANDLER-NOMERGE-LABEL: define dso_local range(i32 -2147483523, -2147483648) i32 @f(
+// HANDLER-NOMERGE-SAME: i32 noundef [[X:%.*]]) local_unnamed_addr #[[ATTR0:[0-9]+]] {
+// HANDLER-NOMERGE-NEXT:  [[ENTRY:.*:]]
+// HANDLER-NOMERGE-NEXT:    [[TMP0:%.*]] = tail call { i32, i1 } @llvm.sadd.with.overflow.i32(i32 [[X]], i32 125), !nosanitize [[META2:![0-9]+]]
+// HANDLER-NOMERGE-NEXT:    [[TMP1:%.*]] = extractvalue { i32, i1 } [[TMP0]], 1, !nosanitize [[META2]]
+// HANDLER-NOMERGE-NEXT:    br i1 [[TMP1]], label %[[HANDLER_ADD_OVERFLOW:.*]], label %[[CONT:.*]], !prof [[PROF3:![0-9]+]], !nosanitize [[META2]]
+// HANDLER-NOMERGE:       [[HANDLER_ADD_OVERFLOW]]:
+// HANDLER-NOMERGE-NEXT:    [[TMP2:%.*]] = zext nneg i32 [[X]] to i64, !nosanitize [[META2]]
+// HANDLER-NOMERGE-NEXT:    tail call void @__ubsan_handle_add_overflow_abort(ptr nonnull @[[GLOB1:[0-9]+]], i64 [[TMP2]], i64 125) #[[ATTR4:[0-9]+]], !nosanitize [[META2]]
+// HANDLER-NOMERGE-NEXT:    unreachable, !nosanitize [[META2]]
+// HANDLER-NOMERGE:       [[CONT]]:
+// HANDLER-NOMERGE-NEXT:    [[TMP3:%.*]] = extractvalue { i32, i1 } [[TMP0]], 0, !nosanitize [[META2]]
+// HANDLER-NOMERGE-NEXT:    ret i32 [[TMP3]]
+//
+// MINRT-NOMERGE-LABEL: define dso_local range(i32 -2147483523, -2147483648) i32 @f(
+// MINRT-NOMERGE-SAME: i32 noundef [[X:%.*]]) local_unnamed_addr #[[ATTR0:[0-9]+]] {
+// MINRT-NOMERGE-NEXT:  [[ENTRY:.*:]]
+// MINRT-NOMERGE-NEXT:    [[TMP0:%.*]] = tail call { i32, i1 } @llvm.sadd.with.overflow.i32(i32 [[X]], i32 125), !nosanitize [[META2:![0-9]+]]
+// MINRT-NO...
[truncated]

@llvmbot
Copy link
Member

llvmbot commented Dec 19, 2024

@llvm/pr-subscribers-clang-codegen

Author: Thurston Dang (thurstond)

Changes

…464)"

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.


Patch is 70.66 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/120511.diff

10 Files Affected:

  • (modified) clang/include/clang/Basic/CodeGenOptions.h (+4)
  • (modified) clang/include/clang/Driver/Options.td (+15)
  • (modified) clang/include/clang/Driver/SanitizerArgs.h (+1)
  • (modified) clang/lib/CodeGen/CGExpr.cpp (+17-13)
  • (modified) clang/lib/CodeGen/CodeGenFunction.h (+2-1)
  • (modified) clang/lib/Driver/SanitizerArgs.cpp (+24-7)
  • (modified) clang/lib/Frontend/CompilerInvocation.cpp (+7)
  • (modified) clang/test/CodeGen/ubsan-trap-debugloc.c (+1-1)
  • (modified) clang/test/CodeGen/ubsan-trap-merge.c (+500-29)
  • (modified) clang/test/Driver/fsanitize.c (+53)
diff --git a/clang/include/clang/Basic/CodeGenOptions.h b/clang/include/clang/Basic/CodeGenOptions.h
index 2dcf98b465661e..8097c9ef772bc7 100644
--- a/clang/include/clang/Basic/CodeGenOptions.h
+++ b/clang/include/clang/Basic/CodeGenOptions.h
@@ -380,6 +380,10 @@ class CodeGenOptions : public CodeGenOptionsBase {
   /// Set of sanitizer checks that trap rather than diagnose.
   SanitizerSet SanitizeTrap;
 
+  /// Set of sanitizer checks that can merge handlers (smaller code size at
+  /// the expense of debuggability).
+  SanitizerSet SanitizeMergeHandlers;
+
   /// List of backend command-line options for -fembed-bitcode.
   std::vector<uint8_t> CmdArgs;
 
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index 14e47f083ecec4..638f8c52053ec5 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -2555,6 +2555,21 @@ def fno_sanitize_trap : Flag<["-"], "fno-sanitize-trap">, Group<f_clang_Group>,
                         Alias<fno_sanitize_trap_EQ>, AliasArgs<["all"]>,
                         Visibility<[ClangOption, CLOption]>,
                         HelpText<"Disable trapping for all sanitizers">;
+def fsanitize_merge_handlers_EQ
+    : CommaJoined<["-"], "fsanitize-merge=">,
+      Group<f_clang_Group>,
+      HelpText<"Allow compiler to merge handlers for specified sanitizers">;
+def fno_sanitize_merge_handlers_EQ
+    : CommaJoined<["-"], "fno-sanitize-merge=">,
+      Group<f_clang_Group>,
+      HelpText<"Do not allow compiler to merge handlers for specified sanitizers">;
+def fsanitize_merge_handlers : Flag<["-"], "fsanitize-merge">, Group<f_clang_Group>,
+                     Alias<fsanitize_merge_handlers_EQ>, AliasArgs<["all"]>,
+                     HelpText<"Allow compiler to merge handlers for all sanitizers">;
+def fno_sanitize_merge_handlers : Flag<["-"], "fno-sanitize-merge">, Group<f_clang_Group>,
+                        Alias<fno_sanitize_merge_handlers_EQ>, AliasArgs<["all"]>,
+                        Visibility<[ClangOption, CLOption]>,
+                        HelpText<"Do not allow compiler to merge handlers for any sanitizers">;
 def fsanitize_undefined_trap_on_error
     : Flag<["-"], "fsanitize-undefined-trap-on-error">, Group<f_clang_Group>,
       Alias<fsanitize_trap_EQ>, AliasArgs<["undefined"]>;
diff --git a/clang/include/clang/Driver/SanitizerArgs.h b/clang/include/clang/Driver/SanitizerArgs.h
index 4f08ea2b260179..7410ad4303011c 100644
--- a/clang/include/clang/Driver/SanitizerArgs.h
+++ b/clang/include/clang/Driver/SanitizerArgs.h
@@ -25,6 +25,7 @@ class SanitizerArgs {
   SanitizerSet Sanitizers;
   SanitizerSet RecoverableSanitizers;
   SanitizerSet TrapSanitizers;
+  SanitizerSet MergeHandlers;
 
   std::vector<std::string> UserIgnorelistFiles;
   std::vector<std::string> SystemIgnorelistFiles;
diff --git a/clang/lib/CodeGen/CGExpr.cpp b/clang/lib/CodeGen/CGExpr.cpp
index 79955f55714164..d3fa5be6777ef4 100644
--- a/clang/lib/CodeGen/CGExpr.cpp
+++ b/clang/lib/CodeGen/CGExpr.cpp
@@ -3546,7 +3546,7 @@ static void emitCheckHandlerCall(CodeGenFunction &CGF,
                                  ArrayRef<llvm::Value *> FnArgs,
                                  SanitizerHandler CheckHandler,
                                  CheckRecoverableKind RecoverKind, bool IsFatal,
-                                 llvm::BasicBlock *ContBB) {
+                                 llvm::BasicBlock *ContBB, bool NoMerge) {
   assert(IsFatal || RecoverKind != CheckRecoverableKind::Unrecoverable);
   std::optional<ApplyDebugLocation> DL;
   if (!CGF.Builder.getCurrentDebugLocation()) {
@@ -3581,10 +3581,9 @@ 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>());
+  NoMerge = NoMerge || ClSanitizeDebugDeoptimization ||
+            !CGF.CGM.getCodeGenOpts().OptimizationLevel ||
+            (CGF.CurCodeDecl && CGF.CurCodeDecl->hasAttr<OptimizeNoneAttr>());
   if (NoMerge)
     HandlerCall->addFnAttr(llvm::Attribute::NoMerge);
   if (!MayReturn) {
@@ -3608,6 +3607,7 @@ void CodeGenFunction::EmitCheck(
   llvm::Value *FatalCond = nullptr;
   llvm::Value *RecoverableCond = nullptr;
   llvm::Value *TrapCond = nullptr;
+  bool NoMerge = false;
   for (int i = 0, n = Checked.size(); i < n; ++i) {
     llvm::Value *Check = Checked[i].first;
     // -fsanitize-trap= overrides -fsanitize-recover=.
@@ -3618,6 +3618,9 @@ void CodeGenFunction::EmitCheck(
                   ? RecoverableCond
                   : FatalCond;
     Cond = Cond ? Builder.CreateAnd(Cond, Check) : Check;
+
+    if (!CGM.getCodeGenOpts().SanitizeMergeHandlers.has(Checked[i].second))
+      NoMerge = true;
   }
 
   if (ClSanitizeGuardChecks) {
@@ -3632,7 +3635,7 @@ void CodeGenFunction::EmitCheck(
   }
 
   if (TrapCond)
-    EmitTrapCheck(TrapCond, CheckHandler);
+    EmitTrapCheck(TrapCond, CheckHandler, NoMerge);
   if (!FatalCond && !RecoverableCond)
     return;
 
@@ -3698,7 +3701,7 @@ void CodeGenFunction::EmitCheck(
     // Simple case: we need to generate a single handler call, either
     // fatal, or non-fatal.
     emitCheckHandlerCall(*this, FnType, Args, CheckHandler, RecoverKind,
-                         (FatalCond != nullptr), Cont);
+                         (FatalCond != nullptr), Cont, NoMerge);
   } else {
     // Emit two handler calls: first one for set of unrecoverable checks,
     // another one for recoverable.
@@ -3708,10 +3711,10 @@ void CodeGenFunction::EmitCheck(
     Builder.CreateCondBr(FatalCond, NonFatalHandlerBB, FatalHandlerBB);
     EmitBlock(FatalHandlerBB);
     emitCheckHandlerCall(*this, FnType, Args, CheckHandler, RecoverKind, true,
-                         NonFatalHandlerBB);
+                         NonFatalHandlerBB, NoMerge);
     EmitBlock(NonFatalHandlerBB);
     emitCheckHandlerCall(*this, FnType, Args, CheckHandler, RecoverKind, false,
-                         Cont);
+                         Cont, NoMerge);
   }
 
   EmitBlock(Cont);
@@ -3901,7 +3904,8 @@ void CodeGenFunction::EmitUnreachable(SourceLocation Loc) {
 }
 
 void CodeGenFunction::EmitTrapCheck(llvm::Value *Checked,
-                                    SanitizerHandler CheckHandlerID) {
+                                    SanitizerHandler CheckHandlerID,
+                                    bool NoMerge) {
   llvm::BasicBlock *Cont = createBasicBlock("cont");
 
   // If we're optimizing, collapse all calls to trap down to just one per
@@ -3911,9 +3915,9 @@ void CodeGenFunction::EmitTrapCheck(llvm::Value *Checked,
 
   llvm::BasicBlock *&TrapBB = TrapBBs[CheckHandlerID];
 
-  bool NoMerge = ClSanitizeDebugDeoptimization ||
-                 !CGM.getCodeGenOpts().OptimizationLevel ||
-                 (CurCodeDecl && CurCodeDecl->hasAttr<OptimizeNoneAttr>());
+  NoMerge = NoMerge || ClSanitizeDebugDeoptimization ||
+            !CGM.getCodeGenOpts().OptimizationLevel ||
+            (CurCodeDecl && CurCodeDecl->hasAttr<OptimizeNoneAttr>());
 
   if (TrapBB && !NoMerge) {
     auto Call = TrapBB->begin();
diff --git a/clang/lib/CodeGen/CodeGenFunction.h b/clang/lib/CodeGen/CodeGenFunction.h
index 847999cf1f28a0..4d4139180e100f 100644
--- a/clang/lib/CodeGen/CodeGenFunction.h
+++ b/clang/lib/CodeGen/CodeGenFunction.h
@@ -5171,7 +5171,8 @@ class CodeGenFunction : public CodeGenTypeCache {
 
   /// Create a basic block that will call the trap intrinsic, and emit a
   /// conditional branch to it, for the -ftrapv checks.
-  void EmitTrapCheck(llvm::Value *Checked, SanitizerHandler CheckHandlerID);
+  void EmitTrapCheck(llvm::Value *Checked, SanitizerHandler CheckHandlerID,
+                     bool NoMerge = false);
 
   /// Emit a call to trap or debugtrap and attach function attribute
   /// "trap-func-name" if specified.
diff --git a/clang/lib/Driver/SanitizerArgs.cpp b/clang/lib/Driver/SanitizerArgs.cpp
index 0edfe641416129..727974833e6d1e 100644
--- a/clang/lib/Driver/SanitizerArgs.cpp
+++ b/clang/lib/Driver/SanitizerArgs.cpp
@@ -68,6 +68,7 @@ static const SanitizerMask TrappingSupported =
     SanitizerKind::ImplicitConversion | SanitizerKind::Nullability |
     SanitizerKind::LocalBounds | SanitizerKind::CFI |
     SanitizerKind::FloatDivideByZero | SanitizerKind::ObjCCast;
+static const SanitizerMask MergeDefault = SanitizerKind::Undefined;
 static const SanitizerMask TrappingDefault = SanitizerKind::CFI;
 static const SanitizerMask CFIClasses =
     SanitizerKind::CFIVCall | SanitizerKind::CFINVCall |
@@ -696,6 +697,13 @@ SanitizerArgs::SanitizerArgs(const ToolChain &TC,
   TrappingKinds &= Kinds;
   RecoverableKinds &= ~TrappingKinds;
 
+  // Parse -f(no-)?sanitize-nonmerged-handlers flags
+  SanitizerMask MergeKinds =
+      parseSanitizeArgs(D, Args, DiagnoseErrors, MergeDefault, {}, {},
+                        options::OPT_fsanitize_merge_handlers_EQ,
+                        options::OPT_fno_sanitize_merge_handlers_EQ);
+  MergeKinds &= Kinds;
+
   // Setup ignorelist files.
   // Add default ignorelist from resource directory for activated sanitizers,
   // and validate special case lists format.
@@ -1114,6 +1122,8 @@ SanitizerArgs::SanitizerArgs(const ToolChain &TC,
   TrapSanitizers.Mask |= TrappingKinds;
   assert(!(RecoverableKinds & TrappingKinds) &&
          "Overlap between recoverable and trapping sanitizers");
+
+  MergeHandlers.Mask |= MergeKinds;
 }
 
 static std::string toString(const clang::SanitizerSet &Sanitizers) {
@@ -1275,6 +1285,10 @@ void SanitizerArgs::addArgs(const ToolChain &TC, const llvm::opt::ArgList &Args,
     CmdArgs.push_back(
         Args.MakeArgString("-fsanitize-trap=" + toString(TrapSanitizers)));
 
+  if (!MergeHandlers.empty())
+    CmdArgs.push_back(
+        Args.MakeArgString("-fsanitize-merge=" + toString(MergeHandlers)));
+
   addSpecialCaseListOpt(Args, CmdArgs,
                         "-fsanitize-ignorelist=", UserIgnorelistFiles);
   addSpecialCaseListOpt(Args, CmdArgs,
@@ -1442,13 +1456,16 @@ void SanitizerArgs::addArgs(const ToolChain &TC, const llvm::opt::ArgList &Args,
 
 SanitizerMask parseArgValues(const Driver &D, const llvm::opt::Arg *A,
                              bool DiagnoseErrors) {
-  assert((A->getOption().matches(options::OPT_fsanitize_EQ) ||
-          A->getOption().matches(options::OPT_fno_sanitize_EQ) ||
-          A->getOption().matches(options::OPT_fsanitize_recover_EQ) ||
-          A->getOption().matches(options::OPT_fno_sanitize_recover_EQ) ||
-          A->getOption().matches(options::OPT_fsanitize_trap_EQ) ||
-          A->getOption().matches(options::OPT_fno_sanitize_trap_EQ)) &&
-         "Invalid argument in parseArgValues!");
+  assert(
+      (A->getOption().matches(options::OPT_fsanitize_EQ) ||
+       A->getOption().matches(options::OPT_fno_sanitize_EQ) ||
+       A->getOption().matches(options::OPT_fsanitize_recover_EQ) ||
+       A->getOption().matches(options::OPT_fno_sanitize_recover_EQ) ||
+       A->getOption().matches(options::OPT_fsanitize_trap_EQ) ||
+       A->getOption().matches(options::OPT_fno_sanitize_trap_EQ) ||
+       A->getOption().matches(options::OPT_fsanitize_merge_handlers_EQ) ||
+       A->getOption().matches(options::OPT_fno_sanitize_merge_handlers_EQ)) &&
+      "Invalid argument in parseArgValues!");
   SanitizerMask Kinds;
   for (int i = 0, n = A->getNumValues(); i != n; ++i) {
     const char *Value = A->getValue(i);
diff --git a/clang/lib/Frontend/CompilerInvocation.cpp b/clang/lib/Frontend/CompilerInvocation.cpp
index 298fafc21588a1..348c56cc37da3f 100644
--- a/clang/lib/Frontend/CompilerInvocation.cpp
+++ b/clang/lib/Frontend/CompilerInvocation.cpp
@@ -1792,6 +1792,10 @@ void CompilerInvocationBase::GenerateCodeGenArgs(const CodeGenOptions &Opts,
   for (StringRef Sanitizer : serializeSanitizerKinds(Opts.SanitizeTrap))
     GenerateArg(Consumer, OPT_fsanitize_trap_EQ, Sanitizer);
 
+  for (StringRef Sanitizer :
+       serializeSanitizerKinds(Opts.SanitizeMergeHandlers))
+    GenerateArg(Consumer, OPT_fsanitize_merge_handlers_EQ, Sanitizer);
+
   if (!Opts.EmitVersionIdentMetadata)
     GenerateArg(Consumer, OPT_Qn);
 
@@ -2269,6 +2273,9 @@ bool CompilerInvocation::ParseCodeGenArgs(CodeGenOptions &Opts, ArgList &Args,
   parseSanitizerKinds("-fsanitize-trap=",
                       Args.getAllArgValues(OPT_fsanitize_trap_EQ), Diags,
                       Opts.SanitizeTrap);
+  parseSanitizerKinds("-fsanitize-merge=",
+                      Args.getAllArgValues(OPT_fsanitize_merge_handlers_EQ),
+                      Diags, Opts.SanitizeMergeHandlers);
 
   Opts.EmitVersionIdentMetadata = Args.hasFlag(OPT_Qy, OPT_Qn, true);
 
diff --git a/clang/test/CodeGen/ubsan-trap-debugloc.c b/clang/test/CodeGen/ubsan-trap-debugloc.c
index 4cad708b0ca50b..87cbfadec7d30c 100644
--- a/clang/test/CodeGen/ubsan-trap-debugloc.c
+++ b/clang/test/CodeGen/ubsan-trap-debugloc.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -emit-llvm -disable-llvm-passes -O -fsanitize=signed-integer-overflow -fsanitize-trap=signed-integer-overflow %s -o - -debug-info-kind=line-tables-only | FileCheck %s
+// RUN: %clang_cc1 -emit-llvm -disable-llvm-passes -O -fsanitize=signed-integer-overflow -fsanitize-trap=signed-integer-overflow -fsanitize-merge=signed-integer-overflow %s -o - -debug-info-kind=line-tables-only | FileCheck %s
 
 
 void foo(volatile int a) {
diff --git a/clang/test/CodeGen/ubsan-trap-merge.c b/clang/test/CodeGen/ubsan-trap-merge.c
index 412ec7b09744ef..f211150a09cb67 100644
--- a/clang/test/CodeGen/ubsan-trap-merge.c
+++ b/clang/test/CodeGen/ubsan-trap-merge.c
@@ -1,10 +1,26 @@
 // NOTE: Assertions have mostly been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --version 5
-// The most important assertion is the attributes at the end of the file, which
-// shows whether -ubsan-unique-traps attaches 'nomerge' to each ubsan call.
+// The most important assertions are the attributes at the end of the file, which
+// show whether -ubsan-unique-traps and -fno-sanitize-merge attach 'nomerge'
+// to each ubsan call.
 //
-// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -fsanitize=signed-integer-overflow -O3 -mllvm -ubsan-unique-traps %s -o - -fsanitize-trap=signed-integer-overflow | FileCheck %s --check-prefix=TRAP
-// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -fsanitize=signed-integer-overflow -O3 -mllvm -ubsan-unique-traps %s -o -                                         | FileCheck %s --check-prefix=HANDLER
-// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -fsanitize=signed-integer-overflow -O3 -mllvm -ubsan-unique-traps %s -o - -fsanitize-minimal-runtime              | FileCheck %s --check-prefix=MINRT
+// N.B. although the clang driver defaults to -fsanitize-merge=undefined,
+// clang_cc1 defaults to non-merge. (This is similar to -fsanitize-recover, for
+// which the default is also applied at the driver level only.)
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -fsanitize=signed-integer-overflow -O3 %s -o - -fsanitize-trap=signed-integer-overflow | FileCheck %s --check-prefixes=TRAP-NOMERGE
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -fsanitize=signed-integer-overflow -O3 %s -o -                                         | FileCheck %s --check-prefixes=HANDLER-NOMERGE
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -fsanitize=signed-integer-overflow -O3 %s -o - -fsanitize-minimal-runtime              | FileCheck %s --check-prefixes=MINRT-NOMERGE
+//
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -fsanitize=signed-integer-overflow -O3 -mllvm -ubsan-unique-traps %s -o - -fsanitize-trap=signed-integer-overflow | FileCheck %s --check-prefixes=TRAP-NOMERGE
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -fsanitize=signed-integer-overflow -O3 -mllvm -ubsan-unique-traps %s -o -                                         | FileCheck %s --check-prefixes=HANDLER-NOMERGE
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -fsanitize=signed-integer-overflow -O3 -mllvm -ubsan-unique-traps %s -o - -fsanitize-minimal-runtime              | FileCheck %s --check-prefixes=MINRT-NOMERGE
+//
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -fsanitize=signed-integer-overflow -O3 -fno-sanitize-merge=signed-integer-overflow  %s -o - -fsanitize-trap=signed-integer-overflow | FileCheck %s --check-prefixes=TRAP-NOMERGE
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -fsanitize=signed-integer-overflow -O3 -fno-sanitize-merge=signed-integer-overflow  %s -o -                                         | FileCheck %s --check-prefixes=HANDLER-NOMERGE
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -fsanitize=signed-integer-overflow -O3 -fno-sanitize-merge=signed-integer-overflow  %s -o - -fsanitize-minimal-runtime              | FileCheck %s --check-prefixes=MINRT-NOMERGE
+//
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -fsanitize=signed-integer-overflow -O3 -fsanitize-merge=signed-integer-overflow %s -o - -fsanitize-trap=signed-integer-overflow | FileCheck %s --check-prefixes=TRAP-MERGE
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -fsanitize=signed-integer-overflow -O3 -fsanitize-merge=signed-integer-overflow %s -o -                                         | FileCheck %s --check-prefixes=HANDLER-MERGE
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -fsanitize=signed-integer-overflow -O3 -fsanitize-merge=signed-integer-overflow %s -o - -fsanitize-minimal-runtime              | FileCheck %s --check-prefixes=MINRT-MERGE
 //
 // REQUIRES: x86-registered-target
 
@@ -47,6 +63,85 @@
 // MINRT:       [[CONT]]:
 // MINRT-NEXT:    [[TMP2:%.*]] = extractvalue { i32, i1 } [[TMP0]], 0, !nosanitize [[META2]]
 // MINRT-NEXT:    ret i32 [[TMP2]]
+// TRAP-NOMERGE-LABEL: define dso_local range(i32 -2147483523, -2147483648) i32 @f(
+// TRAP-NOMERGE-SAME: i32 noundef [[X:%.*]]) local_unnamed_addr #[[ATTR0:[0-9]+]] {
+// TRAP-NOMERGE-NEXT:  [[ENTRY:.*:]]
+// TRAP-NOMERGE-NEXT:    [[TMP0:%.*]] = tail call { i32, i1 } @llvm.sadd.with.overflow.i32(i32 [[X]], i32 125), !nosanitize [[META2:![0-9]+]]
+// TRAP-NOMERGE-NEXT:    [[TMP1:%.*]] = extractvalue { i32, i1 } [[TMP0]], 1, !nosanitize [[META2]]
+// TRAP-NOMERGE-NEXT:    br i1 [[TMP1]], label %[[TRAP:.*]], label %[[CONT:.*]], !nosanitize [[META2]]
+// TRAP-NOMERGE:       [[TRAP]]:
+// TRAP-NOMERGE-NEXT:    tail call void @llvm.ubsantrap(i8 0) #[[ATTR4:[0-9]+]], !nosanitize [[META2]]
+// TRAP-NOMERGE-NEXT:    unreachable, !nosanitize [[META2]]
+// TRAP-NOMERGE:       [[CONT]]:
+// TRAP-NOMERGE-NEXT:    [[TMP2:%.*]] = extractvalue { i32, i1 } [[TMP0]], 0, !nosanitize [[META2]]
+// TRAP-NOMERGE-NEXT:    ret i32 [[TMP2]]
+//
+// HANDLER-NOMERGE-LABEL: define dso_local range(i32 -2147483523, -2147483648) i32 @f(
+// HANDLER-NOMERGE-SAME: i32 noundef [[X:%.*]]) local_unnamed_addr #[[ATTR0:[0-9]+]] {
+// HANDLER-NOMERGE-NEXT:  [[ENTRY:.*:]]
+// HANDLER-NOMERGE-NEXT:    [[TMP0:%.*]] = tail call { i32, i1 } @llvm.sadd.with.overflow.i32(i32 [[X]], i32 125), !nosanitize [[META2:![0-9]+]]
+// HANDLER-NOMERGE-NEXT:    [[TMP1:%.*]] = extractvalue { i32, i1 } [[TMP0]], 1, !nosanitize [[META2]]
+// HANDLER-NOMERGE-NEXT:    br i1 [[TMP1]], label %[[HANDLER_ADD_OVERFLOW:.*]], label %[[CONT:.*]], !prof [[PROF3:![0-9]+]], !nosanitize [[META2]]
+// HANDLER-NOMERGE:       [[HANDLER_ADD_OVERFLOW]]:
+// HANDLER-NOMERGE-NEXT:    [[TMP2:%.*]] = zext nneg i32 [[X]] to i64, !nosanitize [[META2]]
+// HANDLER-NOMERGE-NEXT:    tail call void @__ubsan_handle_add_overflow_abort(ptr nonnull @[[GLOB1:[0-9]+]], i64 [[TMP2]], i64 125) #[[ATTR4:[0-9]+]], !nosanitize [[META2]]
+// HANDLER-NOMERGE-NEXT:    unreachable, !nosanitize [[META2]]
+// HANDLER-NOMERGE:       [[CONT]]:
+// HANDLER-NOMERGE-NEXT:    [[TMP3:%.*]] = extractvalue { i32, i1 } [[TMP0]], 0, !nosanitize [[META2]]
+// HANDLER-NOMERGE-NEXT:    ret i32 [[TMP3]]
+//
+// MINRT-NOMERGE-LABEL: define dso_local range(i32 -2147483523, -2147483648) i32 @f(
+// MINRT-NOMERGE-SAME: i32 noundef [[X:%.*]]) local_unnamed_addr #[[ATTR0:[0-9]+]] {
+// MINRT-NOMERGE-NEXT:  [[ENTRY:.*:]]
+// MINRT-NOMERGE-NEXT:    [[TMP0:%.*]] = tail call { i32, i1 } @llvm.sadd.with.overflow.i32(i32 [[X]], i32 125), !nosanitize [[META2:![0-9]+]]
+// MINRT-NO...
[truncated]

@thurstond thurstond changed the title Reapply "[ubsan] Add -fsanitize-merge (and -fno-sanitize-merge) (#120… Reapply "[ubsan] Add -fsanitize-merge (and -fno-sanitize-merge) (#120…464)" Dec 19, 2024
// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -fsanitize=signed-integer-overflow -O3 -mllvm -ubsan-unique-traps %s -o - | FileCheck %s --check-prefix=HANDLER
// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -fsanitize=signed-integer-overflow -O3 -mllvm -ubsan-unique-traps %s -o - -fsanitize-minimal-runtime | FileCheck %s --check-prefix=MINRT
// N.B. although the clang driver defaults to -fsanitize-merge=undefined,
// clang_cc1 defaults to non-merge. (This is similar to -fsanitize-recover, for
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about removing -ubsan-unique-traps from tests?
Later we can remove it completely.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe removing from tests later is also OK

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am planning to write a patch that will remove -ubsan-unique-traps and update the tests accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am planning to write a patch that will remove -ubsan-unique-traps and update the tests accordingly.

#120613

@thurstond thurstond merged commit ffff7bb into llvm:main Dec 19, 2024
6 of 7 checks passed
thurstond added a commit to thurstond/llvm-project that referenced this pull request Dec 19, 2024
-fno-sanitize-merge (introduced in llvm#120511) duplicates the functionality of -ubsan-unique-traps but also allows individual checks to be specified e.g.,
* "-fno-sanitize-merge" without arguments is equivalent to -ubsan-unique-traps
* "-fno-sanitize-merge=bool,enum" will apply it only to those two checks

This patch also adds negative test examples to bounds-checking.c, and
strengthens the NOOPTARRAY assertion to prevent spurious matches.

Note: this change necessarily breaks compatibility with '-mllvm
-ubsan-unique-traps'. We hope that this is acceptable since '-mllvm
-ubsan-unique-traps' was an experimental flag.

"-bounds-checking-unique-traps" is unaffected by this patch.
thurstond added a commit that referenced this pull request Dec 19, 2024
…#120613)

-fno-sanitize-merge (introduced in
#120511) duplicates the
functionality of -ubsan-unique-traps but also allows individual checks
to be specified e.g.,
* "-fno-sanitize-merge" without arguments is equivalent to
-ubsan-unique-traps
* "-fno-sanitize-merge=bool,enum" will apply it only to those two checks

Additionally, the naming is more consistent with the rest of the
-fsanitize- family.

This patch therefore removes -ubsan-unique-traps. This breaks backwards
compatibility; we hope that this is acceptable since '-mllvm
-ubsan-unique-traps' was an experimental flag.

This patch also adds negative test examples to bounds-checking.c, and
strengthens the NOOPTARRAY assertion to prevent spurious matches.

"-bounds-checking-unique-traps" is unaffected by this patch.
thurstond added a commit to thurstond/llvm-project that referenced this pull request Dec 20, 2024
…e=local-bounds)

-fno-sanitize-merge (introduced in llvm#120511) combines the functionality of -ubsan-unique-traps
and -bounds-checking-unique-traps, while allowing fine-grained control
of which UBSan checks to prevent merging. llvm#120613 removed -ubsan-unique-traps.
This patch removes -bound-checking-unique-traps, which can be controlled
via -fno-sanitize-merge=local-bounds.

Note: this patch subtly changes -fsanitize-merge (the default) to also
include -fsanitize-merge=local-bounds. This is different from the
previous behavior, where -fsanitize-merge (or the old
-ubsan-unique-traps) did not affect local-bounds (requiring the separate
-bounds-checking-unique-traps). However, we argue that the new behavior
is more intuitive.

Removing -bounds-checking-unique-traps and merging its functionality into -fsanitize-merge breaks backwards compatibility; we hope that this is acceptable since '-mllvm -bounds-checking-unique-traps' was an experimental flag.
thurstond added a commit that referenced this pull request Dec 20, 2024
…e=local-bounds) (#120682)

#120613 removed -ubsan-unique-traps and replaced it with
-fno-sanitize-merge (introduced in #120511), which allows fine-grained
control of which UBSan checks to prevent merging. This analogous patch
removes -bound-checking-unique-traps, and allows it to be controlled via
-fno-sanitize-merge=local-bounds.

Most of this patch is simply plumbing through the compiler flags into
the bounds checking pass.

Note: this patch subtly changes -fsanitize-merge (the default) to also
include -fsanitize-merge=local-bounds. This is different from the
previous behavior, where -fsanitize-merge (or the old
-ubsan-unique-traps) did not affect local-bounds (requiring the separate
-bounds-checking-unique-traps). However, we argue that the new behavior
is more intuitive.

Removing -bounds-checking-unique-traps and merging its functionality
into -fsanitize-merge breaks backwards compatibility; we hope that this
is acceptable since '-mllvm -bounds-checking-unique-traps' was an
experimental flag.
github-actions bot pushed a commit to arm/arm-toolchain that referenced this pull request Jan 10, 2025
…tize-merge) (#120613)

-fno-sanitize-merge (introduced in
llvm/llvm-project#120511) duplicates the
functionality of -ubsan-unique-traps but also allows individual checks
to be specified e.g.,
* "-fno-sanitize-merge" without arguments is equivalent to
-ubsan-unique-traps
* "-fno-sanitize-merge=bool,enum" will apply it only to those two checks

Additionally, the naming is more consistent with the rest of the
-fsanitize- family.

This patch therefore removes -ubsan-unique-traps. This breaks backwards
compatibility; we hope that this is acceptable since '-mllvm
-ubsan-unique-traps' was an experimental flag.

This patch also adds negative test examples to bounds-checking.c, and
strengthens the NOOPTARRAY assertion to prevent spurious matches.

"-bounds-checking-unique-traps" is unaffected by this patch.
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:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants