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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
