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

[Marco Elver via Phabricator via llvm-commits]

melver added inline comments.


================
Comment at: llvm/include/llvm/Transforms/Instrumentation/SanitizerBinaryMetadata.h:38
+
+const char *const kSanitizerBinaryMetadataCoveredSection = "sanmd_covered";
+const char *const kSanitizerBinaryMetadataAtomicsSection = "sanmd_atomics";
----------------
inline constexpr char[]


================
Comment at: llvm/lib/CodeGen/SanitizerBinaryMetadata.cpp:79
+  };
+  MD->replaceOperandWith(1, MDNode::get(F.getContext(), NewAuxMDs));
+  return false;
----------------
dvyukov wrote:
> Should this be a new method on MDBuilder?
As discussed, this could just use the MDBuilder, and then extract the operand out of the new MDNode without using the full MDNode.


================
Comment at: llvm/lib/Transforms/Instrumentation/SanitizerBinaryMetadata.cpp:244
   // metadata (if enabled).
   uint32_t PerInstrFeatureMask = getEnabledPerInstructionFeature();
   // Don't emit unnecessary covered metadata for all functions to save space.
----------------
I think this is no longer PerInstrFeatureMask, but should instead become FeatureMask.


================
Comment at: llvm/lib/Transforms/Instrumentation/SanitizerBinaryMetadata.cpp:247
   bool RequiresCovered = false;
-  if (PerInstrFeatureMask) {
+  if (PerInstrFeatureMask || Options.UAR) {
     for (BasicBlock &BB : F)
----------------
Comment why also `|| Options.UAR` (because PerInstrFeatureMask setting of UAR bit is deferred?).


================
Comment at: llvm/lib/Transforms/Instrumentation/SanitizerBinaryMetadata.cpp:254
+  if (F.isVarArg())
+    PerInstrFeatureMask &= ~kSanitizerBinaryMetadataUAR;
+  if (PerInstrFeatureMask & kSanitizerBinaryMetadataUAR)
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136078