fmayer marked an inline comment as done and an inline comment as not done. fmayer added a comment.
Addressed inline comments. ================ Comment at: clang/test/CodeGen/hwasan-stack-safety-analysis-asm.c:4 + +int main(int argc, char **argv) { + char buf[10]; ---------------- vitalybuka wrote: > this patch mostly change code under llvm/ so tests should be also there, as > IR tests > > I don't have strong feelings, but clang/test/CodeGen/lifetime-sanitizer.c is a very similar test, so I think we should either move all of these to llvm/ or add the new ones here to clang/. What do you think? ================ Comment at: llvm/include/llvm/Transforms/Instrumentation/HWAddressSanitizer.h:32 + Triple TargetTriple = {}); PreservedAnalyses run(Module &M, ModuleAnalysisManager &MAM); static bool isRequired() { return true; } ---------------- vitalybuka wrote: > Why not from M.getTargetTriple() ? Mostly for consistency with the legacy pass. Either way is fine for me though, what do you prefer? ================ Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:390 + void getAnalysisUsage(AnalysisUsage &AU) const override { + if (shouldUseStackSafetyAnalysis(TargetTriple)) { + AU.addRequired<StackSafetyGlobalInfoWrapperPass>(); ---------------- vitalybuka wrote: > why we need to check TargetTriple for that? Because we only need the stack safety analysis if we instrument the stack, which we do not do on x86_64 (see shouldInstrumentStack). ================ Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:1275 bool HWAddressSanitizer::isInterestingAlloca(const AllocaInst &AI) { + // clang-format off return (AI.getAllocatedType()->isSized() && ---------------- vitalybuka wrote: > Instead of // clang-format off > could you replace this with > > ``` > # comment1 > if (...) > return false; > > # comment2 > if (...) > return false; > ... > > ``` > > in a separate patch? OK, will do in a followup (or see how I can get clang-tidy to do the right thing here). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105703/new/ https://reviews.llvm.org/D105703 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits