[PATCH] D136078: [RFC] Use-after-return binary metadata.

[Marco Elver via Phabricator via cfe-commits]

melver added a comment.

In D136078#3867627 <https://reviews.llvm.org/D136078#3867627>, @dvyukov wrote:

> Unrelated, but looking at the metadata format more closely, I think this can benefit from LEB128-like varlen encoding.
> Function size is small.
> Features are very small.
> Stack args size is small.
> Function start (if we encode it from the end of the previous one) is very small as well.

Function start is currently encoded as a relative relocation (see AsmPrinter). Anything else, including attempting to use a previous function does not work because of section merging, potential discarding of functions by linker (ffunction-sections), etc.

We could implement compression in the linker, which would be the most reliable. Otherwise we can only play with encoding of the aux constants.

> So potentially it can reduce the entry size from 16 bytes to just 4.
> I am just thinking if we use 10x more metadata in future, the size can grow from percents of binary to tens of percents.

If that's the case, I think the only option is to implement compression in the linker.



================
Comment at: llvm/include/llvm/Transforms/Instrumentation.h:166
 
+constexpr int kSanitizerMetadataAtomicsBit = 0;
+constexpr int kSanitizerMetadataUARBit = 1;
----------------
inline constexpr

But overall, I'd prefer if these wouldn't need to be defined here, but instead in SanitizerBinaryMetadata.h.

Also, re naming "SanitizerMetadata" is already overloaded and we need to stick to SanitizerBinaryMetadata :-/


================
Comment at: llvm/lib/CodeGen/SanitizerMetadata.cpp:1
+//===- SanitizerMetadata.cpp ----------------------------------------------===//
+//
----------------
SanitizerMetadata is already overloaded for other purposes (see clang/lib/CodeGen/SanitizerMetadata.cpp).

(Which is why I had to choose SanitizerBinaryMetadata)


================
Comment at: llvm/lib/CodeGen/SanitizerMetadata.cpp:47
+  if (!MD)
+    return false;
+  auto &AuxMDs = *cast<MDTuple>(MD->getOperand(1));
----------------
I think you also need to check the target section name matches (in operand 0). Because having !pcsections does not imply SanitizerMetadata enabled.


Perhaps at this point it might make sense to move MetadataInfo to SanitizerBinaryMetadata.h (as SanitizerBinaryMetadataInfo perhaps ... long I know :-/ .. unfortunately "SanitizerMetadata" is already overloaded elsewhere -- SanitizerBinaryMDInfo?).


================
Comment at: llvm/lib/CodeGen/SanitizerMetadata.cpp:49
+  auto &AuxMDs = *cast<MDTuple>(MD->getOperand(1));
+  const auto &Features = cast<ConstantAsMetadata>(AuxMDs.getOperand(0))
+                             ->getValue()
----------------
APInt


================
Comment at: llvm/lib/IR/MDBuilder.cpp:176
+      // MachineSanitizerMetadata pass may modify it later.
+      Ops.push_back(MDNode::getDistinct(Context, AuxMDs));
     }
----------------
This is not great. We still want MDNode interning to work for these.

Doesn't replaceOperandWith() work where you want to replace the operand?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136078