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

[Vitaly Buka via Phabricator via llvm-commits]

vitalybuka added inline comments.


================
Comment at: compiler-rt/test/hwasan/TestCases/stack-uas.c:10
+
+// RUN: %clang_hwasan -fexperimental-new-pass-manager -mllvm -hwasan-use-after-scope -g %s -o %t && not %run %t 2>&1 | FileCheck %s
+// RUN: %clang_hwasan -fno-experimental-new-pass-manager -mllvm -hwasan-use-after-scope -g %s -o %t && not %run %t 2>&1 | FileCheck %s
----------------
It looks all these test ported version of asan
it would be nice if in a separate patch you commit them as exact copy (with XFAIL: *)

so in D105201 we can see only change you need to make for HWASAN


================
Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:421
+    PostDominatorTree *PDT = nullptr;
+    if (shouldDetectUseAfterScope(TargetTriple)) {
+      if (auto *P = getAnalysisIfAvailable<DominatorTreeWrapperPass>())
----------------
do we save anything with this if(shouldDetectUseAfterScope)?


================
Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:495
+      // functions that have interesting allocas.
+      DT = FAM.getCachedResult<DominatorTreeAnalysis>(F);
+      PDT = FAM.getCachedResult<PostDominatorTreeAnalysis>(F);
----------------
same here, if it's cached why do we need to avoid forwarding them even if we don't use them later


================
Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:1286-1291
 bool HWAddressSanitizer::instrumentStack(
-    SmallVectorImpl<AllocaInst *> &Allocas,
+    MapVector<AllocaInst *, AllocaInfo> &AllocasToInstrument,
+    SmallVector<Instruction *, 4> &UnrecognizedLifetimes,
     DenseMap<AllocaInst *, std::vector<DbgVariableIntrinsic *>> &AllocaDbgMap,
-    SmallVectorImpl<Instruction *> &RetVec, Value *StackTag) {
+    SmallVectorImpl<Instruction *> &RetVec, Value *StackTag, DominatorTree *DT,
+    PostDominatorTree *PDT) {
----------------
only if you wish to that later, but it would be nice to refactor HWAddressSanitizer into module and function classes
so we can have all these arguments as member of function one




================
Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:1299-1300
+
+  for (auto It = AllocasToInstrument.begin(); It < AllocasToInstrument.end();
+       ++It, ++N) {
+    auto *AI = It->first;
----------------
Range-based looks better to me
```
  for (auto &KV : AllocasToInstrument) {
    auto *AI = KV.first;

    Value *Tag = getAllocaTag(IRB, StackTag, AI, N++);
```


================
Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:1350
+      }
+      if (!StandardLifetime) {
+        for (auto &II : Info.LifetimeStart)
----------------
can we avoid removing them? it can break other optimizations.
I guess the goal is to avoid them in tagLifetimeEnd, but recalculating them in StandardLifetime there is cheap.


================
Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:1499
+  for (auto It = AllocasToInstrument.begin(); It != AllocasToInstrument.end();
+       ++It) {
+    AllocaInst *AI = It->first;
----------------

```
for (auto &KV : AllocasToInstrument) {
  AllocaInst *AI = KV.first;
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105201