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

[Evgenii Stepanov via Phabricator via llvm-commits]

eugenis 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
----------------
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.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105201