[llvm-commits] [patch] Fix the inlining of invokes into invokes for libUnwind-like EH implementations

[John McCall]

On May 23, 2011, at 2:29 PM, Bill Wendling wrote:

> 
> On May 20, 2011, at 6:57 PM, John McCall wrote:
> 
>> I couldn't take it anymore, so I wrote up a dirty little patch to fix inlining through invokes.  The patch does two things:
>> 
>> 1) Calls (not invokes) of _Unwind_Resume in the inlined function are turned into branches to the unwind dest of the outer invoke.
>> 
>> This is required because we really want to be using _Unwind_Resume to resume after a cleanup, but libUnwind and the standard personality functions don't expect us to call _Unwind_Resume without handling the exception if the action table said we were going to handle it.  This is not the ideal solution to this problem — among other things, it requires every landing pad to be capable of correctly handling a cleanup action, even if only trivially — but it's an improvement over the current situation, which actually requires every landing pad to advertise itself as having a catch-all (and then actually handle that!).  The ideal solution is not really attainable while still gluing together EH CFGs.
>> 
>> 2) If a call to llvm.eh.selector can be found for the enclosing unwind destination, calls to llvm.eh.selector in the inlined function are augmented with the actions of the outer selector.
>> 
>> This allows the backend to assemble the correct set of unwind actions for a landing pad without gratuitous labor.
>> 
>> In no way is this patch a replacement for a proper revision of LLVM's EH IR:  the current IR is still awkward, challenging to optimize and lower to reasonable code, and almost inextricably tied to libUnwind-like unwinders and GCC-style personality functions.  But it eliminates the problems with spurious catch-alls that prevent the proper processing of uncaught exceptions, and it permits frontends to go back to emitting cleanups that call themselves cleanups  and  calls to _Unwind_Resume instead of _Unwind_Resume_or_Rethrow.
>> 
> Hi John,
> 
> This looks good, and it may supersede the slow code in DwarfEHPrepare.cpp that's trying to do similar things. A couple of things:
> 
> +static bool isUnwindResume(Value *value) {
>  ...
> +  if (fnType->isVarArg()) return false;
> 
> I think that the Ada front-end generates a call to _Unwind_Resume which takes more than one argument (and possibly a varargs).

I don't know how it would do that.

> +/// [LIBUNWIND] Check whether this selector is "only cleanups":
> +///   call i32 @llvm.eh.selector(blah, blah, i32 0)
> +static bool isCleanupOnlySelector(EHSelectorInst *selector) {
> +  if (selector->getNumArgOperands() != 3) return false;
> +  ConstantInt *val = dyn_cast<ConstantInt>(selector->getArgOperand(2));
> +  return (val && val->isZero());
> +}
> 
> This doesn't take 'filters' into account.

My understanding is that no component of a filter expression is never i32 0.  And a selector with a filter in it is not, in fact, a cleanup-only selector.  So how does this not take filters into account?

> +      // Otherwise, we just append the outer selector to the inner selector.
> +      SmallVector<Value*, 16> NewSelector;
> +      for (unsigned i = 0, e = Inner->getNumArgOperands(); i != e; ++i)
> +        NewSelector.push_back(Inner->getArgOperand(i));
> +      for (unsigned i = 2, e = Outer->getNumArgOperands(); i != e; ++i)
> +        NewSelector.push_back(Outer->getArgOperand(i));
> 
> This might not work 100% if there are filters and / or there is a catchall in one or both of the selectors. Basically, the llvm.eh.selector's structure will have to be interpreted for both selectors and reassembled.


I don't know what filters have to do with it — they're not semantically different from any other "handler", i.e. they can't be re-ordered (please tell me you're not re-ordering filters with respect to catches).  I'm willing to believe that the backend has constraints about whether cleanups can appear redundantly or something like that.

John.