[PATCH] D136078: Use-after-return sanitizer binary metadata

[Dmitry Vyukov via Phabricator via cfe-commits]

dvyukov marked an inline comment as done.
dvyukov added a comment.

PTAL



================
Comment at: llvm/lib/Transforms/Instrumentation/SanitizerBinaryMetadata.cpp:254
+  if (F.isVarArg())
+    PerInstrFeatureMask &= ~kSanitizerBinaryMetadataUAR;
+  if (PerInstrFeatureMask & kSanitizerBinaryMetadataUAR)
----------------
melver wrote:
> What if Options.Covered==true?
> 
> Will it still emit some UAR metadata or will it emit something it shouldn't?
> 
> 
> Should the F.isVarArg() check be done above in `if (PerInstrFeatureMask || (Options.UAR && !F.isVarArg())` ? Then you wouldn't need the `PerInstrFeatureMask && RequiresCovered` change below and it could still just check `RequiresCovered` as before.
> What if Options.Covered==true?
> Will it still emit some UAR metadata or will it emit something it shouldn't?

If Options.Covered==true && F.isVarArg(),  we emit covered metadata with features=0. This looks WAI.

> Should the F.isVarArg() check be done above in if (PerInstrFeatureMask || (Options.UAR && !F.isVarArg()) ? Then you wouldn't need the PerInstrFeatureMask && RequiresCovered change below and it could still just check RequiresCovered as before.

It's tricky.
I forgot why I structured the code this way, but I added a new test for all permutations of covered/atomics/uar and it shows the following breakage with your proposed change:

```
  - CHECK-AU: ellipsis: features=1 stack_args=0
  + CHECK-AU: ellipsis: features=3 stack_args=0
```


================
Comment at: llvm/test/Instrumentation/SanitizerBinaryMetadata/covered.cpp:1
+// REQUIRES: native && target-x86_64
+// RUN: clang++ %s -o %t && %t | FileCheck %s
----------------
This is the new test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136078