[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
----------------
eugenis wrote:
> fmayer wrote:
> > eugenis wrote:
> > > fmayer wrote:
> > > > vitalybuka wrote:
> > > > > vitalybuka wrote:
> > > > > > 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
> > > > > Actually compiler-rt should not be in this patch. Ideally two additional patches
> > > > > 1. D105201 only llvm/
> > > > > 2. I guess we  will add proper clang fronted flag to avoid "-mllvm -hwasan-use-after-scope" ? similar -fsanitize-address-use-after-scope. Doing this before 4 will save one patch to replace them "-mllvm -hwasan-use-after-scope" -> -fsanitize-hwaddress-use-after-scope
> > > > > 3. Clone compiler-rt tests with XFAIL:*
> > > > > 4. Update and enable -hwasan-use-after-scope
> > > > > 
> > > > > D105201 changes llvm/lib/Transforms/Instrumentation/, so we need tests in llvm/test/Instrumentation/, similar to AddressSanitizer/lifetime.ll
> > > > > 
> > > > > if you move AArch64StackTagging.cpp into a separate refactoring NFC patch, it does not need new tests
> > > > >  
> > > > Ad 2. I will let eugenis comment on that, but to me it seems a lot of the HWAsan feature flags are passed via -mllvm without any intention to make them first-class clang options. Especially given we want to make this the default soon (see the other comment thread between eugenis and me).
> > > Let's look at performance and code size impact first, but I'd like that to just be on all the time, and the -mllvm option can be used to opt out if something goes wrong.
> > > 
> > > Note that memtag does not have a frontend flag for use-after-scope, either.
> > So is it okay to leave as is? I'll add an LLVM IR level test as well to this change.
> Yes. I'd rather not introduce a clang flag unless we know that there will be users who would want it both ways. We can not easily deprecate clang flags.
> 
> Yes. I'd rather not introduce a clang flag unless we know that there will be users who would want it both ways. We can not easily deprecate clang flags.
OK

> So is it okay to leave as is? I'll add an LLVM IR level test as well to this change.
OK




================
Comment at: compiler-rt/test/hwasan/TestCases/use-after-scope-types.cpp:24
+template <class T>
+struct Ptr {
   void Store(T *ptr) { t = ptr; }
----------------
fmayer wrote:
> i'll sort out the autoformatting  diff.
That's would be great after D106159 and before this one


================
Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:1294
     // Replace uses of the alloca with tagged address.
-    Value *Tag = getAllocaTag(IRB, StackTag, AI, N);
+    Value *Tag = getAllocaTag(IRB, StackTag, AI, ++N);
     Value *AILong = IRB.CreatePointerCast(AI, IntptrTy);
----------------
Probably not important but now it's starts with N==1


================
Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:1319
+    bool StandardLifetime = UnrecognizedLifetimes.empty() &&
+                            Info.LifetimeStart.size() == 1 &&
+                            Info.LifetimeEnd.size() == 1;
----------------
I guess asan handles multiple starts/ends?


================
Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:1331
+      tagAlloca(IRB, AI, Tag, Size);
+      if (!forAllReachableExits(DT, PDT, Start, End, RetVec, TagEnd)) {
+        End->eraseFromParent();
----------------
{} unneeded


================
Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:1476
+      if (LocalDT == nullptr) {
+        DeleteDT = std::make_unique<DominatorTree>(F);
+        LocalDT = DeleteDT.get();
----------------
why we need these empty Trees?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105201