Page MenuHomePhabricator

[IR] - Make User construction exception safe
ClosedPublic

Authored by kkretzsch on Jan 31 2018, 4:13 AM.

Details

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

Diff Detail

Repository
rL LLVM

Event Timeline

kkretzsch created this revision.Jan 31 2018, 4:13 AM

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

Thanks,
Klaus

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 added a comment.Feb 15 2018, 9:51 AM

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 ↗(On Diff #132124)

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 ↗(On Diff #132124)

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

kkretzsch updated this revision to Diff 134597.EditedFeb 16 2018, 5:43 AM
kkretzsch marked 2 inline comments as done.

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!

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 accepted this revision.Feb 27 2018, 10:12 AM

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.

This revision is now accepted and ready to land.Feb 27 2018, 10:12 AM
Closed by commit rL326316: [IR] - Make User construction exception safe (authored by kkretzschmar, committed by ). · Explain WhyFeb 28 2018, 3:34 AM
This revision was automatically updated to reflect the committed changes.