This is an archive of the discontinued LLVM Phabricator instance.

Make llvm.eh.begincatch use an outparam
ClosedPublic

Authored by rnk on Feb 26 2015, 3:22 PM.

Details

Summary

Ultimately, __CxxFrameHandler3 needs us to put a stack offset in a
table, and it will take responsibility for copying the exception object
into that slot. Modelling the exception object as an SSA value returned
by begincatch isn't going to work in general, so make it use an output
parameter.

Diff Detail

Repository
rL LLVM

Event Timeline

rnk updated this revision to Diff 20799.Feb 26 2015, 3:22 PM
rnk retitled this revision from to Make llvm.eh.begincatch use an outparam.
rnk updated this object.
rnk added a reviewer: andrew.w.kaylor.
rnk added a subscriber: Unknown Object (MLST).
andrew.w.kaylor edited edge metadata.EditedFeb 27 2015, 10:46 AM

I don't think this is necessary.

I've got an extra level of indirection that shouldn't be there in my current handling of the exception object, but I think the basic idea is sound. My intention is to assign a fixed spot (element 1) in the frame allocation block to the exception object and then bitcast it to the correct type in the handler. The llvm.eh.begin catch call tells me where the handler code wants to access the exception object and the bitcasts are already being generated.

So like I said, what the currently committed code is doing is wrong, but it's pretty close. Here's what we are currently generating for the catch handler in cppeh-catch-scalar:

define i8* @_Z4testv.catch(i8*, i8*) {
catch.entry:
  %eh.alloc = call i8* @llvm.framerecover(i8* bitcast (void ()* @_Z4testv to i8*), i8* %1)
  %eh.data = bitcast i8* %eh.alloc to %struct._Z4testv.ehdata*
  %eh.obj.ptr = getelementptr inbounds %struct._Z4testv.ehdata* %eh.data, i32 0, i32 1
  %eh.obj = load i8** %eh.obj.ptr
  %i = getelementptr inbounds %struct._Z4testv.ehdata* %eh.data, i32 0, i32 2
  %2 = bitcast i8* %eh.obj to i32*
  %3 = load i32* %2, align 4
  store i32 %3, i32* %i, align 4
  %4 = load i32* %i, align 4
  call void @_Z10handle_inti(i32 %4)
  ret i8* blockaddress(@_Z4testv, %try.cont)
}

The GEP for %i is an unnecessary artifact as %i is not (and cannot be) referenced outside of the catch handler. Also, there is a load-store-load sequence that confuses things but is just an artifact of having started with unoptimized IR. The big issue, however, is that the indirection with %eh.obj.ptr shouldn't be there. I didn't have that originally and then I guess one day I wasn't thinking clearly and convinced myself that I needed it. Without that, I think you can see how this ought to work with the existing form of llvm.eh.begincatch.

Here is what I think we should be generating:

define i8* @_Z4testv.catch(i8*, i8*) {
catch.entry:
  %eh.alloc = call i8* @llvm.framerecover(i8* bitcast (void ()* @_Z4testv to i8*), i8* %1)
  %eh.data = bitcast i8* %eh.alloc to %struct._Z4testv.ehdata*
  ; "%4 = call i8* @llvm.eh.begincatch(i8* %exn11)" gets mapped to the instruction below
  %eh.obj = getelementptr inbounds %struct._Z4testv.ehdata* %eh.data, i32 0, i32 1
  %2 = bitcast i8* %eh.obj to i32*
  %i = load i32* %2, align 4
  call void @_Z10handle_inti(i32 %i)
  ret i8* blockaddress(@_Z4testv, %try.cont)
}

I need to do some experimentation to see what happens when the exception object is bigger than a pointer size, but I'm fairly certain that this model can handle that just by putting some padding in the frame allocation structure.

Note also that I am intentionally using the same stack location for the exception object in all catch handlers. I can't see any advantage to not doing so.

In Clang's current IR, what information would you use to size the exception object slot? Currently it looks a bit like this:

; 'catch (A e)' where A is an aggregate.
%e.addr = alloca %struct.A
...
%ehobj = call i8* @llvm.eh.begincatch(i8* %exn)
%ehobj.A = bitcast i8* %ehobj to %struct.A*
call void @A_copy_ctor(%struct.A* %e.addr, %struct.A* %ehobj.A)
... ; catch body
call void @A_dtor(%struct.A* %e.addr)

We could use the bitcast to figure out the size needed. You’re right that the runtime handles the calling of the copy constructor in this case.

What about this case?

void f() {

try {
  might_throw();
} catch (int e1) {
  try {
    might_throw2();
  } catch (int e2) {
    use_both_exceptions(e1, e2);
  }
}

}

I think we need distinct storage for e1 and e2.

I suppose you’re right.

I’ve been doing a lot of poking around in the debugger to figure out how existing compilers support this stuff. I don’t know if maybe I’ve been inconsistent in the optimization levels I’ve used, but what I’ve seen is that the Microsoft compiler has used different stack slots for each catch handler in the cases I’ve looked at while the Intel compiler has put everything in the same stack slot. I’ll run your test case above through ICC to see what it does. I’m guessing that it will probably use two different stack slots.

So, this is leading me to think that maybe your proposed change to llvm.eh.begincatch is useful. Have you talked to John McCall about this? As I recall he had some ideas about possibly wanting to use the llvm.eh.begincatch intrinsic for non-Windows purposes somewhere down the road.

… from a "throw information" table that David is currently looking at.

Does this mean that David is working on generating the .xdata tables? If so, I’m OK with that. I’m not sure if I’ve shared everything I know about that, but I’ve got extensive notes from research I’ve done to this point. I’d be happy to offer assistance.

On the other hand, if I’m reading too much into the comment, that’s fine too. I’ve actually been planning to do the .xdata table generation myself.

-Andy

From: llvm-commits-bounces@cs.uiuc.edu [mailto:llvm-commits-bounces@cs.uiuc.edu] On Behalf Of Reid Kleckner
Sent: Friday, February 27, 2015 11:11 AM
To: reviews+D7920+public+4a2276adf4c6c5f0@reviews.llvm.org
Cc: llvm-commits
Subject: Re: [PATCH] Make llvm.eh.begincatch use an outparam

I think this is a reasonable approach. I've noted a couple of places in the tests where artifacts of the earlier use of llvm.eh.begin catch are still hanging around, but I think it's fine if you want to just leave that for a later revision that strips the code that is generating those lines. I can even roll that into one of my in-flight change sets if this is going to land before those do.

test/CodeGen/X86/cppeh-catch-scalar.ll
65 ↗(On Diff #20799)

Any reason that the bitcast isn't done in the call line?

90 ↗(On Diff #20799)

This should go away.

test/CodeGen/X86/cppeh-frame-vars.ll
184 ↗(On Diff #20799)

This should go away.

andrew.w.kaylor accepted this revision.Mar 2 2015, 12:56 PM
andrew.w.kaylor edited edge metadata.
This revision is now accepted and ready to land.Mar 2 2015, 12:56 PM
This revision was automatically updated to reflect the committed changes.
rnk added inline comments.Mar 3 2015, 2:00 PM
test/CodeGen/X86/cppeh-catch-scalar.ll
65 ↗(On Diff #20799)

That only happens for bitcasts of constants. Typically pointer bitcasts of global objects get that.

90 ↗(On Diff #20799)

True, I guess I should do a follow-up for that.