[IR] Use alloc markers for operator delete variants

[marcauberer]

Follow-up to #129914
This fixes the problem properly by using the data via the allocation markers.

PS: Sorry for the long delay ;)

[llvmbot]

@llvm/pr-subscribers-llvm-ir

Author: Marc Auberer (marcauberer)

Changes

Follow-up to #129914
This fixes the problem properly by using the data via the allocation markers.

PS: Sorry for the long delay ;)

---

Full diff: https://github.com/llvm/llvm-project/pull/138261.diff

2 Files Affected:

• (modified) llvm/include/llvm/IR/User.h (+7-35)
• (modified) llvm/lib/IR/User.cpp (+64-22)

diff --git a/llvm/include/llvm/IR/User.h b/llvm/include/llvm/IR/User.h
index 39e1314bd8130..634b14341d243 100644
--- a/llvm/include/llvm/IR/User.h
+++ b/llvm/include/llvm/IR/User.h
@@ -54,7 +54,10 @@ class User : public Value {
   void *operator new(size_t Size) = delete;
 
   /// Indicates this User has operands "hung off" in another allocation.
-  struct HungOffOperandsAllocMarker {};
+  struct HungOffOperandsAllocMarker {
+    /// The number of operands for this User.
+    const unsigned NumOps;
+  };
 
   /// Indicates this User has operands co-allocated.
   struct IntrusiveOperandsAllocMarker {
@@ -145,42 +148,11 @@ class User : public Value {
   /// Free memory allocated for User and Use objects.
   void operator delete(void *Usr);
   /// Placement delete - required by std, called if the ctor throws.
-  void operator delete(void *Usr, HungOffOperandsAllocMarker) {
-    // Note: If a subclass manipulates the information which is required to
-    // calculate the Usr memory pointer, e.g. NumUserOperands, the operator
-    // delete of that subclass has to restore the changed information to the
-    // original value, since the dtor of that class is not called if the ctor
-    // fails.
-    User::operator delete(Usr);
-
-#ifndef LLVM_ENABLE_EXCEPTIONS
-    llvm_unreachable("Constructor throws?");
-#endif
-  }
+  void operator delete(void *Usr, HungOffOperandsAllocMarker);
   /// Placement delete - required by std, called if the ctor throws.
-  void operator delete(void *Usr, IntrusiveOperandsAllocMarker) {
-    // Note: If a subclass manipulates the information which is required to calculate the
-    // Usr memory pointer, e.g. NumUserOperands, the operator delete of that subclass has
-    // to restore the changed information to the original value, since the dtor of that class
-    // is not called if the ctor fails.
-    User::operator delete(Usr);
-
-#ifndef LLVM_ENABLE_EXCEPTIONS
-    llvm_unreachable("Constructor throws?");
-#endif
-  }
+  void operator delete(void *Usr, IntrusiveOperandsAllocMarker Marker);
   /// Placement delete - required by std, called if the ctor throws.
-  void operator delete(void *Usr, IntrusiveOperandsAndDescriptorAllocMarker) {
-    // Note: If a subclass manipulates the information which is required to calculate the
-    // Usr memory pointer, e.g. NumUserOperands, the operator delete of that subclass has
-    // to restore the changed information to the original value, since the dtor of that class
-    // is not called if the ctor fails.
-    User::operator delete(Usr);
-
-#ifndef LLVM_ENABLE_EXCEPTIONS
-    llvm_unreachable("Constructor throws?");
-#endif
-  }
+  void operator delete(void *Usr, IntrusiveOperandsAndDescriptorAllocMarker);
 
 protected:
   template <int Idx, typename U> static Use &OpFrom(const U *that) {
diff --git a/llvm/lib/IR/User.cpp b/llvm/lib/IR/User.cpp
index ab44cb4b8a3f7..fea8aa6a02338 100644
--- a/llvm/lib/IR/User.cpp
+++ b/llvm/lib/IR/User.cpp
@@ -20,13 +20,14 @@ class BasicBlock;
 
 bool User::replaceUsesOfWith(Value *From, Value *To) {
   bool Changed = false;
-  if (From == To) return Changed;   // Duh what?
+  if (From == To)
+    return Changed; // Duh what?
 
   assert((!isa<Constant>(this) || isa<GlobalValue>(this)) &&
          "Cannot call User::replaceUsesOfWith on a constant!");
 
   for (unsigned i = 0, E = getNumOperands(); i != E; ++i)
-    if (getOperand(i) == From) {  // Is This operand is pointing to oldval?
+    if (getOperand(i) == From) { // Is This operand is pointing to oldval?
       // The side effects of this setOperand call include linking to
       // "To", adding "this" to the uses list of To, and
       // most importantly, removing "this" from the use list of "From".
@@ -57,7 +58,7 @@ void User::allocHungoffUses(unsigned N, bool IsPhi) {
   size_t size = N * sizeof(Use);
   if (IsPhi)
     size += N * sizeof(BasicBlock *);
-  Use *Begin = static_cast<Use*>(::operator new(size));
+  Use *Begin = static_cast<Use *>(::operator new(size));
   Use *End = Begin + N;
   setOperandList(Begin);
   for (; Begin != End; Begin++)
@@ -89,7 +90,6 @@ void User::growHungoffUses(unsigned NewNumUses, bool IsPhi) {
   Use::zap(OldOps, OldOps + OldNumUses, true);
 }
 
-
 // This is a private struct used by `User` to track the co-allocated descriptor
 // section.
 struct DescriptorInfo {
@@ -191,28 +191,70 @@ void *User::operator new(size_t Size, HungOffOperandsAllocMarker) {
 LLVM_NO_SANITIZE_MEMORY_ATTRIBUTE void User::operator delete(void *Usr) {
   // Hung off uses use a single Use* before the User, while other subclasses
   // use a Use[] allocated prior to the user.
-  User *Obj = static_cast<User *>(Usr);
+  const auto *Obj = static_cast<User *>(Usr);
   if (Obj->HasHungOffUses) {
-    assert(!Obj->HasDescriptor && "not supported!");
-
-    Use **HungOffOperandList = static_cast<Use **>(Usr) - 1;
-    // drop the hung off uses.
-    Use::zap(*HungOffOperandList, *HungOffOperandList + Obj->NumUserOperands,
-             /* Delete */ true);
-    ::operator delete(HungOffOperandList);
+    const HungOffOperandsAllocMarker Marker{
+        Obj->NumUserOperands,
+    };
+    operator delete(Usr, Marker);
   } else if (Obj->HasDescriptor) {
-    Use *UseBegin = static_cast<Use *>(Usr) - Obj->NumUserOperands;
-    Use::zap(UseBegin, UseBegin + Obj->NumUserOperands, /* Delete */ false);
-
-    auto *DI = reinterpret_cast<DescriptorInfo *>(UseBegin) - 1;
-    uint8_t *Storage = reinterpret_cast<uint8_t *>(DI) - DI->SizeInBytes;
-    ::operator delete(Storage);
+    const IntrusiveOperandsAndDescriptorAllocMarker Marker{
+        Obj->NumUserOperands,
+        Obj->HasDescriptor,
+    };
+    operator delete(Usr, Marker);
   } else {
-    Use *Storage = static_cast<Use *>(Usr) - Obj->NumUserOperands;
-    Use::zap(Storage, Storage + Obj->NumUserOperands,
-             /* Delete */ false);
-    ::operator delete(Storage);
+    const IntrusiveOperandsAllocMarker Marker{
+        Obj->NumUserOperands,
+    };
+    operator delete(Usr, Marker);
   }
 }
 
+// Repress memory sanitization, due to use-after-destroy by operator
+// delete. Bug report 24578 identifies this issue.
+LLVM_NO_SANITIZE_MEMORY_ATTRIBUTE void
+User::operator delete(void *Usr, const HungOffOperandsAllocMarker Marker) {
+  // Note: If a subclass manipulates the information which is required to
+  // calculate the Usr memory pointer, e.g. NumUserOperands, the operator
+  // delete of that subclass has to restore the changed information to the
+  // original value, since the dtor of that class is not called if the ctor
+  // fails.
+  Use **HungOffOperandList = static_cast<Use **>(Usr) - 1;
+  // drop the hung off uses.
+  Use::zap(*HungOffOperandList, *HungOffOperandList + Marker.NumOps,
+           /* Delete */ true);
+  ::operator delete(HungOffOperandList);
+}
+
+// Repress memory sanitization, due to use-after-destroy by operator
+// delete. Bug report 24578 identifies this issue.
+LLVM_NO_SANITIZE_MEMORY_ATTRIBUTE void
+User::operator delete(void *Usr, const IntrusiveOperandsAllocMarker Marker) {
+  // Note: If a subclass manipulates the information which is required to
+  // calculate the Usr memory pointer, e.g. NumUserOperands, the operator delete
+  // of that subclass has to restore the changed information to the original
+  // value, since the dtor of that class is not called if the ctor fails.
+  Use *Storage = static_cast<Use *>(Usr) - Marker.NumOps;
+  Use::zap(Storage, Storage + Marker.NumOps, /* Delete */ false);
+  ::operator delete(Storage);
+}
+
+// Repress memory sanitization, due to use-after-destroy by operator
+// delete. Bug report 24578 identifies this issue.
+LLVM_NO_SANITIZE_MEMORY_ATTRIBUTE void
+User::operator delete(void *Usr,
+                      const IntrusiveOperandsAndDescriptorAllocMarker Marker) {
+  // Note: If a subclass manipulates the information which is required to
+  // calculate the Usr memory pointer, e.g. NumUserOperands, the operator delete
+  // of that subclass has to restore the changed information to the original
+  // value, since the dtor of that class is not called if the ctor fails.
+  Use *UseBegin = static_cast<Use *>(Usr) - Marker.NumOps;
+  Use::zap(UseBegin, UseBegin + Marker.NumOps, /* Delete */ false);
+
+  auto *DI = reinterpret_cast<DescriptorInfo *>(UseBegin) - 1;
+  uint8_t *Storage = reinterpret_cast<uint8_t *>(DI) - DI->SizeInBytes;
+  ::operator delete(Storage);
+}
+
 } // namespace llvm

[nikic]

LGTM, thanks for following up on this!

[marcauberer]

I will run our malfunction tests again tomorrow and see if they pass. If so, I will merge this. Thanks for the reviews!

[marcauberer]

Heads up: Our malfunction tests revealed another problem with this:
(Some of the) child classes implement a custom operator new and operator delete, which in turn call their respective counterparts in User. Until now, in those child classes operator new(Size) delegates to User::operator new(Size, AllocMarker), but operator delete(void*) delegates to User::operator delete(void*), which is a mismatch and does not work in the scenario, where the constructor throws.
I will investigate further, but this blocks the merge for now.