[PATCH] D148654: Modify BoundsSan to improve debuggability

[Oskar Wirga via Phabricator via cfe-commits]

oskarwirga added inline comments.



Comment at: clang/lib/CodeGen/CGExpr.cpp:47
 #include "llvm/Transforms/Utils/SanitizerStats.h"
 
 #include 

vitalybuka wrote:
> this file and BoundsChecking.cpp belong to different patches
Just to be clear on terms here, these changes should be two different commits?



Comment at: clang/lib/CodeGen/CGExpr.cpp:56
+static llvm::cl::opt ClSanitizeDebugDeoptimization(
+"sanitizer-de-opt-traps", llvm::cl::Optional,
+llvm::cl::desc("Deoptimize traps for sanitizers"), llvm::cl::init(false));

vitalybuka wrote:
> 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?
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. 



Comment at: clang/lib/CodeGen/CGExpr.cpp:3581
 
-  if (!CGM.getCodeGenOpts().OptimizationLevel || !TrapBB ||
-  (CurCodeDecl && CurCodeDecl->hasAttr())) {
+  if (!ClSanitizeDebugDeoptimization &&
+  CGM.getCodeGenOpts().OptimizationLevel && TrapBB &&

vitalybuka wrote:
> 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
> 
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. 



Comment at: clang/lib/CodeGen/CGExpr.cpp:3597-3599
+llvm::ConstantInt::get(CGM.Int8Ty, ClSanitizeDebugDeoptimization
+   ? TrapBB->getParent()->size()
+   : CheckHandlerID));

vitalybuka wrote:
>   (TrapBB->getParent()->size() * 
> 0x1 + CheckHandlerID)
> 
Why `* 0x1` ?



Comment at: llvm/lib/Transforms/Instrumentation/BoundsChecking.cpp:196
+  CallInst *TrapCall =
+  IRB.CreateCall(F, ConstantInt::get(IRB.getInt8Ty(), Fn->size()));
+  TrapCall->setDoesNotReturn();

vitalybuka wrote:
> why Fn->size(), to make a counter?
> 
> 
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. 



Comment at: llvm/lib/Transforms/Instrumentation/BoundsChecking.cpp:202
+} else {
+  if (TrapBB && SingleTrapBB)
+return TrapBB;

vitalybuka wrote:
> can you please create a test where bounds-checking-single-trap=0 and 
> setCannotMerge produce invalid result.
Can do!



Comment at: llvm/lib/Transforms/Instrumentation/BoundsChecking.cpp:189
   auto GetTrapBB = [](BuilderTy ) {
-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();
-
+if (DebugTrapBB) {
+  Function *Fn = IRB.GetInsertBlock()->getParent();

vitalybuka wrote:
> smeenai wrote:
> > oskarwirga wrote:
> > > nlopes wrote:
> > > > 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.
> > > Unfortunately, even with the single trap flag it gets optimized out in 
> > > later passes because the machine code emitted is the exact same 
> > 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.
> branches of `if (DebugTrapBB) ` condition has a lot of code duplication, can 
> you try to imrove?
Yes, I will address all your comments in a new patch on github, thank you for 
your feedback I appreciate it :) 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148654/new/

https://reviews.llvm.org/D148654

___

[PATCH] D148654: Modify BoundsSan to improve debuggability

[Oskar Wirga via Phabricator via cfe-commits]

oskarwirga added a comment.

ping


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148654/new/

https://reviews.llvm.org/D148654

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148654: Modify BoundsSan to improve debuggability

[Oskar Wirga via Phabricator via cfe-commits]

oskarwirga updated this revision to Diff 550355.
oskarwirga added a comment.

Fix clang crash and retest


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148654/new/

https://reviews.llvm.org/D148654

Files:
  clang/lib/CodeGen/CGExpr.cpp
  clang/test/CodeGen/bounds-checking.c
  llvm/lib/Transforms/Instrumentation/BoundsChecking.cpp

Index: llvm/lib/Transforms/Instrumentation/BoundsChecking.cpp
===
--- llvm/lib/Transforms/Instrumentation/BoundsChecking.cpp
+++ llvm/lib/Transforms/Instrumentation/BoundsChecking.cpp
@@ -37,6 +37,10 @@
 static cl::opt SingleTrapBB("bounds-checking-single-trap",
   cl::desc("Use one trap block per function"));
 
+static cl::opt
+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");
@@ -180,24 +184,39 @@
   // will create a fresh block every time it is called.
   BasicBlock *TrapBB = nullptr;
   auto GetTrapBB = [](BuilderTy ) {
-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();
-
+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;
   };
 
Index: clang/test/CodeGen/bounds-checking.c
===
--- clang/test/CodeGen/bounds-checking.c
+++ clang/test/CodeGen/bounds-checking.c
@@ -1,5 +1,7 @@
 // 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
 
@@ -66,3 +68,16 @@
   // 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';
+}
Index: clang/lib/CodeGen/CGExpr.cpp
===
--- clang/lib/CodeGen/CGExpr.cpp
+++ clang/lib/CodeGen/CGExpr.cpp
@@ -51,6 +51,11 @@
 using namespace clang;
 using namespace CodeGen;
 
+// Experiment to make sanitizers easier to debug
+static llvm::cl::opt ClSanitizeDebugDeoptimization(
+"sanitizer-de-opt-traps", llvm::cl::Optional,
+  

[PATCH] D148654: Modify BoundsSan to improve debuggability

[Oskar Wirga via Phabricator via cfe-commits]

oskarwirga updated this revision to Diff 543557.
oskarwirga added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148654/new/

https://reviews.llvm.org/D148654

Files:
  clang/lib/CodeGen/CGExpr.cpp
  clang/test/CodeGen/bounds-checking.c
  llvm/lib/Transforms/Instrumentation/BoundsChecking.cpp

Index: llvm/lib/Transforms/Instrumentation/BoundsChecking.cpp
===
--- llvm/lib/Transforms/Instrumentation/BoundsChecking.cpp
+++ llvm/lib/Transforms/Instrumentation/BoundsChecking.cpp
@@ -37,6 +37,10 @@
 static cl::opt SingleTrapBB("bounds-checking-single-trap",
   cl::desc("Use one trap block per function"));
 
+static cl::opt
+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");
@@ -180,24 +184,39 @@
   // will create a fresh block every time it is called.
   BasicBlock *TrapBB = nullptr;
   auto GetTrapBB = [](BuilderTy ) {
-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();
-
+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;
   };
 
Index: clang/test/CodeGen/bounds-checking.c
===
--- clang/test/CodeGen/bounds-checking.c
+++ clang/test/CodeGen/bounds-checking.c
@@ -1,5 +1,7 @@
 // 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
 
@@ -66,3 +68,16 @@
   // 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';
+}
Index: clang/lib/CodeGen/CGExpr.cpp
===
--- clang/lib/CodeGen/CGExpr.cpp
+++ clang/lib/CodeGen/CGExpr.cpp
@@ -51,6 +51,11 @@
 using namespace clang;
 using namespace CodeGen;
 
+// Experiment to make sanitizers easier to debug
+static llvm::cl::opt ClSanitizeDebugDeoptimization(
+

[PATCH] D148654: Modify BoundsSan to improve debuggability

[Oskar Wirga via Phabricator via cfe-commits]

oskarwirga added a comment.

ping


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148654/new/

https://reviews.llvm.org/D148654

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148654: Modify BoundsSan to improve debuggability

[Oskar Wirga via Phabricator via cfe-commits]

oskarwirga added inline comments.



Comment at: clang/lib/CodeGen/CGExpr.cpp:3594
+TrapCall->addFnAttr(A);
+  }
+  TrapCall->setDoesNotReturn();

aeubanks wrote:
> oskarwirga wrote:
> > vitalybuka wrote:
> > > wouldn't be you issues solved with
> > > TrapCall->setCannotMerge() here?
> > 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. 
> isn't that a `nomerge` bug that should get fixed instead?
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. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148654/new/

https://reviews.llvm.org/D148654

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148654: Modify BoundsSan to improve debuggability

[Oskar Wirga via Phabricator via cfe-commits]

oskarwirga updated this revision to Diff 540181.
oskarwirga added a comment.

Refactor CodeGen code


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148654/new/

https://reviews.llvm.org/D148654

Files:
  clang/lib/CodeGen/CGExpr.cpp
  clang/test/CodeGen/bounds-checking.c
  llvm/lib/Transforms/Instrumentation/BoundsChecking.cpp

Index: llvm/lib/Transforms/Instrumentation/BoundsChecking.cpp
===
--- llvm/lib/Transforms/Instrumentation/BoundsChecking.cpp
+++ llvm/lib/Transforms/Instrumentation/BoundsChecking.cpp
@@ -37,6 +37,10 @@
 static cl::opt SingleTrapBB("bounds-checking-single-trap",
   cl::desc("Use one trap block per function"));
 
+static cl::opt
+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");
@@ -180,24 +184,39 @@
   // will create a fresh block every time it is called.
   BasicBlock *TrapBB = nullptr;
   auto GetTrapBB = [](BuilderTy ) {
-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();
-
+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;
   };
 
Index: clang/test/CodeGen/bounds-checking.c
===
--- clang/test/CodeGen/bounds-checking.c
+++ clang/test/CodeGen/bounds-checking.c
@@ -1,5 +1,7 @@
 // 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
 
@@ -66,3 +68,16 @@
   // 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';
+}
Index: clang/lib/CodeGen/CGExpr.cpp
===
--- clang/lib/CodeGen/CGExpr.cpp
+++ clang/lib/CodeGen/CGExpr.cpp
@@ -51,6 +51,11 @@
 using namespace clang;
 using namespace CodeGen;
 
+// Experiment to make sanitizers easier to debug
+static llvm::cl::opt ClSanitizeDebugDeoptimization(
+"sanitizer-de-opt-traps", llvm::cl::Optional,
+

[PATCH] D148654: Modify BoundsSan to improve debuggability

[Oskar Wirga via Phabricator via cfe-commits]

oskarwirga added inline comments.



Comment at: clang/lib/CodeGen/CGExpr.cpp:3576-3597
+if (TrapBBs.size() <= CheckHandlerID)
+  TrapBBs.resize(CheckHandlerID + 1);
+llvm::BasicBlock * = TrapBBs[CheckHandlerID];
+
+if (!CGM.getCodeGenOpts().OptimizationLevel || !TrapBB ||
+(CurCodeDecl && CurCodeDecl->hasAttr())) {
+  TrapBB = createBasicBlock("trap");

vitalybuka wrote:
> looks like a lot of code duplication
Let me refactor this 



Comment at: clang/lib/CodeGen/CGExpr.cpp:3594
+TrapCall->addFnAttr(A);
+  }
+  TrapCall->setDoesNotReturn();

vitalybuka wrote:
> wouldn't be you issues solved with
> TrapCall->setCannotMerge() here?
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. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148654/new/

https://reviews.llvm.org/D148654

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148654: Modify BoundsSan to improve debuggability

[Oskar Wirga via Phabricator via cfe-commits]

oskarwirga added a comment.

ping :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148654/new/

https://reviews.llvm.org/D148654

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148654: Modify BoundsSan to improve debuggability

[Oskar Wirga via Phabricator via cfe-commits]

oskarwirga added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148654/new/

https://reviews.llvm.org/D148654

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148654: Modify BoundsSan to improve debuggability

[Oskar Wirga via Phabricator via cfe-commits]

oskarwirga added inline comments.



Comment at: llvm/lib/Transforms/Instrumentation/BoundsChecking.cpp:189
   auto GetTrapBB = [](BuilderTy ) {
-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();
-
+if (DebugTrapBB) {
+  Function *Fn = IRB.GetInsertBlock()->getParent();

nlopes wrote:
> 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.
Unfortunately, even with the single trap flag it gets optimized out in later 
passes because the machine code emitted is the exact same 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148654/new/

https://reviews.llvm.org/D148654

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148654: Modify BoundsSan to improve debuggability

[Oskar Wirga via Phabricator via cfe-commits]

oskarwirga updated this revision to Diff 521767.
oskarwirga added a comment.

Add full context


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148654/new/

https://reviews.llvm.org/D148654

Files:
  clang/lib/CodeGen/CGExpr.cpp
  clang/test/CodeGen/bounds-checking.c
  llvm/lib/Transforms/Instrumentation/BoundsChecking.cpp

Index: llvm/lib/Transforms/Instrumentation/BoundsChecking.cpp
===
--- llvm/lib/Transforms/Instrumentation/BoundsChecking.cpp
+++ llvm/lib/Transforms/Instrumentation/BoundsChecking.cpp
@@ -39,6 +39,10 @@
 static cl::opt SingleTrapBB("bounds-checking-single-trap",
   cl::desc("Use one trap block per function"));
 
+static cl::opt
+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");
@@ -182,24 +186,39 @@
   // will create a fresh block every time it is called.
   BasicBlock *TrapBB = nullptr;
   auto GetTrapBB = [](BuilderTy ) {
-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();
-
+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;
   };
 
Index: clang/test/CodeGen/bounds-checking.c
===
--- clang/test/CodeGen/bounds-checking.c
+++ clang/test/CodeGen/bounds-checking.c
@@ -1,5 +1,7 @@
 // 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
 
@@ -66,3 +68,16 @@
   // 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';
+}
Index: clang/lib/CodeGen/CGExpr.cpp
===
--- clang/lib/CodeGen/CGExpr.cpp
+++ clang/lib/CodeGen/CGExpr.cpp
@@ -48,6 +48,11 @@
 using namespace clang;
 using namespace CodeGen;
 
+// Experiment to make sanitizers easier to debug
+static llvm::cl::opt ClSanitizeDebugDeoptimization(
+"sanitizer-de-opt-traps", llvm::cl::Optional,
+

[PATCH] D148654: Modify BoundsSan to improve debuggability

[Oskar Wirga via Phabricator via cfe-commits]

oskarwirga added a comment.

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!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148654/new/

https://reviews.llvm.org/D148654

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148654: Modify BoundsSan to improve debuggability

[Oskar Wirga via Phabricator via cfe-commits]

oskarwirga updated this revision to Diff 521525.
oskarwirga added a comment.

clang-format 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148654/new/

https://reviews.llvm.org/D148654

Files:
  clang/lib/CodeGen/CGExpr.cpp
  clang/test/CodeGen/bounds-checking.c
  llvm/lib/Transforms/Instrumentation/BoundsChecking.cpp

Index: llvm/lib/Transforms/Instrumentation/BoundsChecking.cpp
===
--- llvm/lib/Transforms/Instrumentation/BoundsChecking.cpp
+++ llvm/lib/Transforms/Instrumentation/BoundsChecking.cpp
@@ -39,6 +39,10 @@
 static cl::opt SingleTrapBB("bounds-checking-single-trap",
   cl::desc("Use one trap block per function"));
 
+static cl::opt
+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");
@@ -182,24 +186,39 @@
   // will create a fresh block every time it is called.
   BasicBlock *TrapBB = nullptr;
   auto GetTrapBB = [](BuilderTy ) {
-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();
-
+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;
   };
 
Index: clang/test/CodeGen/bounds-checking.c
===
--- clang/test/CodeGen/bounds-checking.c
+++ clang/test/CodeGen/bounds-checking.c
@@ -1,5 +1,7 @@
 // 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
 
@@ -66,3 +68,16 @@
   // 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';
+}
Index: clang/lib/CodeGen/CGExpr.cpp
===
--- clang/lib/CodeGen/CGExpr.cpp
+++ clang/lib/CodeGen/CGExpr.cpp
@@ -48,6 +48,11 @@
 using namespace clang;
 using namespace CodeGen;
 
+// Experiment to make sanitizers easier to debug
+static llvm::cl::opt ClSanitizeDebugDeoptimization(
+"sanitizer-de-opt-traps", llvm::cl::Optional,
+

[PATCH] D148654: Modify BoundsSan to improve debuggability

[Oskar Wirga via Phabricator via cfe-commits]

oskarwirga updated this revision to Diff 521180.
oskarwirga added a comment.

Rebase on trunk :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148654/new/

https://reviews.llvm.org/D148654

Files:
  clang/lib/CodeGen/CGExpr.cpp
  clang/test/CodeGen/bounds-checking.c
  llvm/lib/Transforms/Instrumentation/BoundsChecking.cpp

Index: llvm/lib/Transforms/Instrumentation/BoundsChecking.cpp
===
--- llvm/lib/Transforms/Instrumentation/BoundsChecking.cpp
+++ llvm/lib/Transforms/Instrumentation/BoundsChecking.cpp
@@ -37,6 +37,10 @@
 static cl::opt SingleTrapBB("bounds-checking-single-trap",
   cl::desc("Use one trap block per function"));
 
+static cl::opt
+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");
@@ -180,24 +184,39 @@
   // will create a fresh block every time it is called.
   BasicBlock *TrapBB = nullptr;
   auto GetTrapBB = [](BuilderTy ) {
-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();
-
+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;
   };
 
Index: clang/test/CodeGen/bounds-checking.c
===
--- clang/test/CodeGen/bounds-checking.c
+++ clang/test/CodeGen/bounds-checking.c
@@ -1,5 +1,7 @@
 // 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
 
@@ -66,3 +68,16 @@
   // 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';
+}
Index: clang/lib/CodeGen/CGExpr.cpp
===
--- clang/lib/CodeGen/CGExpr.cpp
+++ clang/lib/CodeGen/CGExpr.cpp
@@ -48,6 +48,11 @@
 using namespace clang;
 using namespace CodeGen;
 
+// Experiment to make sanitizers easier to debug
+static llvm::cl::opt ClSanitizeDebugDeoptimization(
+"sanitizer-de-opt-traps", llvm::cl::Optional,
+

[PATCH] D148654: Modify BoundsSan to improve debuggability

[Oskar Wirga via Phabricator via cfe-commits]

oskarwirga updated this revision to Diff 519190.
oskarwirga edited the summary of this revision.
oskarwirga added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148654/new/

https://reviews.llvm.org/D148654

Files:
  clang/lib/CodeGen/CGExpr.cpp
  clang/test/CodeGen/bounds-checking.c
  llvm/lib/Transforms/Instrumentation/BoundsChecking.cpp

Index: clang/lib/CodeGen/CGExpr.cpp
===
--- clang/lib/CodeGen/CGExpr.cpp
+++ clang/lib/CodeGen/CGExpr.cpp
@@ -47,6 +47,11 @@
 using namespace clang;
 using namespace CodeGen;
 
+// Experiment to make sanitizers easier to debug
+static llvm::cl::opt ClSanitizeDebugDeoptimization(
+"sanitizer-de-opt-traps", llvm::cl::Optional,
+llvm::cl::desc("Deoptimize traps for sanitizers"), llvm::cl::init(false));
+
 //======//
 //Miscellaneous Helper Methods
 //======//
@@ -3568,22 +3573,16 @@
 
 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 * = TrapBBs[CheckHandlerID];
+  if (ClSanitizeDebugDeoptimization) {
+llvm::BasicBlock *Cont = createBasicBlock("cont");
 
-  if (!CGM.getCodeGenOpts().OptimizationLevel || !TrapBB) {
-TrapBB = createBasicBlock("trap");
+llvm::BasicBlock *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, TrapBB->getParent()->size()));
 
 if (!CGM.getCodeGenOpts().TrapFuncName.empty()) {
   auto A = llvm::Attribute::get(getLLVMContext(), "trap-func-name",
@@ -3593,16 +3592,45 @@
 TrapCall->setDoesNotReturn();
 TrapCall->setDoesNotThrow();
 Builder.CreateUnreachable();
+
+EmitBlock(Cont);
   } else {
-auto Call = TrapBB->begin();
-assert(isa(Call) && "Expected call in trap BB");
+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 * = TrapBBs[CheckHandlerID];
+
+if (!CGM.getCodeGenOpts().OptimizationLevel || !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->addFnAttr(A);
+  }
+  TrapCall->setDoesNotReturn();
+  TrapCall->setDoesNotThrow();
+  Builder.CreateUnreachable();
+} else {
+  auto Call = TrapBB->begin();
+  assert(isa(Call) && "Expected call in trap BB");
 
-Call->applyMergedLocation(Call->getDebugLoc(),
-  Builder.getCurrentDebugLocation());
-Builder.CreateCondBr(Checked, Cont, TrapBB);
-  }
+  Call->applyMergedLocation(Call->getDebugLoc(),
+Builder.getCurrentDebugLocation());
+  Builder.CreateCondBr(Checked, Cont, TrapBB);
+}
 
-  EmitBlock(Cont);
+EmitBlock(Cont);
+  }
 }
 
 llvm::CallInst *CodeGenFunction::EmitTrapCall(llvm::Intrinsic::ID IntrID) {
Index: clang/test/CodeGen/bounds-checking.c
===
--- clang/test/CodeGen/bounds-checking.c
+++ clang/test/CodeGen/bounds-checking.c
@@ -1,5 +1,7 @@
 // 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 

[PATCH] D148654: Modify BoundsSan to improve debuggability

[Oskar Wirga via Phabricator via cfe-commits]

oskarwirga updated this revision to Diff 518933.
oskarwirga edited the summary of this revision.
oskarwirga added a comment.

Update the diff to use the `nomerge` attribute


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148654/new/

https://reviews.llvm.org/D148654

Files:
  clang/lib/CodeGen/CGExpr.cpp
  llvm/lib/Transforms/Instrumentation/BoundsChecking.cpp


Index: clang/lib/CodeGen/CGExpr.cpp
===
--- clang/lib/CodeGen/CGExpr.cpp
+++ clang/lib/CodeGen/CGExpr.cpp
@@ -47,6 +47,11 @@
 using namespace clang;
 using namespace CodeGen;
 
+// Experiment to make sanitizers easier to debug
+static llvm::cl::opt ClSanitizeDebugDeoptimization(
+"sanitizer-de-opt-traps", llvm::cl::Optional,
+llvm::cl::desc("Deoptimize traps for sanitizers"), llvm::cl::init(false));
+
 //======//
 //Miscellaneous Helper Methods
 //======//
@@ -3576,7 +3581,8 @@
 TrapBBs.resize(CheckHandlerID + 1);
   llvm::BasicBlock * = TrapBBs[CheckHandlerID];
 
-  if (!CGM.getCodeGenOpts().OptimizationLevel || !TrapBB) {
+  if (ClSanitizeDebugDeoptimization ||
+  !CGM.getCodeGenOpts().OptimizationLevel || !TrapBB) {
 TrapBB = createBasicBlock("trap");
 Builder.CreateCondBr(Checked, Cont, TrapBB);
 EmitBlock(TrapBB);
@@ -3590,6 +3596,8 @@
 CGM.getCodeGenOpts().TrapFuncName);
   TrapCall->addFnAttr(A);
 }
+if (ClSanitizeDebugDeoptimization)
+  TrapCall->addFnAttr(llvm::Attribute::NoMerge);
 TrapCall->setDoesNotReturn();
 TrapCall->setDoesNotThrow();
 Builder.CreateUnreachable();
Index: llvm/lib/Transforms/Instrumentation/BoundsChecking.cpp
===
--- llvm/lib/Transforms/Instrumentation/BoundsChecking.cpp
+++ llvm/lib/Transforms/Instrumentation/BoundsChecking.cpp
@@ -39,6 +39,11 @@
 static cl::opt SingleTrapBB("bounds-checking-single-trap",
   cl::desc("Use one trap block per function"));
 
+// Experiment to make sanitizers easier to debug
+static llvm::cl::opt ClBoundsSanitizeDebugDeoptimization(
+"bounds-de-opt-traps", llvm::cl::Optional,
+llvm::cl::desc("Deoptimize traps for sanitizers"), llvm::cl::init(false));
+
 STATISTIC(ChecksAdded, "Bounds checks added");
 STATISTIC(ChecksSkipped, "Bounds checks skipped");
 STATISTIC(ChecksUnable, "Bounds checks unable to add");
@@ -181,7 +186,7 @@
   // will create a fresh block every time it is called.
   BasicBlock *TrapBB = nullptr;
   auto GetTrapBB = [](BuilderTy ) {
-if (TrapBB && SingleTrapBB)
+if (!ClBoundsSanitizeDebugDeoptimization && TrapBB && SingleTrapBB)
   return TrapBB;
 
 Function *Fn = IRB.GetInsertBlock()->getParent();
@@ -194,6 +199,8 @@
 
 auto *F = Intrinsic::getDeclaration(Fn->getParent(), Intrinsic::trap);
 CallInst *TrapCall = IRB.CreateCall(F, {});
+if (ClBoundsSanitizeDebugDeoptimization)
+  TrapCall->addFnAttr(llvm::Attribute::NoMerge);
 TrapCall->setDoesNotReturn();
 TrapCall->setDoesNotThrow();
 TrapCall->setDebugLoc(DebugLoc);


Index: clang/lib/CodeGen/CGExpr.cpp
===
--- clang/lib/CodeGen/CGExpr.cpp
+++ clang/lib/CodeGen/CGExpr.cpp
@@ -47,6 +47,11 @@
 using namespace clang;
 using namespace CodeGen;
 
+// Experiment to make sanitizers easier to debug
+static llvm::cl::opt ClSanitizeDebugDeoptimization(
+"sanitizer-de-opt-traps", llvm::cl::Optional,
+llvm::cl::desc("Deoptimize traps for sanitizers"), llvm::cl::init(false));
+
 //======//
 //Miscellaneous Helper Methods
 //======//
@@ -3576,7 +3581,8 @@
 TrapBBs.resize(CheckHandlerID + 1);
   llvm::BasicBlock * = TrapBBs[CheckHandlerID];
 
-  if (!CGM.getCodeGenOpts().OptimizationLevel || !TrapBB) {
+  if (ClSanitizeDebugDeoptimization ||
+  !CGM.getCodeGenOpts().OptimizationLevel || !TrapBB) {
 TrapBB = createBasicBlock("trap");
 Builder.CreateCondBr(Checked, Cont, TrapBB);
 EmitBlock(TrapBB);
@@ -3590,6 +3596,8 @@
 CGM.getCodeGenOpts().TrapFuncName);
   TrapCall->addFnAttr(A);
 }
+if (ClSanitizeDebugDeoptimization)
+  TrapCall->addFnAttr(llvm::Attribute::NoMerge);
 TrapCall->setDoesNotReturn();
 TrapCall->setDoesNotThrow();
 Builder.CreateUnreachable();
Index: llvm/lib/Transforms/Instrumentation/BoundsChecking.cpp
===
--- llvm/lib/Transforms/Instrumentation/BoundsChecking.cpp
+++ llvm/lib/Transforms/Instrumentation/BoundsChecking.cpp
@@ -39,6 +39,11 @@
 static cl::opt 

[PATCH] D148654: Modify BoundsSan to improve debuggability

[Oskar Wirga via Phabricator via cfe-commits]

oskarwirga added a comment.

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 :(


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148654/new/

https://reviews.llvm.org/D148654

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits