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