[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 <optional>
----------------
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<bool> 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<OptimizeNoneAttr>())) {
+  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() * 0x10000 + CheckHandlerID)
> 
Why `* 0x10000` ?


================
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 = [&TrapBB](BuilderTy &IRB) {
-    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