[Metadata][NFC] Refactor allocation, initialization, and deletion of MDNodes

[wolfgangp]

Summary

This patch is refactoring the allocation, initialization and deletion of MDNodes. It is intended as a preparatory patch for the upcoming
addition of dynamic resizability of MDNodes. It is fundamentally NFC, but removes the necessity for suppressing the memory sanitizer for
MDNode's operator delete.

Refer to the discussion in D124548 for more context.

The intended usage of SmallVector to handle the operands of "large" and resizable MDNodes seems to require that we define move constructors, copy constructors and the respective assignment operators for MDOperand. I've saved these for a later patch, as they're not required for this one.

The refactoring of operator new() requiring an extra parameter (Storage) is preparatory in the sense that operator new will have to decide whether to allocate a node for resizability based on storage type.

[dexonsmith]

LGTM! Thanks for splitting this out (and fixing the MSan problem properly!).

llvm/lib/IR/Metadata.cpp

538–539

For reference, I dug this up as https://github.com/llvm/llvm-project/issues/24952.

llvm/include/llvm/IR/Metadata.h

 /// If an unresolved node is part of a cycle, \a resolveCycles() needs
 /// to be called on some member of the cycle once all temporary nodes have been
 /// replaced.
 class MDNode : public Metadata {
   friend class ReplaceableMetadataImpl;
   friend class LLVMContextImpl;
   friend class DIArgList;
 
+  /// The header that is coallocated with an MDNode, along with the operands.
+  /// It is located immediately before the main body of the node. The operands
+  /// are in turn located immediately before the header.
+  struct Header {
     unsigned NumOperands;
-  unsigned NumUnresolved;
+    unsigned NumUnresolved = 0;
+
+    static constexpr size_t getOpSize(unsigned NumOps) {
+      return sizeof(MDOperand) * NumOps;
+    }
+    static constexpr size_t getAllocSize(unsigned NumOps) {
+      return getOpSize(NumOps) + sizeof(Header);
+    }
+    void *getAllocation() {
+      return reinterpret_cast<char *>(this + 1) -
+             alignTo(getAllocSize(NumOperands), alignof(uint64_t));
+    }
+
+    explicit Header(unsigned NumOperands);
+    ~Header();
+    MutableArrayRef<MDOperand> operands() {
+      return makeMutableArrayRef(
+          reinterpret_cast<MDOperand *>(this) - NumOperands, NumOperands);
+    }
+    ArrayRef<MDOperand> operands() const {
+      return makeArrayRef(
+          reinterpret_cast<const MDOperand *>(this) - NumOperands, NumOperands);
+    }
+  };
+
+  Header &getHeader() { return *(reinterpret_cast<Header *>(this) - 1); }
+
+  const Header &getHeader() const {
+    return *(reinterpret_cast<const Header *>(this) - 1);
+  }
 
   ContextAndReplaceableUses Context;
 
 protected:
   MDNode(LLVMContext &Context, unsigned ID, StorageType Storage,
          ArrayRef<Metadata *> Ops1, ArrayRef<Metadata *> Ops2 = None);
   ~MDNode() = default;
 
-  void *operator new(size_t Size, unsigned NumOps);
+  void *operator new(size_t Size, unsigned NumOps, StorageType Storage);
   void operator delete(void *Mem);
 
   /// Required by std, but never called.
   void operator delete(void *, unsigned) {
     llvm_unreachable("Constructor throws?");
   }
 
   /// Required by std, but never called.
   void operator delete(void *, unsigned, bool) {
     llvm_unreachable("Constructor throws?");
   }
 
   void dropAllReferences();
 
-  MDOperand *mutable_begin() { return mutable_end() - NumOperands; }
-  MDOperand *mutable_end() { return reinterpret_cast<MDOperand *>(this); }
+  MDOperand *mutable_begin() { return getHeader().operands().begin(); }
+  MDOperand *mutable_end() { return getHeader().operands().end(); }
 
   using mutable_op_range = iterator_range<MDOperand *>;
 
   mutable_op_range mutable_operands() {
     return mutable_op_range(mutable_begin(), mutable_end());
   }
 
 public:
public:
   /// this always returns \c true.
   ///
   /// If \a isUniqued(), returns \c true if this has already dropped RAUW
   /// support (because all operands are resolved).
   ///
   /// As forward declarations are resolved, their containers should get
   /// resolved automatically.  However, if this (or one of its operands) is
   /// involved in a cycle, \a resolveCycles() needs to be called explicitly.
-  bool isResolved() const { return !isTemporary() && !NumUnresolved; }
+  bool isResolved() const { return !isTemporary() && !getNumUnresolved(); }
 
   bool isUniqued() const { return Storage == Uniqued; }
   bool isDistinct() const { return Storage == Distinct; }
   bool isTemporary() const { return Storage == Temporary; }
 
   /// RAUW a temporary.
   ///
   /// \pre \a isTemporary() must be \c true.
private:
   MDNode *replaceWithDistinctImpl();
 
 protected:
   /// Set an operand.
   ///
   /// Sets the operand directly, without worrying about uniquing.
   void setOperand(unsigned I, Metadata *New);
 
+  unsigned getNumUnresolved() const { return getHeader().NumUnresolved; }
+
+  void setNumUnresolved(unsigned N) { getHeader().NumUnresolved = N; }
   void storeDistinctInContext();
   template <class T, class StoreT>
   static T *storeImpl(T *N, StorageType Storage, StoreT &Store);
   template <class T> static T *storeImpl(T *N, StorageType Storage);
 
 private:
   void handleChangedOperand(void *Ref, Metadata *New);
 
public:
 
   op_iterator op_end() const {
     return const_cast<MDNode *>(this)->mutable_end();
   }
 
   op_range operands() const { return op_range(op_begin(), op_end()); }
 
   const MDOperand &getOperand(unsigned I) const {
-    assert(I < NumOperands && "Out of range");
-    return op_begin()[I];
+    assert(I < getNumOperands() && "Out of range");
+    return getHeader().operands()[I];
   }
 
   /// Return number of MDNode operands.
-  unsigned getNumOperands() const { return NumOperands; }
+  unsigned getNumOperands() const { return getHeader().NumOperands; }
 
   /// Methods for support type inquiry through isa, cast, and dyn_cast:
   static bool classof(const Metadata *MD) {
     switch (MD->getMetadataID()) {
     default:
       return false;
 #define HANDLE_MDNODE_LEAF(CLASS)                                              \
   case CLASS##Kind:                                                            \

llvm/lib/IR/DebugInfoMetadata.cpp

DILocation *DILocation::getImpl(LLVMContext &Context, unsigned Line,
   } else {
     assert(ShouldCreate && "Expected non-uniqued nodes to always be created");
   }
 
   SmallVector<Metadata *, 2> Ops;
   Ops.push_back(Scope);
   if (InlinedAt)
     Ops.push_back(InlinedAt);
-  return storeImpl(new (Ops.size()) DILocation(Context, Storage, Line, Column,
-                                               Ops, ImplicitCode),
+  return storeImpl(new (Ops.size(), Storage) DILocation(
+                       Context, Storage, Line, Column, Ops, ImplicitCode),
                    Storage, Context.pImpl->DILocations);
 }
 
 const DILocation *
 DILocation::getMergedLocations(ArrayRef<const DILocation *> Locs) {
   if (Locs.empty())
     return nullptr;
   if (Locs.size() == 1)
if (Storage == Uniqued) {
     Hash = Key.getHash();
   } else {
     assert(ShouldCreate && "Expected non-uniqued nodes to always be created");
   }
 
   // Use a nullptr for empty headers.
   assert(isCanonical(Header) && "Expected canonical MDString");
   Metadata *PreOps[] = {Header};
-  return storeImpl(new (DwarfOps.size() + 1) GenericDINode(
+  return storeImpl(new (DwarfOps.size() + 1, Storage) GenericDINode(
                        Context, Storage, Hash, Tag, PreOps, DwarfOps),
                    Storage, Context.pImpl->GenericDINodes);
 }
 
 void GenericDINode::recalculateHash() {
   setHash(GenericDINodeInfo::KeyTy::calculateHash(this));
 }
 
 #define UNWRAP_ARGS_IMPL(...) __VA_ARGS__
 #define UNWRAP_ARGS(ARGS) UNWRAP_ARGS_IMPL ARGS
 #define DEFINE_GETIMPL_LOOKUP(CLASS, ARGS)                                     \
   do {                                                                         \
     if (Storage == Uniqued) {                                                  \
       if (auto *N = getUniqued(Context.pImpl->CLASS##s,                        \
                                CLASS##Info::KeyTy(UNWRAP_ARGS(ARGS))))         \
         return N;                                                              \
       if (!ShouldCreate)                                                       \
         return nullptr;                                                        \
     } else {                                                                   \
       assert(ShouldCreate &&                                                   \
              "Expected non-uniqued nodes to always be created");               \
     }                                                                          \
   } while (false)
 #define DEFINE_GETIMPL_STORE(CLASS, ARGS, OPS)                                 \
-  return storeImpl(new (array_lengthof(OPS))                                   \
+  return storeImpl(new (array_lengthof(OPS), Storage)                          \
                        CLASS(Context, Storage, UNWRAP_ARGS(ARGS), OPS),        \
                    Storage, Context.pImpl->CLASS##s)
 #define DEFINE_GETIMPL_STORE_NO_OPS(CLASS, ARGS)                               \
-  return storeImpl(new (0u) CLASS(Context, Storage, UNWRAP_ARGS(ARGS)),        \
+  return storeImpl(new (0u, Storage)                                           \
+                       CLASS(Context, Storage, UNWRAP_ARGS(ARGS)),             \
                    Storage, Context.pImpl->CLASS##s)
 #define DEFINE_GETIMPL_STORE_NO_CONSTRUCTOR_ARGS(CLASS, OPS)                   \
-  return storeImpl(new (array_lengthof(OPS)) CLASS(Context, Storage, OPS),     \
+  return storeImpl(new (array_lengthof(OPS), Storage)                          \
+                       CLASS(Context, Storage, OPS),                           \
                    Storage, Context.pImpl->CLASS##s)
 #define DEFINE_GETIMPL_STORE_N(CLASS, ARGS, OPS, NUM_OPS)                      \
-  return storeImpl(new (NUM_OPS)                                               \
+  return storeImpl(new (NUM_OPS, Storage)                                      \
                        CLASS(Context, Storage, UNWRAP_ARGS(ARGS), OPS),        \
                    Storage, Context.pImpl->CLASS##s)
 
 DISubrange::DISubrange(LLVMContext &C, StorageType Storage,
                        ArrayRef<Metadata *> Ops)
     : DINode(C, DISubrangeKind, Storage, dwarf::DW_TAG_subrange_type, Ops) {}
 DISubrange *DISubrange::getImpl(LLVMContext &Context, int64_t Count, int64_t Lo,
                                 StorageType Storage, bool ShouldCreate) {
Metadata *Ops[] = {File,
                      SplitDebugFilename,
                      EnumTypes,
                      RetainedTypes,
                      GlobalVariables,
                      ImportedEntities,
                      Macros,
                      SysRoot,
                      SDK};
-  return storeImpl(new (array_lengthof(Ops)) DICompileUnit(
+  return storeImpl(new (array_lengthof(Ops), Storage) DICompileUnit(
                        Context, Storage, SourceLanguage, IsOptimized,
                        RuntimeVersion, EmissionKind, DWOId, SplitDebugInlining,
                        DebugInfoForProfiling, NameTableKind, RangesBaseAddress,
                        Ops),
                    Storage);
 }
 
 Optional<DICompileUnit::DebugEmissionKind>

llvm/lib/IR/Metadata.cpp

 // Assert that the MDNode types will not be unaligned by the objects
 // prepended to them.
 #define HANDLE_MDNODE_LEAF(CLASS)                                              \
   static_assert(                                                               \
       alignof(uint64_t) >= alignof(CLASS),                                     \
       "Alignment is insufficient after objects prepended to " #CLASS);
 #include "llvm/IR/Metadata.def"
 
-void *MDNode::operator new(size_t Size, unsigned NumOps) {
-  size_t OpSize = NumOps * sizeof(MDOperand);
+void *MDNode::operator new(size_t Size, unsigned NumOps,
+                           StorageType /* Storage */) {
   // uint64_t is the most aligned type we need support (ensured by static_assert
   // above)
-  OpSize = alignTo(OpSize, alignof(uint64_t));
-  void *Ptr = reinterpret_cast<char *>(::operator new(OpSize + Size)) + OpSize;
-  MDOperand *O = static_cast<MDOperand *>(Ptr);
-  for (MDOperand *E = O - NumOps; O != E; --O)
-    (void)new (O - 1) MDOperand;
-  return Ptr;
+  size_t AllocSize = alignTo(Header::getAllocSize(NumOps), alignof(uint64_t));
+  char *Mem = reinterpret_cast<char *>(::operator new(AllocSize + Size));
+  Header *H = new (Mem + AllocSize - sizeof(Header)) Header(NumOps);
+  return reinterpret_cast<void *>(H + 1);
 }
 
-// Repress memory sanitization, due to use-after-destroy by operator
-// delete. Bug report 24578 identifies this issue.
-LLVM_NO_SANITIZE_MEMORY_ATTRIBUTE void MDNode::operator delete(void *Mem) {
-  MDNode *N = static_cast<MDNode *>(Mem);
-  size_t OpSize = N->NumOperands * sizeof(MDOperand);
-  OpSize = alignTo(OpSize, alignof(uint64_t));
-
-  MDOperand *O = static_cast<MDOperand *>(Mem);
-  for (MDOperand *E = O - N->NumOperands; O != E; --O)
-    (O - 1)->~MDOperand();
-  ::operator delete(reinterpret_cast<char *>(Mem) - OpSize);
+void MDNode::operator delete(void *N) {
+  Header *H = reinterpret_cast<Header *>(N) - 1;
+  void *Mem = H->getAllocation();
+  H->~Header();
+  ::operator delete(Mem);
 }
 
 MDNode::MDNode(LLVMContext &Context, unsigned ID, StorageType Storage,
                ArrayRef<Metadata *> Ops1, ArrayRef<Metadata *> Ops2)
-    : Metadata(ID, Storage), NumOperands(Ops1.size() + Ops2.size()),
-      NumUnresolved(0), Context(Context) {
+    : Metadata(ID, Storage), Context(Context) {
   unsigned Op = 0;
   for (Metadata *MD : Ops1)
     setOperand(Op++, MD);
   for (Metadata *MD : Ops2)
     setOperand(Op++, MD);
 
   if (!isUniqued())
     return;
default:
     llvm_unreachable("Invalid MDNode subclass");
 #define HANDLE_MDNODE_LEAF(CLASS)                                              \
   case CLASS##Kind:                                                            \
     return cast<CLASS>(this)->cloneImpl();
 #include "llvm/IR/Metadata.def"
   }
 }
 
+MDNode::Header::Header(unsigned NumOps) {
+  NumOperands = NumOps;
+  MDOperand *O = reinterpret_cast<MDOperand *>(this);
+  for (MDOperand *E = O - NumOps; O != E; --O)
+    (void)new (O - 1) MDOperand();
+}
+
+MDNode::Header::~Header() {
+  MDOperand *O = reinterpret_cast<MDOperand *>(this) - NumOperands;
+  for (MDOperand *E = O + NumOperands; O != E; ++O)
+    (void)O->~MDOperand();
+}
+
 static bool isOperandUnresolved(Metadata *Op) {
   if (auto *N = dyn_cast_or_null<MDNode>(Op))
     return !N->isResolved();
   return false;
 }
 
 void MDNode::countUnresolvedOperands() {
-  assert(NumUnresolved == 0 && "Expected unresolved ops to be uncounted");
+  assert(getNumUnresolved() == 0 && "Expected unresolved ops to be uncounted");
   assert(isUniqued() && "Expected this to be uniqued");
-  NumUnresolved = count_if(operands(), isOperandUnresolved);
+  setNumUnresolved(count_if(operands(), isOperandUnresolved));
 }
 
 void MDNode::makeUniqued() {
   assert(isTemporary() && "Expected this to be temporary");
   assert(!isResolved() && "Expected this to be unresolved");
 
   // Enable uniquing callbacks.
   for (auto &Op : mutable_operands())
     Op.reset(Op.get(), this);
 
   // Make this 'uniqued'.
   Storage = Uniqued;
   countUnresolvedOperands();
-  if (!NumUnresolved) {
+  if (!getNumUnresolved()) {
     dropReplaceableUses();
     assert(isResolved() && "Expected this to be resolved");
   }
 
   assert(isUniqued() && "Expected this to be uniqued");
 }
 
 void MDNode::makeDistinct() {
   assert(isTemporary() && "Expected this to be temporary");
   assert(!isResolved() && "Expected this to be unresolved");
 
   // Drop RAUW support and store as a distinct node.
   dropReplaceableUses();
   storeDistinctInContext();
 
   assert(isDistinct() && "Expected this to be distinct");
   assert(isResolved() && "Expected this to be resolved");
 }
 
 void MDNode::resolve() {
   assert(isUniqued() && "Expected this to be uniqued");
   assert(!isResolved() && "Expected this to be unresolved");
 
-  NumUnresolved = 0;
+  setNumUnresolved(0);
   dropReplaceableUses();
 
   assert(isResolved() && "Expected this to be resolved");
 }
 
 void MDNode::dropReplaceableUses() {
-  assert(!NumUnresolved && "Unexpected unresolved operand");
+  assert(!getNumUnresolved() && "Unexpected unresolved operand");
 
   // Drop any RAUW support.
   if (Context.hasReplaceableUses())
     Context.takeReplaceableUses()->resolveAllUses();
 }
 
 void MDNode::resolveAfterOperandChange(Metadata *Old, Metadata *New) {
   assert(isUniqued() && "Expected this to be uniqued");
-  assert(NumUnresolved != 0 && "Expected unresolved operands");
+  assert(getNumUnresolved() != 0 && "Expected unresolved operands");
 
   // Check if an operand was resolved.
   if (!isOperandUnresolved(Old)) {
     if (isOperandUnresolved(New))
       // An operand was un-resolved!
-      ++NumUnresolved;
+      setNumUnresolved(getNumUnresolved() + 1);
   } else if (!isOperandUnresolved(New))
     decrementUnresolvedOperandCount();
 }
 
 void MDNode::decrementUnresolvedOperandCount() {
   assert(!isResolved() && "Expected this to be unresolved");
   if (isTemporary())
     return;
 
   assert(isUniqued() && "Expected this to be uniqued");
-  if (--NumUnresolved)
+  setNumUnresolved(getNumUnresolved() - 1);
+  if (getNumUnresolved())
     return;
 
   // Last unresolved operand has just been resolved.
   dropReplaceableUses();
   assert(isResolved() && "Expected this to become resolved");
 }
 
 void MDNode::resolveCycles() {
MDNode *MDNode::replaceWithDistinctImpl() {
   return this;
 }
 
 void MDTuple::recalculateHash() {
   setHash(MDTupleInfo::KeyTy::calculateHash(this));
 }
 
 void MDNode::dropAllReferences() {
-  for (unsigned I = 0, E = NumOperands; I != E; ++I)
+  for (unsigned I = 0, E = getNumOperands(); I != E; ++I)
     setOperand(I, nullptr);
   if (Context.hasReplaceableUses()) {
     Context.getReplaceableUses()->resolveAllUses(/* ResolveUsers */ false);
     (void)Context.takeReplaceableUses();
   }
 }
 
 void MDNode::handleChangedOperand(void *Ref, Metadata *New) {
if (auto *N = getUniqued(Context.pImpl->MDTuples, Key))
       return N;
     if (!ShouldCreate)
       return nullptr;
     Hash = Key.getHash();
   } else {
     assert(ShouldCreate && "Expected non-uniqued nodes to always be created");
   }
 
-  return storeImpl(new (MDs.size()) MDTuple(Context, Storage, Hash, MDs),
+  return storeImpl(new (MDs.size(), Storage)
+                       MDTuple(Context, Storage, Hash, MDs),
                    Storage, Context.pImpl->MDTuples);
 }
 
 void MDNode::deleteTemporary(MDNode *N) {
   assert(N->isTemporary() && "Expected temporary node");
   N->replaceAllUsesWith(nullptr);
   N->deleteAsSubclass();
 }
 
 void MDNode::storeDistinctInContext() {
   assert(!Context.hasReplaceableUses() && "Unexpected replaceable uses");
-  assert(!NumUnresolved && "Unexpected unresolved nodes");
+  assert(!getNumUnresolved() && "Unexpected unresolved nodes");
   Storage = Distinct;
   assert(isResolved() && "Expected this to be resolved");
 
   // Reset the hash.
   switch (getMetadataID()) {
   default:
     llvm_unreachable("Invalid subclass of MDNode");
 #define HANDLE_MDNODE_LEAF(CLASS)                                              \
if (!isUniqued()) {
     setOperand(I, New);
     return;
   }
 
   handleChangedOperand(mutable_begin() + I, New);
 }
 
 void MDNode::setOperand(unsigned I, Metadata *New) {
-  assert(I < NumOperands);
+  assert(I < getNumOperands());
   mutable_begin()[I].reset(New, isUniqued() ? this : nullptr);
 }
 
 /// Get a node or a self-reference that looks like it.
 ///
 /// Special handling for finding self-references, for use by \a
 /// MDNode::concatenate() and \a MDNode::intersect() to maintain behaviour from
 /// when self-referencing nodes were still uniqued.  If the first operand has