[PATCH] CodeGen support for x86_64 SEH catch handlers in LLVM

[Andy Kaylor]

================
Comment at: lib/CodeGen/AsmPrinter/Win64Exception.cpp:132
@@ +131,3 @@
+/// are not really RTTI data, but pointers to filter functions that return an
+/// EXCEPTION_DISPOSITION (1, 0, or -1) to indicate how to handle the exception.
+///
----------------
This comment isn't quite correct.  EXCEPTION_DISPOSITION is an enumerated value:

  typedef enum _EXCEPTION_DISPOSITION \{
      ExceptionContinueExecution, // 0
      ExceptionContinueSearch, // 1
      ExceptionNestedException, // 2
      ExceptionCollidedUnwind // 3
  } EXCEPTION_DISPOSITION;

This is the type of the value returned by the MS runtime personality function.  The values you are referring to are just defined constants:

  #define EXCEPTION_EXECUTE_HANDLER       1
  #define EXCEPTION_CONTINUE_SEARCH       0
  #define EXCEPTION_CONTINUE_EXECUTION    -1

I made a mistake with this when I was experimenting with a custom personality function.  I couldn't understand why I had to return EXCEPTION_EXECUTE_HANDLER (1) to get the OS to skip my frame and find a handler lower on the stack.  It was because my personality function should have been returning ExceptionContinueSearch (1).  You are, of course, correct that the filter function needs to return 1, 0 or -1.

In any event, I think it would be worth having the comment name the constants here.  Their names are probably sufficient to document what they do.

================
Comment at: lib/CodeGen/AsmPrinter/Win64Exception.cpp:223
@@ +222,3 @@
+        assert(unsigned(TypeID - 1) < TypeInfos.size());
+        const GlobalValue *TI = TypeInfos[TypeID - 1];
+        if (TI)
----------------
rnk wrote:
> andrew.w.kaylor wrote:
> > Based on the comments above, this must be a function pointer, right?  I'm confused by the references to TypeID.
> Yes, it's a function pointer, but the existing code consistently refers to this operand of the landing pad catch clause as the "type info" and it is assigned a "type id" returned from @llvm.eh.typeid.for(i8* @my_type_info). More comments...
I'd be much happier if the code didn't refer to it as a type ID.  At the very least the variables here could call it what it is with an explanation of why it pulled a function poitner of of an array of TypeIDs.

================
Comment at: lib/CodeGen/AsmPrinter/Win64Exception.cpp:226
@@ +225,3 @@
+          Asm->OutStreamer.EmitValue(createImageRel32(Asm->getSymbol(TI)), 4);
+        else // catch i8* null
+          Asm->OutStreamer.EmitIntValue(0, 4);
----------------
rnk wrote:
> andrew.w.kaylor wrote:
> > rnk wrote:
> > > andrew.w.kaylor wrote:
> > > > Are there two different ways to specify a __finally handler?
> > > "catch i8* null" is a catch-all handler, not a `__finally` handler. The difference is that unwinding will continue through `__finally`, but catch-all handles the exception.
> > It looks like the same information is being written to the table in either case.  How does that work?
> This should be __finally:
>   try_begin_label
>   try_end_label
>   finally_callback
>   0 // zero for landing pad label
> 
> This should be catch all:
>   try_begin_label
>   try_end_label
>   0 // no filter function
>   landing_pad_label
> 
> Clear how that is different?
OK, that makes sense, though it isn't clear from reading the code that a ClauseLabel of zero is an expected occurance.

I guess my question above should have been, "Are there two different ways to specify a catch all handler?"  What I'm wondering about is the fact that if TypeID is zero the emitted filter/finally function is zero, and if TI (TypeInfos[TypeID - 1]) is zero the emitted filter/finally function is zero.  I would have thought one of these conditions shouldn't happen.

http://reviews.llvm.org/D6300