[IR] - Make User construction exception safe

[kkretzsch]

Summary

There are many instruction ctors that call the setName method of the Value base class, which can throw a bad_alloc exception in OOM situations. In such situations special User delete operators are called which are not implemented yet.

Note that some User subclasses change the information which is required to calculate the storage pointer of a User object (e.g.NumUserOperands). Those subclasses need an operator delete that restores the changed information to its original value. Example: GlobalVariable.h

[kkretzsch]

Hi,
should I discuss this change on llvm-dev first?

Thanks,
Klaus

[kkretzsch]

Here is some additional information about the problem that this patch is intending to fix. Lets look at the construction of a CallInst instruction during IR generation:

static CallInst *Create(FunctionType *Ty, Value *Func, ArrayRef<Value *> Args, .. ){
...

return new (TotalOps, DescriptorBytes) CallInst(Ty, Func, Args, Bundles, NameStr, InsertBefore);

}

CallInst::CalInst(Value* Func, ...) {
...
Op<-1>() = Func;
....
setName(name); // throws
...
}
Op<-1>() returns a reference to a Use object of the CallInst instruction and the operator= inserts this use object into the UseList of Func.
The same object is removed from that UseList by calling the User::operator delete If the CallInst object is deleted.
Since setName can throw a bad_alloc exception (if LLVM_ENABLE_EXCEPTIONS is switched on), the unwind chain runs into assertions ("Constructor throws?") in
special User::operator deletes operators:

• operator delete(void* Usr, unsigned)
• operator delete(void* Usr, unsigned, bool)

This situation can be fixed by simlpy calling the User::operator delete(void*) in these unimplemented methods.

To ensure that this additional call succeeds all information that is necessary to calculate the storage pointer from the Usr address
must be restored in the special case that a sublass has changed this information, e.g. GlobalVariable can change the NumberOfOperands.

[rnk]

Seems reasonable, a few nits.

It would be nice if we could add a test for this scenario that runs when EH is enabled, but everything I can think of seems too heavyweight and non-portable to be worth it.

include/llvm/IR/GlobalVariable.h

79

LLVM usually binds pointers and references to the identifier, not the type. clang-format with LLVM style will do this for you here and elsewhere.

include/llvm/IR/User.h

104

Make this comment a complete sentence with a leading capital, or delete it. It's slightly redundant with the doc comment.

[kkretzsch]

Thanks Reid, I've updated the diff.
Concerning tests, yes we will add some tests which will test those exceptional OOM situations. We are thinking to introduce special out-of-memory malfunction tests in the llvm test-suite, which will be able to provoke bad_alloc exceptions when the number of allocations hits a given threshold. By increasing this threshold you will be able to test the unwinding at all places where allocations occur.

However since those tests can fail whenever someone adds code which is not bad_alloc-exception safe, we first have to discuss adding this new test-category with the llvm-dev community. That's why we didn't add tests in this change. But that will be our next step ...

When you are ok with the latest diff, can you approve? I have commit rights now and can commit the diff myself.

Thanks!

[kkretzsch]

Hi Chandler,
it is difficult to find a responsible for the IR part, but I heard the you have worked in that area recently. I'd like to get this change approved asap; if no approval is possible now, any help what I can do to get an approval is much appreciated.

Thanks,
Klaus

[rnk]

In D42731#1010035, @kkretzsch wrote:

Thanks Reid, I've updated the diff.
Concerning tests, yes we will add some tests which will test those exceptional OOM situations. We are thinking to introduce special out-of-memory malfunction tests in the llvm test-suite, which will be able to provoke bad_alloc exceptions when the number of allocations hits a given threshold. By increasing this threshold you will be able to test the unwinding at all places where allocations occur.

However since those tests can fail whenever someone adds code which is not bad_alloc-exception safe, we first have to discuss adding this new test-category with the llvm-dev community. That's why we didn't add tests in this change. But that will be our next step ...

I was imagining something more like a unit test that gets basic coverage of these codepaths when configured with LLVM_ENABLE_EXCEPTIONS, but in any case, I don't think it's a blocking issue.

When you are ok with the latest diff, can you approve? I have commit rights now and can commit the diff myself.

Yes, looks good. Sorry for the delay, I was ill.

include/llvm/IR/GlobalVariable.h

 
   ~GlobalVariable() {
     dropAllReferences();
-
-    // FIXME: needed by operator delete
-    setGlobalVariableNumOperands(1);
   }
 
   // allocate space for exactly one operand

     return User::operator new(s, 1);
   }
 
+  // delete space for exactly one operand as created in the corresponding new operator
+  void operator delete(void* ptr){
+    assert(ptr != nullptr && "must not be nullptr");
+    User *Obj = static_cast<User *>(ptr);
+    // Number of operands can be set to 0 after construction and initialization. Make sure
+    // that number of operands is reset to 1, as this is needed in User::operator delete
+    Obj->setGlobalVariableNumOperands(1);
+    User::operator delete(Obj);
+  }
+
   /// Provide fast operand accessors
   DECLARE_TRANSPARENT_OPERAND_ACCESSORS(Value);
 

include/llvm/IR/User.h

 
   /// \brief Free memory allocated for User and Use objects.
   void operator delete(void *Usr);
-  /// \brief Placement delete - required by std, but never called.
-  void operator delete(void*, unsigned) {
+  /// \brief Placement delete - required by std, called if the ctor throws.
+  void operator delete(void* Usr, unsigned) {
+    // called when constructor throws.
+    // 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
   }
-  /// \brief Placement delete - required by std, but never called.
-  void operator delete(void*, unsigned, bool) {
+  /// \brief Placement delete - required by std, called if the ctor throws.
+  void operator delete(void* Usr, unsigned, bool) {
+    // called when constructor throws.
+    // 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
   }
 
 protected: