[PATCH] D105201: [hwasan] Detect use after scope within function.

[Florian Mayer via Phabricator via llvm-commits]

fmayer added inline comments.


================
Comment at: llvm/include/llvm/Transforms/Instrumentation/AddressSanitizerCommon.h:52
+template <typename F>
+void tagLifetimeEnd(DominatorTree *DT, PostDominatorTree *PDT,
+                    IntrinsicInst *Start, IntrinsicInst *End,
----------------
eugenis wrote:
> tagLifetimeEnd is not descriptive enough for namespace llvm. M/b something like ForAllReachableExits? Start and End can be just Instruction *, and RetVec should be const. Instead of erasing End I'd rather return a bool that's true if any callbacks were invoked on any of the RetVec, and caller can remove lifetime.end then.
> 
> This refactoring is better done in a separate change.
> This refactoring is better done in a separate change.

Confirming that you mean I should split this change and pull this helper function out before?


================
Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:124
+                    cl::desc("detect use after scope within function"),
+                    cl::Hidden, cl::init(false));
+
----------------
eugenis wrote:
> Probably should be on by default.
Wouldn't it be prudent to leave it behind a disabled flag until we get some more experience, and then make it opt-out?


================
Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:390
   void getAnalysisUsage(AnalysisUsage &AU) const override {
-    AU.setPreservesCFG();
     if (shouldUseStackSafetyAnalysis(TargetTriple)) {
----------------
eugenis wrote:
> why?
I uploaded a new version of the stack safety change (where I had accidentally introduced this), and then forgot to upload the rebased change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105201