[PATCH] D148654: Modify BoundsSan to improve debuggability

[Vitaly Buka via Phabricator via cfe-commits]

vitalybuka added inline comments.


================
Comment at: clang/lib/CodeGen/CGExpr.cpp:47
 #include "llvm/Transforms/Utils/SanitizerStats.h"
 
 #include <optional>
----------------
this file and BoundsChecking.cpp belong to different patches


================
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));
----------------
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?


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



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



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




================
Comment at: llvm/lib/Transforms/Instrumentation/BoundsChecking.cpp:202
+    } else {
+      if (TrapBB && SingleTrapBB)
+        return TrapBB;
----------------
can you please create a test where bounds-checking-single-trap=0 and setCannotMerge produce invalid result.


================
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();
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148654