Add PNaClSjLjEH pass to implement C++ exception handling using setjmp()+longjmp()

Add PNaClSjLjEH pass to implement C++ exception handling using setjmp()+longjmp()

There are two parts to this:

 * PNaClSjLjEH.cpp expands out the "invoke", "landingpad" and "resume"
   instructions, modifying the control flow to use setjmp().

 * ExceptionInfoWriter.cpp lowers landingpads' clause lists to data
   that PNaCl's C++ runtime library will interpret.  This part will be
   reused when we drop the SjLj part and create a stable ABI for
   zero-cost EH.

This pass isn't enabled in PNaClABISimplify yet: I'll do that in a
separate change.

BUG=https://code.google.com/p/nativeclient/issues/detail?id=3696
TEST=*.ll tests (also tested end-to-end: plumbing for this will follow later)

Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-llvm.git;a=commit;h=f058041

[sehr (please use chromium), 2013-10-11 18:59:48]

I would like to review this before it is committed, but cannot do so today.
 Please wait until next week.

David


On Fri, Oct 11, 2013 at 10:32 AM, <mseaborn@chromium.org> wrote:

> Reviewers: Derek Schuff,
>
> Description:
> Add PNaClSjLjEH pass to implement C++ exception handling using
> setjmp()+longjmp()
>
> There are two parts to this:
>
>  * PNaClSjLjEH.cpp expands out the "invoke", "landingpad" and "resume"
>    instructions, modifying the control flow to use setjmp().
>
>  * ExceptionInfoWriter.cpp lowers landingpads' clause lists to data
>    that PNaCl's C++ runtime library will interpret.  This part will be
>    reused when we drop the SjLj part and create a stable ABI for
>    zero-cost EH.
>
> This pass isn't enabled in PNaClABISimplify yet: I'll do that in a
> separate change.
>
>
BUG=https://code.google.com/p/**nativeclient/issues/detail?id=**3696<https://code.google.com/p/nativeclient/issues/detail?id=3696>
> TEST=*.ll tests (also tested end-to-end: plumbing for this will follow
> later)
>
>
> Please review this at
https://codereview.chromium.**org/24777002/<https://codereview.chromium.org/2...
>
> SVN Base:
http://git.chromium.org/**native_client/pnacl-llvm.git@**master<http://git.ch...
>
> Affected files (+899, -0 lines):
>   M include/llvm/InitializePasses.**h
>   M include/llvm/Transforms/NaCl.h
>   M lib/Transforms/NaCl/**CMakeLists.txt
>   A lib/Transforms/NaCl/**ExceptionInfoWriter.h
>   A lib/Transforms/NaCl/**ExceptionInfoWriter.cpp
>   A lib/Transforms/NaCl/**PNaClSjLjEH.cpp
>   A test/Transforms/NaCl/pnacl-eh-**exception-info.ll
>   A test/Transforms/NaCl/pnacl-**sjlj-eh.ll
>   M tools/opt/opt.cpp
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "Native-Client-Reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to
native-client-reviews+**unsubscribe@googlegroups.com<native-client-reviews%2B...
> .
> To post to this group, send email to native-client-reviews@**
> googlegroups.com <native-client-reviews@googlegroups.com>.
> Visit this group at
http://groups.google.com/**group/native-client-reviews<http://groups.google.c...
> .
> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
> .
>

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at http://groups.google.com/group/native-client-reviews.
For more options, visit https://groups.google.com/groups/opt_out.

[Derek Schuff, 2013-10-15 17:39:25]

https://codereview.chromium.org/24777002/diff/23001/lib/Transforms/NaCl/Excep...
File lib/Transforms/NaCl/ExceptionInfoWriter.cpp (right):

https://codereview.chromium.org/24777002/diff/23001/lib/Transforms/NaCl/Excep...
lib/Transforms/NaCl/ExceptionInfoWriter.cpp:10: // The ExceptionInfoWriter class
converts the clauses of a
I do like the idea of starting with the top-down description. My problem was
that I read the header file first, and got the bottom-up description first
anyway. Maybe we could just put a note at the top of the header pointing to the
general description here.

https://codereview.chromium.org/24777002/diff/23001/lib/Transforms/NaCl/Excep...
lib/Transforms/NaCl/ExceptionInfoWriter.cpp:26: // ExceptionInfoWriter encodes
each clause as a 32-bit "clause ID".  A
maybe say "represents" instead of "encodes" (the way you do elsewhere below),
since "encodes" seems to imply that you pack all of the relevant information
into the 32-bit identifier.

More generally, it wasn't immediately clear to me that each clause in a
landingpad's list is uniquely identified by the pair of its list id (which is
unique to the landingpad instruction) and its clause id (which is the same as
other clauses which catch or filter the same types). It would almost be clearer
to call "clause ID" a "clause type ID" because it really identifies the *kind*
of clause rather than the clause itself. but of course "type" isn't a great word
to use in that context.

https://codereview.chromium.org/24777002/diff/23001/lib/Transforms/NaCl/Excep...
lib/Transforms/NaCl/ExceptionInfoWriter.cpp:112: report_fatal_error("Exception
type not a constant");
We've been using report_fatal_error for things that depends on user/developer
input or which might be conceivably reported to the user, but all the RFE's in
here are basically violations of invariants in the construction of the
EH-related IR, and would basically imply a broken module. It might make more
sense just to have them as asserts (and if the module verifier doesn't check
them, we can fix that too). That would also allow conversion of a bunch of
dyn_casts to casts.

https://codereview.chromium.org/24777002/diff/23001/lib/Transforms/NaCl/Excep...
lib/Transforms/NaCl/ExceptionInfoWriter.cpp:138: // Add 1 so that the empty list
can be represent as 0.
represent->represented

https://codereview.chromium.org/24777002/diff/23001/lib/Transforms/NaCl/Excep...
lib/Transforms/NaCl/ExceptionInfoWriter.cpp:200: if (OldGlobal) {
If there does not exist a variable by that name already, where is it actually
created?

https://codereview.chromium.org/24777002/diff/23001/lib/Transforms/NaCl/Excep...
File lib/Transforms/NaCl/ExceptionInfoWriter.h (right):

https://codereview.chromium.org/24777002/diff/23001/lib/Transforms/NaCl/Excep...
lib/Transforms/NaCl/ExceptionInfoWriter.h:53: ExceptionInfoWriter(LLVMContext
*Context);
i think it's LLVM style to always use explicit for 1-arg consructors.

https://codereview.chromium.org/24777002/diff/23001/lib/Transforms/NaCl/PNaCl...
File lib/Transforms/NaCl/PNaClSjLjEH.cpp (right):

https://codereview.chromium.org/24777002/diff/23001/lib/Transforms/NaCl/PNaCl...
lib/Transforms/NaCl/PNaClSjLjEH.cpp:58: //       MyException &exc =
*(MyException *) frame.result.exception;
frame.result.exception_obj

https://codereview.chromium.org/24777002/diff/23001/lib/Transforms/NaCl/PNaCl...
lib/Transforms/NaCl/PNaClSjLjEH.cpp:106: Instruction *EHStackTlsVar;  //
__pnacl_eh_stack thread-local variable
maybe mention that this is a bitcast, since it looks odd that it's an
Instruction instead of a GlobalVariable

[Mark Seaborn, 2013-10-15 21:41:17]

https://codereview.chromium.org/24777002/diff/23001/lib/Transforms/NaCl/Excep...
File lib/Transforms/NaCl/ExceptionInfoWriter.cpp (right):

https://codereview.chromium.org/24777002/diff/23001/lib/Transforms/NaCl/Excep...
lib/Transforms/NaCl/ExceptionInfoWriter.cpp:10: // The ExceptionInfoWriter class
converts the clauses of a
On 2013/10/15 17:39:26, Derek Schuff wrote:
> I do like the idea of starting with the top-down description. My problem was
> that I read the header file first, and got the bottom-up description first
> anyway. Maybe we could just put a note at the top of the header pointing to
the
> general description here.

Comment added to ExceptionInfoWriter.h.

https://codereview.chromium.org/24777002/diff/23001/lib/Transforms/NaCl/Excep...
lib/Transforms/NaCl/ExceptionInfoWriter.cpp:26: // ExceptionInfoWriter encodes
each clause as a 32-bit "clause ID".  A
On 2013/10/15 17:39:26, Derek Schuff wrote:
> maybe say "represents" instead of "encodes" (the way you do elsewhere below),
> since "encodes" seems to imply that you pack all of the relevant information
> into the 32-bit identifier.

Hmm, I thought "represent" would carry the same implication that you're
concerned about here. :-)

OK, changed to "represent".


> More generally, it wasn't immediately clear to me that each clause in a
> landingpad's list is uniquely identified by the pair of its list id (which is
> unique to the landingpad instruction)

No, none of these IDs are unique to specific instances of landingpad
instructions...

The same IDs can be used across converted landingpads.  Type IDs and clause list
IDs are currently interned (though filter IDs are not).


> and its clause id (which is the same as
> other clauses which catch or filter the same types). It would almost be
clearer
> to call "clause ID" a "clause type ID" because it really identifies the *kind*
> of clause rather than the clause itself. but of course "type" isn't a great
word
> to use in that context.

Not sure I understand what you've understood from this...

How about if I include an example like the following?

std::type_info *const __pnacl_eh_type_table[] = {
  &typeinfo(ExcA),  // defines type ID 0 == ExcA and clause ID 0 == "catch ExcA"
  &typeinfo(ExcB),  // defines type ID 1 == ExcB and clause ID 1 == "catch ExcA"
};

const uint32_t __pnacl_eh_filter_table[] = {
  0,   // refers to ExcA;  defines clause ID -1 as "filter [ExcA, ExcB]"
  1,   // refers to ExcB;  defines clause ID -2 as "filter [ExcB]"
  -1,  // list terminator; defines clause ID -3 as "filter []"
};

const struct action_table_entry __pnacl_eh_action_table[] = {
  // defines clause list ID 1:
  {
    0,  // "catch ExcA"
    2,  // else go to clause list ID 2
  },
  // defines clause list ID 2:
  {
    1,  // "catch ExcB"
    3,  // else go to clause list ID 3
  },
  // defines clause list ID 3:
  {
    -1,  // "filter [ExcA, ExcB]"
    0,  // end of list (no more actions)
  },
};

So if a landingpad contains the clause list:
  catch ExcA,
  catch ExcB
  filter [ExcA, ExcB]
this will be represented as clause list ID 1.

Maybe I should rename "clause list ID" to "action ID"?

https://codereview.chromium.org/24777002/diff/23001/lib/Transforms/NaCl/Excep...
lib/Transforms/NaCl/ExceptionInfoWriter.cpp:112: report_fatal_error("Exception
type not a constant");
On 2013/10/15 17:39:26, Derek Schuff wrote:
> We've been using report_fatal_error for things that depends on user/developer
> input or which might be conceivably reported to the user, but all the RFE's in
> here are basically violations of invariants in the construction of the
> EH-related IR, and would basically imply a broken module. It might make more
> sense just to have them as asserts (and if the module verifier doesn't check
> them, we can fix that too). That would also allow conversion of a bunch of
> dyn_casts to casts.

Not sure I agree.  It's inelegant for the module verifier to have to check usage
of miscellaneous intrinsics.  I'd hope the module verifier only has to check
core IR features (instructions).  The purpose of having intrinsics is to make IR
extensible without having the extend core code (such as the module verifier).

It also might be slower to have checks in the module verifier than at the point
where intrinsics are used.

The module verifier generally isn't run between passes, so it wouldn't protect
us from bad output by (e.g.) an optimisation pass.

ExpandConstantExpr is an example of a pass that would make this check fail.

https://codereview.chromium.org/24777002/diff/23001/lib/Transforms/NaCl/Excep...
lib/Transforms/NaCl/ExceptionInfoWriter.cpp:138: // Add 1 so that the empty list
can be represent as 0.
On 2013/10/15 17:39:26, Derek Schuff wrote:
> represent->represented

Done.

https://codereview.chromium.org/24777002/diff/23001/lib/Transforms/NaCl/Excep...
lib/Transforms/NaCl/ExceptionInfoWriter.cpp:200: if (OldGlobal) {
On 2013/10/15 17:39:26, Derek Schuff wrote:
> If there does not exist a variable by that name already, where is it
> actually created?

This code is expecting an external reference to __pnacl_eh_type_table, etc.  It
resolves the external reference.

I've added a check to give an error if the variable is not an external reference
(if it has an initialiser).

https://codereview.chromium.org/24777002/diff/23001/lib/Transforms/NaCl/Excep...
File lib/Transforms/NaCl/ExceptionInfoWriter.h (right):

https://codereview.chromium.org/24777002/diff/23001/lib/Transforms/NaCl/Excep...
lib/Transforms/NaCl/ExceptionInfoWriter.h:53: ExceptionInfoWriter(LLVMContext
*Context);
On 2013/10/15 17:39:26, Derek Schuff wrote:
> i think it's LLVM style to always use explicit for 1-arg consructors.

Done.  I always forget about this C++ gotcha.

https://codereview.chromium.org/24777002/diff/23001/lib/Transforms/NaCl/PNaCl...
File lib/Transforms/NaCl/PNaClSjLjEH.cpp (right):

https://codereview.chromium.org/24777002/diff/23001/lib/Transforms/NaCl/PNaCl...
lib/Transforms/NaCl/PNaClSjLjEH.cpp:58: //       MyException &exc =
*(MyException *) frame.result.exception;
On 2013/10/15 17:39:26, Derek Schuff wrote:
> frame.result.exception_obj

Done.

https://codereview.chromium.org/24777002/diff/23001/lib/Transforms/NaCl/PNaCl...
lib/Transforms/NaCl/PNaClSjLjEH.cpp:106: Instruction *EHStackTlsVar;  //
__pnacl_eh_stack thread-local variable
On 2013/10/15 17:39:26, Derek Schuff wrote:
> maybe mention that this is a bitcast, since it looks odd that it's an
> Instruction instead of a GlobalVariable

Done.

[Derek Schuff, 2013-10-15 22:36:19]

lgtm

https://codereview.chromium.org/24777002/diff/23001/lib/Transforms/NaCl/Excep...
File lib/Transforms/NaCl/ExceptionInfoWriter.cpp (right):

https://codereview.chromium.org/24777002/diff/23001/lib/Transforms/NaCl/Excep...
lib/Transforms/NaCl/ExceptionInfoWriter.cpp:26: // ExceptionInfoWriter encodes
each clause as a 32-bit "clause ID".  A
On 2013/10/15 21:41:18, Mark Seaborn wrote:
> Hmm, I thought "represent" would carry the same implication that you're
> concerned about here. :-)
Yeah, after seeing it the other way, it doesn't really seem any better or worse.
Your call.

And yes, the example is helpful.

> Maybe I should rename "clause list ID" to "action ID"?

I'm not sure action ID is any better, since there's not a really good idea of
what an action is. clause list ID at least makes sense in the context of the
landingpad instructions.

[Mark Seaborn, 2013-10-15 22:57:05]

Since you asked me what interprets the tables that ExceptionInfoWriter.cpp
produces, have a look at the corresponding libstdc++ change:
https://codereview.chromium.org/27341005/diff/1/libstdc++-v3/libsupc++/eh_pna...
(see __pnacl_eh_sjlj_Unwind_RaiseException()).  I haven't cleaned up that change
yet, though.

[Mark Seaborn, 2013-10-15 23:08:57]

https://codereview.chromium.org/24777002/diff/23001/lib/Transforms/NaCl/Excep...
File lib/Transforms/NaCl/ExceptionInfoWriter.cpp (right):

https://codereview.chromium.org/24777002/diff/23001/lib/Transforms/NaCl/Excep...
lib/Transforms/NaCl/ExceptionInfoWriter.cpp:26: // ExceptionInfoWriter encodes
each clause as a 32-bit "clause ID".  A
On 2013/10/15 22:36:20, Derek Schuff wrote:
> On 2013/10/15 21:41:18, Mark Seaborn wrote:
> > Hmm, I thought "represent" would carry the same implication that you're
> > concerned about here. :-)
> Yeah, after seeing it the other way, it doesn't really seem any better
> or worse. Your call.

OK, I've changed it back to "encode".

> And yes, the example is helpful.

I've added it to the comment in the code.  Please take a look.

> > Maybe I should rename "clause list ID" to "action ID"?
> 
> I'm not sure action ID is any better, since there's not a really good idea of
> what an action is. clause list ID at least makes sense in the context of the
> landingpad instructions.

OK.

[Mark Seaborn, 2013-10-16 17:59:52]

As discussed, I've added a second filter to the example in the comments, and
fixed __pnacl_eh_filter_table[] to be signed.

[Derek Schuff, 2013-10-16 18:16:22]

thanks, it's definitely better. We should probably add this to the design doc as
well
LGTM

[Mark Seaborn, 2013-10-16 20:06:29]

Committed patchset #15 manually as rf058041 (presubmit successful).

[sehr, 2013-10-16 20:52:57]

On 2013/10/16 18:16:22, Derek Schuff wrote:
> thanks, it's definitely better. We should probably add this to the design doc
as
> well
> LGTM

I agree we should add this to the design document now and use that for future
discussions of zero-cost exceptions, etc.
Otherwise, LGTM also.