[Sanitizers] Add a flag -f[no-]sanitize-merge-traps

[rnk]

Summary

Without this flag, enabling optimizations causes clang to emit a single
ubsantrap for every check failure of a particular kind. Adding this flag
allows the user to control this behavior separately, so they can choose
to have increased code size in exchange for more debuggable code.

A Chrome developer requested this feature here:
https://crbug.com/1185451

I made this change in such a way that it doesn't litter the cc1 line
with redundant flags: if the user does not pass the positive or negative
version if this flag, it is not forwarded to the cc1 invocation.

[jansvoboda11]

The CodeGen tests you added are failing pre-merge checks. This is most likely because we recently (D97462) started verifying that all all CC1 command line options can be serialized from a CompilerInvocation instance. To reproduce this locally, you'd need to build with assertions or manually set -DCLANG_ROUND_TRIP_CC1_ARGS=ON when building with CMake.

The solution would be to add complementary code that generates "-f[no-]sanitize-merge-traps" from CodeGenOptions::SanitizeMergeTraps in CompilerInvocation::GenerateCodeGenArgs.

More info is here https://clang.llvm.org/docs/InternalsManual.html#compiler-invocation and in the following sections.

clang/include/clang/Basic/CodeGenOptions.def

CODEGENOPT(SanitizeCoverageTracePC, 1, 0) ///< Enable PC tracing
                                           ///< in sanitizer coverage.
 CODEGENOPT(SanitizeCoverageTracePCGuard, 1, 0) ///< Enable PC tracing with guard
                                                ///< in sanitizer coverage.
 CODEGENOPT(SanitizeCoverageInline8bitCounters, 1, 0) ///< Use inline 8bit counters.
 CODEGENOPT(SanitizeCoverageInlineBoolFlag, 1, 0) ///< Use inline bool flag.
 CODEGENOPT(SanitizeCoveragePCTable, 1, 0) ///< Create a PC Table.
 CODEGENOPT(SanitizeCoverageNoPrune, 1, 0) ///< Disable coverage pruning.
 CODEGENOPT(SanitizeCoverageStackDepth, 1, 0) ///< Enable max stack depth tracing
+CODEGENOPT(SanitizeMergeTraps, 1, 0) ///< Whether to share trap instructions.
 CODEGENOPT(SanitizeStats     , 1, 0) ///< Collect statistics for sanitizers.
 CODEGENOPT(SimplifyLibCalls  , 1, 1) ///< Set when -fbuiltin is enabled.
 CODEGENOPT(SoftFloat         , 1, 0) ///< -soft-float.
 CODEGENOPT(SpeculativeLoadHardening, 1, 0) ///< Enable speculative load hardening.
 CODEGENOPT(FineGrainedBitfieldAccesses, 1, 0) ///< Enable fine-grained bitfield accesses.
 CODEGENOPT(StrictEnums       , 1, 0) ///< Optimize based on strict enum definition.
 CODEGENOPT(StrictVTablePointers, 1, 0) ///< Optimize based on the strict vtable pointers
 CODEGENOPT(TimePasses        , 1, 0) ///< Set when -ftime-report or -ftime-report= is enabled.

clang/include/clang/Driver/Options.td

defm sanitize_cfi_canonical_jump_tables : BoolOption<"f", "sanitize-cfi-canonical-jump-tables",
   PosFlag<SetTrue, [], "Make">, NegFlag<SetFalse, [CoreOption, NoXarchOption], "Do not make">,
   BothFlags<[], " the jump table addresses canonical in the symbol table">>,
   Group<f_clang_Group>;
 defm sanitize_stats : BoolOption<"f", "sanitize-stats",
   CodeGenOpts<"SanitizeStats">, DefaultFalse,
   PosFlag<SetTrue, [], "Enable">, NegFlag<SetFalse, [CoreOption, NoXarchOption], "Disable">,
   BothFlags<[], " sanitizer statistics gathering.">>,
   Group<f_clang_Group>;
+def fsanitize_merge_traps : Flag<["-"], "fsanitize-merge-traps">,
+                                     Group<f_clang_Group>,
+                                     HelpText<"Merge sanitizer check trap instructions">;
+def fno_sanitize_merge_traps : Flag<["-"], "fno-sanitize-merge-traps">,
+                                        Group<f_clang_Group>,
+                                        Flags<[CoreOption, NoXarchOption]>,
+                                        HelpText<"Do not merge sanitizer check trap instructions">;
 def fsanitize_thread_memory_access : Flag<["-"], "fsanitize-thread-memory-access">,
                                      Group<f_clang_Group>,
                                      HelpText<"Enable memory access instrumentation in ThreadSanitizer (default)">;
 def fno_sanitize_thread_memory_access : Flag<["-"], "fno-sanitize-thread-memory-access">,
                                         Group<f_clang_Group>,
                                         Flags<[CoreOption, NoXarchOption]>,
                                         HelpText<"Disable memory access instrumentation in ThreadSanitizer">;
 def fsanitize_thread_func_entry_exit : Flag<["-"], "fsanitize-thread-func-entry-exit">,

clang/include/clang/Driver/SanitizerArgs.h

class SanitizerArgs {
   bool Stats = false;
   bool TsanMemoryAccess = true;
   bool TsanFuncEntryExit = true;
   bool TsanAtomics = true;
   bool MinimalRuntime = false;
   // True if cross-dso CFI support if provided by the system (i.e. Android).
   bool ImplicitCfiRuntime = false;
   bool NeedsMemProfRt = false;
+  Optional<bool> MergeTraps;
 
 public:
   /// Parses the sanitizer arguments from an argument list.
   SanitizerArgs(const ToolChain &TC, const llvm::opt::ArgList &Args);
 
   bool needsSharedRt() const { return SharedRuntime; }
 
   bool needsMemProfRt() const { return NeedsMemProfRt; }

clang/lib/CodeGen/CGExpr.cpp

void CodeGenFunction::EmitTrapCheck(llvm::Value *Checked,
   llvm::BasicBlock *Cont = createBasicBlock("cont");
 
   // If we're optimizing, collapse all calls to trap down to just one per
   // check-type per function to save on code size.
   if (TrapBBs.size() <= CheckHandlerID)
     TrapBBs.resize(CheckHandlerID + 1);
   llvm::BasicBlock *&TrapBB = TrapBBs[CheckHandlerID];
 
-  if (!CGM.getCodeGenOpts().OptimizationLevel || !TrapBB) {
+  if (!CGM.getCodeGenOpts().SanitizeMergeTraps || !TrapBB) {
     TrapBB = createBasicBlock("trap");
     Builder.CreateCondBr(Checked, Cont, TrapBB);
     EmitBlock(TrapBB);
 
     llvm::CallInst *TrapCall =
         Builder.CreateCall(CGM.getIntrinsic(llvm::Intrinsic::ubsantrap),
                            llvm::ConstantInt::get(CGM.Int8Ty, CheckHandlerID));
 
     if (!CGM.getCodeGenOpts().TrapFuncName.empty()) {
       auto A = llvm::Attribute::get(getLLVMContext(), "trap-func-name",
                                     CGM.getCodeGenOpts().TrapFuncName);
       TrapCall->addAttribute(llvm::AttributeList::FunctionIndex, A);
     }
     TrapCall->setDoesNotReturn();
     TrapCall->setDoesNotThrow();
+    if (!CGM.getCodeGenOpts().SanitizeMergeTraps) {
+      // Prevent the optimizer from merging traps if the user asked us not
+      // to.
+      TrapCall->addAttribute(llvm::AttributeList::FunctionIndex,
+                             llvm::Attribute::NoMerge);
+    }
     Builder.CreateUnreachable();
   } else {
     auto Call = TrapBB->begin();
     assert(isa<llvm::CallInst>(Call) && "Expected call in trap BB");
 
     Call->applyMergedLocation(Call->getDebugLoc(),
                               Builder.getCurrentDebugLocation());
     Builder.CreateCondBr(Checked, Cont, TrapBB);

clang/lib/Driver/SanitizerArgs.cpp

if (AllAddedKinds & SanitizerKind::CFI) {
     CfiCanonicalJumpTables =
         Args.hasFlag(options::OPT_fsanitize_cfi_canonical_jump_tables,
                      options::OPT_fno_sanitize_cfi_canonical_jump_tables, true);
   }
 
   Stats = Args.hasFlag(options::OPT_fsanitize_stats,
                        options::OPT_fno_sanitize_stats, false);
 
+  // If the user didn't pass an explicit sanitize-merge-traps value, don't add
+  // it to the cc1 line.
+  if (Args.hasArg(options::OPT_fsanitize_merge_traps,
+                  options::OPT_fno_sanitize_merge_traps)) {
+    MergeTraps = Args.hasFlag(options::OPT_fsanitize_merge_traps,
+                              options::OPT_fno_sanitize_merge_traps);
+  }
+
   if (MinimalRuntime) {
     SanitizerMask IncompatibleMask =
         Kinds & ~setGroupBits(CompatibleWithMinimalRuntime);
     if (IncompatibleMask)
       D.Diag(clang::diag::err_drv_argument_not_allowed_with)
           << "-fsanitize-minimal-runtime"
           << lastArgumentForMask(D, Args, IncompatibleMask);
 
if (CfiICallGeneralizePointers)
     CmdArgs.push_back("-fsanitize-cfi-icall-generalize-pointers");
 
   if (CfiCanonicalJumpTables)
     CmdArgs.push_back("-fsanitize-cfi-canonical-jump-tables");
 
   if (Stats)
     CmdArgs.push_back("-fsanitize-stats");
 
+  if (MergeTraps) {
+    if (*MergeTraps)
+      CmdArgs.push_back("-fsanitize-merge-traps");
+    else
+      CmdArgs.push_back("-fno-sanitize-merge-traps");
+  }
+
   if (MinimalRuntime)
     CmdArgs.push_back("-fsanitize-minimal-runtime");
 
   if (AsanFieldPadding)
     CmdArgs.push_back(Args.MakeArgString("-fsanitize-address-field-padding=" +
                                          Twine(AsanFieldPadding)));
 
   if (AsanUseAfterScope)

clang/lib/Frontend/CompilerInvocation.cpp

#undef CODEGEN_OPTION_WITH_MARSHALLING
   // FIXME: Report unrecoverable sanitizers incorrectly specified here.
   parseSanitizerKinds("-fsanitize-recover=",
                       Args.getAllArgValues(OPT_fsanitize_recover_EQ), Diags,
                       Opts.SanitizeRecover);
   parseSanitizerKinds("-fsanitize-trap=",
                       Args.getAllArgValues(OPT_fsanitize_trap_EQ), Diags,
                       Opts.SanitizeTrap);
 
+  // Merging sanitizer traps saves code size, but makes debugging harder. By
+  // default, merge them when optimizations are enabled. Let the user override
+  // the setting either way.
+  Opts.SanitizeMergeTraps = Args.hasFlag(options::OPT_fsanitize_merge_traps,
+                                         options::OPT_fno_sanitize_merge_traps,
+                                         OptimizationLevel > 0);
+
   Opts.EmitVersionIdentMetadata = Args.hasFlag(OPT_Qy, OPT_Qn, true);
 
   if (Args.hasArg(options::OPT_ffinite_loops))
     Opts.FiniteLoops = CodeGenOptions::FiniteLoopsKind::Always;
   else if (Args.hasArg(options::OPT_fno_finite_loops))
     Opts.FiniteLoops = CodeGenOptions::FiniteLoopsKind::Never;
 
   return Success && Diags.getNumErrors() == NumErrorsBefore;

clang/test/CodeGen/catch-undef-behavior.c

void call_nonnull_variadic(int a, int *b) {
   // CHECK-UBSAN-NOT: __ubsan_handle_nonnull_arg
 
   // CHECK-COMMON: call void (i32, ...) @nonnull_variadic
   nonnull_variadic(a, b);
 }
 
 // CHECK-UBSAN: ![[WEIGHT_MD]] = !{!"branch_weights", i32 1048575, i32 1}
 
-// CHECK-TRAP: attributes [[NR_NUW]] = { noreturn nounwind }
+// CHECK-TRAP: attributes [[NR_NUW]] = { nomerge noreturn nounwind }

clang/test/CodeGen/cfi-nomerge.c

+// RUN: %clang_cc1 -triple x86_64-unknown-linux -fsanitize=cfi-icall -fsanitize-trap=cfi-icall -emit-llvm -o - %s -fsanitize-merge-traps | FileCheck --check-prefix=MERGE %s
+// RUN: %clang_cc1 -triple x86_64-unknown-linux -fsanitize=cfi-icall -fsanitize-trap=cfi-icall -emit-llvm -o - %s -fno-sanitize-merge-traps | FileCheck --check-prefix=NOMERGE %s
+
+int twoChecks(void (*f)(), void (*g)()) {
+  f();
+  g();
+  return 42;
+}
+
+// NOMERGE-LABEL: define dso_local i32 @twoChecks(
+// NOMERGE:      call void @llvm.ubsantrap(i8 2) #[[ATTR:[0-9]+]]
+// NOMERGE-NEXT: unreachable
+// NOMERGE:      call void @llvm.ubsantrap(i8 2) #[[ATTR]]
+// NOMERGE-NEXT: unreachable
+
+// NOMERGE: #[[ATTR]] = { nomerge noreturn nounwind }
+
+// MERGE-LABEL: define dso_local i32 @twoChecks(
+// MERGE:      call void @llvm.ubsantrap(i8 2)
+// MERGE-NEXT: unreachable
+// MERGE-NOT: llvm.ubsantrap
+// MERGE: ret i32 42

clang/test/CodeGen/trapv-nomerge.c

+// RUN: %clang_cc1 -O2 -triple x86_64-apple-darwin10 -ftrapv -fno-sanitize-merge-traps %s -emit-llvm -o - | FileCheck %s --check-prefix=NOMERGE
+// RUN: %clang_cc1 -O2 -triple x86_64-apple-darwin10 -ftrapv -fsanitize-merge-traps %s -emit-llvm -o - | FileCheck %s --check-prefix=MERGE
+
+unsigned int ui, uj, uk;
+int i, j, k;
+
+int twoChecks() {
+  ++i;
+  ++j;
+  return 42;
+}
+
+// NOMERGE-LABEL: define i32 @twoChecks()
+// NOMERGE:      call void @llvm.ubsantrap(i8 0) #[[ATTR:[0-9]+]]
+// NOMERGE-NEXT: unreachable
+// NOMERGE:      call void @llvm.ubsantrap(i8 0) #[[ATTR]]
+// NOMERGE-NEXT: unreachable
+
+// NOMERGE: #[[ATTR]] = { nomerge noreturn nounwind }
+
+// MERGE-LABEL: define i32 @twoChecks()
+// MERGE:      call void @llvm.ubsantrap(i8 0)
+// MERGE-NEXT: unreachable
+// MERGE-NOT: llvm.ubsantrap
+// MERGE: ret i32 42

clang/test/Driver/sanitize-merge-traps.c

+// RUN: %clang -c %s -fsanitize=cfi -flto -O2 -fno-sanitize-merge-traps -### 2>&1 | FileCheck %s --check-prefix=NOMERGE
+// RUN: %clang -c %s -fsanitize=cfi -flto -O0 -fsanitize-merge-traps -### 2>&1 | FileCheck %s --check-prefix=MERGE
+// RUN: %clang -c %s -fsanitize=cfi -flto -O2 -### 2>&1 | FileCheck %s --check-prefix=NONE
+// RUN: %clang -c %s -fsanitize=cfi -flto -O0 -### 2>&1 | FileCheck %s --check-prefix=NONE
+
+// NOMERGE: "-fno-sanitize-merge-traps"
+// MERGE: "-fsanitize-merge-traps"
+// NONE-NOT: "-f{{(no-)?}}sanitize-merge-traps"