[llvm-commits] [Patch] Exception Handling Documentation

Bill Wendling wendling at apple.com
Tue Aug 2 14:29:32 PDT 2011


On Aug 2, 2011, at 1:24 PM, Chris Lattner wrote:

> @@ -3372,17 +3384,18 @@
>    successors.</p>
> 
> <h5>Arguments:</h5>
> -<p>The '<tt>resume</tt>' instruction's argument must have the same type as the
> -   result of any '<tt>landingpad</tt>' instruction in the same function.</p>
> +<p>The '<tt>resume</tt>' instruction requires one argument, which must have the
> +   same type as the result of any '<tt>landingpad</tt>' instruction in the same
> +   function.</p>
> 
> Thanks for mentioning this.  As a devil's advocate question: shouldn't this be forced to match the landingpad instruction that interrupted the unwind?  isn't it theoretically possible to have code like this:
> 
> 
>  invoke @somefunc  -> unwind %landingpadbb
> 
> landingpadbb:
>  %A = landingpad ...
>  call foo(%A)
>  unreachable
> 
> void foo( ... %A) {
>  resume %A
> }
> 
> In other words, does the resume actually have to be in the same function as the landing pad?
> 
Interesting. No, it doesn't need to be in the same function. So I'll change the wording.

> +<h5>Syntax:</h5>
> +<pre>
> +  <resultval> = landingpad <somety> personality <type> <pers_fn> cleanup? <clause>+
> +  <clause> := catch <type> <value> {, <type> <value>}*
> +  <clause> := filter <type> <value> {, <type> <value>}*
> 
> 
> I thought we dropped "cleanup" for now.  Also, it doesn't make sense to me to allow both lists of clauses and lists of values in each clause.  Is there any reason not to simplify this down to:
> 
> +  <resultval> = landingpad <somety> personality <type> <pers_fn><clause>+
> +  <clause> := catch <type> <value>
> +  <clause> := filter <type> <value>
> 
> ?
> 
It's a bit more verbose, but I have no real objections to it.

> The optional
> +   <tt>cleanup</tt> flag indicates that the landing pad block is a cleanup.</p>
> 
> Again, I thought we were leaving this out of the first cut.
> 
I think you may be thinking of the 'terminate' clause? The 'cleanup' bit needs to be here for correct EH semantics.

> 
> +  <li>A landing pad blcok must have a '<tt>landingpad</tt>' instruction as its
> +      first non-PHI instruction.</li>
> +  <li>The '<tt>landingpad</tt>' instruction must be the first non-PHI
> +      instruction in the landing pad block.</li>
> 
> typo "blcok".  It's not clear to me what the difference between these two points is.
> 
The second is restricting the placement of the landingpad instruction. The first is restricting how the landing pad block is set up. It would prevent this, admittedly broken, landing pad block:

lpad:
  %exn = landingpad ...
  %exn1 = landingpad ...

Though this may be overkill?  

> 
> +  <li>Like indirect branches, splitting the critical edge to a landing pad block
> +      requires considerable care, and <tt>SplitCriticalEdge</tt> will refuse to
> +      do it.</li>
> 
> This shouldn't be in LangRef.html
> 
Okay.

> </div>
> </div>
> +</div>
> 
> Something seems wrong here.  Why so many /div's?
> 
I looked carefully and it looks like the 'divs' really are nested three deep. *shrug*

> Otherwise this looks great, please commit with the changes above if you agree.
> 
The latest draft is attached. The only issues that remain are the 'cleanup' and the 'restrictions'. If you are okay with them, I can commit the changes.

Thanks for the review!

-bw

-------------- next part --------------
A non-text attachment was scrubbed...
Name: eh.1.2.lp.diff
Type: application/octet-stream
Size: 5656 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20110802/59c58397/attachment.obj>
-------------- next part --------------




More information about the llvm-commits mailing list