Modify BoundsSan to improve debuggability

[oskarwirga]

Summary

Context

BoundsSanitizer is a mitigation that is part of UBSAN. It can be enabled in "trap" mode to crash on OOB array accesses.

Problem

BoundsSan has zero false positives meaning every crash is a OOB array access, unfortunately optimizations cause these crashes in production builds to be a bit useless because we only know which function is crashing but not which line of code.

Godbolt example of the optimization: https://godbolt.org/z/6qjax9z1b

This Diff

I wanted to provide a way to know exactly which LOC is responsible for the crash. What we do here is use the size of the basic block as an iterator to an immediate value for the ubsan trap.

[smeenai]

You should upload this with full context and add some test cases :)

[smeenai]

Thinking about this a bit more, should the trap not have an associated stack trace that can be symbolicated to tell you which line of code was crashing? If the issue is that multiple traps can get folded together, the nomerge attribute (D78659) could be useful.

[oskarwirga]

In D148654#4312478, @smeenai wrote:

Thinking about this a bit more, should the trap not have an associated stack trace that can be symbolicated to tell you which line of code was crashing? If the issue is that multiple traps can get folded together, the nomerge attribute (D78659) could be useful.

I tried adding the nomerge attribute to TrapCall but I still found the call being optimized to a single site :(

Edit: Added it incorrectly, seems to work fine!

[oskarwirga]

Update the diff to use the nomerge attribute

[oskarwirga]

Turns out the lowering of ubsantrap() to a single instruction resulted in binaries that did NOT have nonmerged traps, so this is going back to what we had before. I also added tests to show that the trap gets preserved.

[oskarwirga]

Rebase on trunk :)

[oskarwirga]

clang-format 😺

[oskarwirga]

CC: @nlopes @chandlerc @jgalenson

I have y'all added here because of your past work on BoundsSan, if you know of anyone else who may be able to provide review please tag them!

[smeenai]

The patch should be uploaded with full context to make review easier.

Adding another potential reviewer.

[oskarwirga]

Add full context

[nlopes]

llvm/lib/Transforms/Instrumentation/BoundsChecking.cpp

187–219

this seems like code duplication. This pass already has the single-trap flag to exactly control if you get a single trap BB or one per check for better debug info.

[oskarwirga]

llvm/lib/Transforms/Instrumentation/BoundsChecking.cpp

187–219

Unfortunately, even with the single trap flag it gets optimized out in later passes because the machine code emitted is the exact same

[smeenai]

llvm/lib/Transforms/Instrumentation/BoundsChecking.cpp

187–219

I believe we end up tail merging the trap instructions. A previous iteration of this patch attempted to use the nomerge attribute to directly avoid the tail merging, but that only works for function calls, not for the trap instruction ultimately emitted here.

[oskarwirga]

Jobs are now passing, CC: @nlopes @chandlerc @jgalenson for review :)

[oskarwirga]

ping :)

[vitalybuka]

clang/lib/CodeGen/CGExpr.cpp

3612–3633

looks like a lot of code duplication

3630

wouldn't be you issues solved with
TrapCall->setCannotMerge() here?

[oskarwirga]

clang/lib/CodeGen/CGExpr.cpp

3612–3633

Let me refactor this

3630

ubsantrap gets lowered to an instruction in MIR which then gets merged later. This was the only way I was able to create unique instruction per check.

[oskarwirga]

Refactor CodeGen code

[aeubanks]

clang/lib/CodeGen/CGExpr.cpp

3630

isn't that a nomerge bug that should get fixed instead?

[oskarwirga]

clang/lib/CodeGen/CGExpr.cpp

3630

I don't think so because nomerge is a function attribute and the ubsantrap intrinsic gets lowered as an instruction. Creating instruction attributes from lowered function attributes seems a bit involved to me.

[oskarwirga]

ping

[oskarwirga]

Properly refactor the code bc my last attempt was flawed -_-

[oskarwirga]

Fix clang crash and retest

[oskarwirga]

ping

[vitalybuka]

clang/lib/CodeGen/CGExpr.cpp

47

this file and BoundsChecking.cpp belong to different patches

56

this applies only to fsanitize=undefined, and does not apply to llvm level sanitizers, like msan, asan
we need better name: maybe ubsan-unique-traps

BTW do we want this as frontend flag?

3581

so here we have two problems?

1. OptimizationLevel > 0 clang creates only one TrapBB per check type
2. even if we create multiple bb here, branch-folder will merge them later

3597–3599

(TrapBB->getParent()->size() * 0x10000 + CheckHandlerID)

llvm/lib/Transforms/Instrumentation/BoundsChecking.cpp

187–219

branches of if (DebugTrapBB) condition has a lot of code duplication, can you try to imrove?

196

why Fn->size(), to make a counter?

202

can you please create a test where bounds-checking-single-trap=0 and setCannotMerge produce invalid result.

[oskarwirga]

clang/lib/CodeGen/CGExpr.cpp

47

Just to be clear on terms here, these changes should be two different commits?

56

Good point, as for being a frontend flag I don't feel strongly one way or the other, not sure how useful this is outside of my use-case.

3581

Yeah exactly, despite my best efforts to come up with an existing way to fix this issue, because the ubsantrap intrinsic ends up being an instruction in MIR, it loses any function attributes we tack on now.

3597–3599

Why * 0x10000 ?

llvm/lib/Transforms/Instrumentation/BoundsChecking.cpp

187–219

Yes, I will address all your comments in a new patch on github, thank you for your feedback I appreciate it :)

196

I was looking for a way to create a more or less unique value without creating a global iterator. It doesn't have to be this, I just thought that the fn->size would be increasing as checks go in so it would give a unique value per check.

202

Can do!

[vitalybuka]

I assume we moved to https://github.com/llvm/llvm-project/pull/65972

clang/lib/CodeGen/CGExpr.cpp

 #include "llvm/IR/MatrixBuilder.h"
 #include "llvm/Passes/OptimizationLevel.h"
 #include "llvm/Support/ConvertUTF.h"
 #include "llvm/Support/MathExtras.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/SaveAndRestore.h"
 #include "llvm/Support/xxhash.h"
 #include "llvm/Transforms/Utils/SanitizerStats.h"
 
 #include <optional>
 #include <string>
 
 using namespace clang;
 using namespace CodeGen;
 
+// Experiment to make sanitizers easier to debug
+static llvm::cl::opt<bool> ClSanitizeDebugDeoptimization(
+    "sanitizer-de-opt-traps", llvm::cl::Optional,
+    llvm::cl::desc("Deoptimize traps for sanitizers"), llvm::cl::init(false));
+
 //===--------------------------------------------------------------------===//
 //                        Miscellaneous Helper Methods
 //===--------------------------------------------------------------------===//
 
 /// CreateTempAlloca - This creates a alloca and inserts it into the entry
 /// block.
 Address CodeGenFunction::CreateTempAllocaWithoutCast(llvm::Type *Ty,
                                                      CharUnits Align,

 void CodeGenFunction::EmitTrapCheck(llvm::Value *Checked,
                                     SanitizerHandler CheckHandlerID) {
   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 ||
-      (CurCodeDecl && CurCodeDecl->hasAttr<OptimizeNoneAttr>())) {
+  if (!ClSanitizeDebugDeoptimization &&
+      CGM.getCodeGenOpts().OptimizationLevel && TrapBB &&
+      (!CurCodeDecl || !CurCodeDecl->hasAttr<OptimizeNoneAttr>())) {
+    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);
+  } else {
     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));
+    llvm::CallInst *TrapCall = Builder.CreateCall(
+        CGM.getIntrinsic(llvm::Intrinsic::ubsantrap),
+        llvm::ConstantInt::get(CGM.Int8Ty, ClSanitizeDebugDeoptimization
+                                               ? TrapBB->getParent()->size()
+                                               : CheckHandlerID));
 
     if (!CGM.getCodeGenOpts().TrapFuncName.empty()) {
       auto A = llvm::Attribute::get(getLLVMContext(), "trap-func-name",
                                     CGM.getCodeGenOpts().TrapFuncName);
       TrapCall->addFnAttr(A);
     }
     TrapCall->setDoesNotReturn();
     TrapCall->setDoesNotThrow();
     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);
   }
 
   EmitBlock(Cont);
 }
 
 llvm::CallInst *CodeGenFunction::EmitTrapCall(llvm::Intrinsic::ID IntrID) {
   llvm::CallInst *TrapCall =
       Builder.CreateCall(CGM.getIntrinsic(IntrID));
 
   if (!CGM.getCodeGenOpts().TrapFuncName.empty()) {
     auto A = llvm::Attribute::get(getLLVMContext(), "trap-func-name",
                                   CGM.getCodeGenOpts().TrapFuncName);
     TrapCall->addFnAttr(A);
   }
 
   return TrapCall;
 }
 
 Address CodeGenFunction::EmitArrayToPointerDecay(const Expr *E,
                                                  LValueBaseInfo *BaseInfo,
                                                  TBAAAccessInfo *TBAAInfo) {
   assert(E->getType()->isArrayType() &&
          "Array to pointer decay must have array source type!");
 
   // Expressions of array type can't be bitfields or vector elements.
   LValue LV = EmitLValue(E);
   Address Addr = LV.getAddress(*this);
 
   // If the array type was an incomplete type, we need to make sure
   // the decay ends up being the right type.
   llvm::Type *NewTy = ConvertType(E->getType());
   Addr = Addr.withElementType(NewTy);
 

clang/test/CodeGen/bounds-checking.c

 // RUN: %clang_cc1 -fsanitize=local-bounds -emit-llvm -triple x86_64-apple-darwin10 %s -o - | FileCheck %s
 // RUN: %clang_cc1 -fsanitize=array-bounds -O -fsanitize-trap=array-bounds -emit-llvm -triple x86_64-apple-darwin10 -DNO_DYNAMIC %s -o - | FileCheck %s
+// RUN: %clang_cc1 -fsanitize=local-bounds -fsanitize-trap=local-bounds -O3 -mllvm -bounds-checking-debug-trap -emit-llvm -triple x86_64-apple-darwin10 %s -o - | FileCheck %s --check-prefixes=NOOPTLOCAL
+// RUN: %clang_cc1 -fsanitize=array-bounds -fsanitize-trap=array-bounds -O3 -mllvm -sanitizer-de-opt-traps -emit-llvm -triple x86_64-apple-darwin10 %s -o - | FileCheck %s --check-prefixes=NOOPTARRAY
 //
 // REQUIRES: x86-registered-target
 
 // CHECK-LABEL: @f1
 double f1(int b, int i) {
   double a[b];
   // CHECK: call {{.*}} @llvm.{{(ubsan)?trap}}
   return a[i];

 }
 
 // CHECK-LABEL: @f7
 int f7(union U *u, int i) {
   // c is treated as a flexible array member.
   // CHECK-NOT: @llvm.ubsantrap
   return u->c[i];
 }
+
+char B[10];
+char B2[10];
+// CHECK-LABEL: @f8
+void f8(int i, int k) {
+  // NOOPTLOCAL: call void @llvm.ubsantrap(i8 3)
+  // NOOPTARRAY: call void @llvm.ubsantrap(i8 2)
+  B[i] = '\0';
+
+  // NOOPTLOCAL: call void @llvm.ubsantrap(i8 5)
+  // NOOPTARRAY: call void @llvm.ubsantrap(i8 4)
+  B2[k] = '\0';
+}

llvm/lib/Transforms/Instrumentation/BoundsChecking.cpp

 
 using namespace llvm;
 
 #define DEBUG_TYPE "bounds-checking"
 
 static cl::opt<bool> SingleTrapBB("bounds-checking-single-trap",
                                   cl::desc("Use one trap block per function"));
 
+static cl::opt<bool>
+    DebugTrapBB("bounds-checking-debug-trap",
+                cl::desc("Use one trap block per check despite optimizations"));
+
 STATISTIC(ChecksAdded, "Bounds checks added");
 STATISTIC(ChecksSkipped, "Bounds checks skipped");
 STATISTIC(ChecksUnable, "Bounds checks unable to add");
 
 using BuilderTy = IRBuilder<TargetFolder>;
 
 /// Gets the conditions under which memory accessing instructions will overflow.
 ///
if (Or)
       TrapInfo.push_back(std::make_pair(&I, Or));
   }
 
   // Create a trapping basic block on demand using a callback. Depending on
   // flags, this will either create a single block for the entire function or
   // will create a fresh block every time it is called.
   BasicBlock *TrapBB = nullptr;
   auto GetTrapBB = [&TrapBB](BuilderTy &IRB) {
+    if (DebugTrapBB) {
+      Function *Fn = IRB.GetInsertBlock()->getParent();
+      auto DebugLoc = IRB.getCurrentDebugLocation();
+      IRBuilder<>::InsertPointGuard Guard(IRB);
+      TrapBB = BasicBlock::Create(Fn->getContext(), "trap", Fn);
+      IRB.SetInsertPoint(TrapBB);
+      auto *F =
+          Intrinsic::getDeclaration(Fn->getParent(), Intrinsic::ubsantrap);
+      CallInst *TrapCall =
+          IRB.CreateCall(F, ConstantInt::get(IRB.getInt8Ty(), Fn->size()));
+      TrapCall->setDoesNotReturn();
+      TrapCall->setDoesNotThrow();
+      TrapCall->setDebugLoc(DebugLoc);
+      IRB.CreateUnreachable();
+    } else {
       if (TrapBB && SingleTrapBB)
         return TrapBB;
 
       Function *Fn = IRB.GetInsertBlock()->getParent();
       // FIXME: This debug location doesn't make a lot of sense in the
       // `SingleTrapBB` case.
       auto DebugLoc = IRB.getCurrentDebugLocation();
       IRBuilder<>::InsertPointGuard Guard(IRB);
       TrapBB = BasicBlock::Create(Fn->getContext(), "trap", Fn);
       IRB.SetInsertPoint(TrapBB);
 
       auto *F = Intrinsic::getDeclaration(Fn->getParent(), Intrinsic::trap);
       CallInst *TrapCall = IRB.CreateCall(F, {});
       TrapCall->setDoesNotReturn();
       TrapCall->setDoesNotThrow();
       TrapCall->setDebugLoc(DebugLoc);
       IRB.CreateUnreachable();
-
+    }
     return TrapBB;
   };
 
   // Add the checks.
   for (const auto &Entry : TrapInfo) {
     Instruction *Inst = Entry.first;
     BuilderTy IRB(Inst->getParent(), BasicBlock::iterator(Inst), TargetFolder(DL));
     insertBoundsCheck(Entry.second, IRB, GetTrapBB);