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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits