Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inconsistency in documentation for try-delegate behavior for function body #176

Closed
takikawa opened this issue Jul 15, 2021 · 38 comments · Fixed by #178
Closed

Inconsistency in documentation for try-delegate behavior for function body #176

takikawa opened this issue Jul 15, 2021 · 38 comments · Fixed by #178

Comments

@takikawa
Copy link
Collaborator

takikawa commented Jul 15, 2021

In #175 there's some discussion of what the intended behavior of try-delegate with a label pointing to the function body is supposed to be (#175 (comment)).

My understanding was it should be valid and rethrow the exception out of the function, as discussed in #146 (see example there, especially for delegate 3) and as covered in the core tests in this test case which I'll just inline below:

  (func (export "delegate-to-caller")
    (try (do (try (do (throw $e0)) (delegate 1))) (catch_all))
  )

There's an inconsistency though with the overview in https://github.com/WebAssembly/exception-handling/blob/master/proposals/exception-handling/Exceptions.md as it says:

When the specified label within a delegate instruction does not correspond to a try instruction, it is a validation failure.

Assuming we agree on the behavior, we could change the overview to say "does not correspond to a try instruction or function body" (and an example could be helpful) and possibly adjust some other prose.

@aheejin
Copy link
Member

aheejin commented Jul 15, 2021

I strongly support (and I've been assuming that was the case) the 'caller' case for delegate. Actually, the toolchain has been using this quite a lot already; if it is not supported I guess we have to wrap all functions that are using this with a big try-catch-rethrow-end. I imagined the caller concept not as a 'function block' but more as 'outside of this function', so it will delegate the handling to the caller.

aheejin added a commit to aheejin/exception-handling that referenced this issue Jul 16, 2021
This clarifies that `delegate` can target the function-level block,
which means the exception is thrown to the caller. Also added more
examples and shuffled some paragraphs around to make them more sense.

Fixes WebAssembly#176.
aheejin added a commit to aheejin/exception-handling that referenced this issue Jul 16, 2021
This clarifies that `delegate` can target the function-level block,
which means the exception is thrown to the caller. Also added a few more
examples.

Fixes WebAssembly#176.
aheejin added a commit to aheejin/exception-handling that referenced this issue Jul 16, 2021
This clarifies that `delegate` can target the function-level block,
which means the exception is thrown to the caller. Also added a few more
examples.

AFAIK this is consistent with the current implementations in the
toolchain, V8, and Spidermonkey.

Fixes WebAssembly#176.
aheejin added a commit to aheejin/exception-handling that referenced this issue Jul 16, 2021
This clarifies that `delegate` can target the function-level block,
which means the exception is thrown to the caller. Also added a few more
examples.

AFAIK this is consistent with the current implementations in the
toolchain, V8, and Spidermonkey.

Fixes WebAssembly#176.
@rossberg
Copy link
Member

Hm, to be honest, I would highly prefer to not make that change. The function-level block scope was always intended to be just a shorthand for saving a block instruction. This change would make it into a very special case, something new that is neither a regular block nor a try, which is the kind of one-off semantics that makes me uncomfortable. For example, it is non-composable, and breaks the ability to inline functions without ad-hoc transformations.

If we allow this then we should allow targeting any block with a delegate.

Is there a concrete use case for making this block special?

@conrad-watt
Copy link
Contributor

conrad-watt commented Jul 19, 2021

This change would make it into a very special case, something new that is neither a regular block nor a try

If we allow this then we should allow targeting any block with a delegate.

We already define entering a function as introducing a label that breaks to the end of the function body (we could have instead required the function body to be explicitly wrapped in an outer block for this functionality) - could we support the desired semantics in the OP by enriching this label to be a try block with empty catch?

@rossberg
Copy link
Member

Indeed, this is exactly like a catchless try. So we could handle it in the semantics by reducing a function call to frame(label(catch(...))). But of course, that is still a complication that potentially affects tools as well. So I'd still be curious if there actually is a strong use case for it.

@tlively
Copy link
Member

tlively commented Jul 19, 2021

A delegate may be necessary to tunnel through one or more trys to reach the top-level function scope in essentially all the same situations where it may be necessary to reach an outer try scope. It seems simpler to me to inline functions as catchless try's than to further complicate the delegate insertion algorithm by making the function scope a special case.

@aheejin
Copy link
Member

aheejin commented Jul 19, 2021

If targeting the function level block with delegate is disallowed, in the current spec, we can work around it by introducing a big try-catch_all-end wrapping the whole function and rethrow within that catch_all body. But this will not work if we introduce 2PEH: the first search phase cannot access the bodies of catch/catch_all, but only can follow catchs and delegates to search for the next catch to handle the given exception. It was described in detail in "Incompatibility of first-class exnref and two-phase unwinding" section in #123; we don't have exnref now, but the problem still remains that we don't run the bodies of catches in the first phase.

I'm not sure if we will actually end up doing 2PEH, but given that one of the reason delegate was introduced was at least to be compatible with it in case we need it later, I'm concerned if we preclude the possibility now.

I think considering it as a catchless try is a good way to interpret it.

@rossberg

So I'd still be curious if there actually is a strong use case for it.

There are functions that we don't have complete info on if it ever throws in the toolchain backend. But they can be happened to be wrapped into a try-catch. For example:

(func $test
  try
    ...
    call $foo  ;; if throws, should go to `catch` below
    ...
    call $bar  ;; if throws, should NOT go to `catch` below
    ...
  catch
    ...
  end
end)

In LLVM IR terms, call $foo would have been an invoke instruction, which has a destination landing pad (catch here). But call $bar is just a plain call instruction that does not have a destination landing pad. But we cannot guarantee it does not throw in the backend. And if it ever throws, because its destination is not catch, it should throw to the caller. We use "delegate to caller" in this case so that we wrap call $bar with try-delegate N where N is an immediate for the function-level block.

@RossTate
Copy link
Contributor

But this will not work if we introduce 2PEH: the first search phase cannot access the bodies of catch/catch_all, but only can follow catchs and delegates to search for the next catch to handle the given exception. It was described in detail in "Incompatibility of first-class exnref and two-phase unwinding" section in #123; we don't have exnref now, but the problem still remains that we don't run the bodies of catches in the first phase.

The design of catch_all (as opposed to unwind) is also incompatible with 2PEH, as was also discussed in #123.

@aheejin
Copy link
Member

aheejin commented Jul 19, 2021

@RossTate You are right when we use catch_all for cleanups. But if we add unwind again in 2PEH, replacing uses of catch_all for cleanups with unwind is a simple task, and it doesn't mean the fundamentals of the current proposal have to change. I think maybe "extensibility" is a better word, and that's also more in line with our past discussions. "Compatibility" doesn't mean we can run the MVP binary on 2PEH proposal, so it can be misleading.

The reason we removed unwind in this MVP proposal is we don't have 2PEH yet and there's not much meaningful difference between catch_all and unwind without 2PEH. So I think the current proposal with catch_all is still extensible to a future 2PEH proposal.

But disallowing delegate to caller in MVP and allowing it in 2PEH sounds weirder. Adding unwind is an addition (or extension) to the current propsoal, but allowing delgate to caller later is more about changing the semantics of an existing instruction, which I prefer not to do.

@rossberg
Copy link
Member

@tlively:

A delegate may be necessary to tunnel through one or more trys to reach the top-level function scope in essentially all the same situations where it may be necessary to reach an outer try scope. It seems simpler to me to inline functions as catchless try's than to further complicate the delegate insertion algorithm by making the function scope a special case.

I see, makes sense. But of course, a producer could equally well just insert a catchless try explicitly in such cases. That would put the burden on the producer actually needing this functionality rather than everybody in the larger ecosystem?

I wonder, is there appetite for resolving this tension by simply removing the restriction that delegate can only target try-blocks? In other words, treat every block like a catchless try? That would avoid making an odd special case here. Given that engines already have to handle that case, I presume it wouldn't add particular difficulties to allow it everywhere.

@tlively
Copy link
Member

tlively commented Jul 21, 2021

Yes, I suppose the "special case" for tunneling to the function scope would be one additional catchless try, which sounds simple enough.

I wonder, is there appetite for resolving this tension by simply removing the restriction that delegate can only target try-blocks? In other words, treat every block like a catchless try?

Yes, personally I like this idea. It changes the mental model for try-delegate from delegating to a particular (potentially empty) set of handlers to delegating the exception propagation to any outer scope. try-delegate is primarily used to "tunnel" exceptions through enclosing try-catch scopes, so I think being able to target any block seems reasonable for that tunneling.

@conrad-watt
Copy link
Contributor

conrad-watt commented Jul 21, 2021

If we were to generalise delegate in this way, is there any remaining difference between a block and a catchless try?

Or put another way, could engines/tools get any benefit from knowing upfront (streaming?) that a block scope can't be delegated to (which IIUC is currently exactly the difference between block and catchless try)?

@rossberg
Copy link
Member

Delegation is lexical, so whether a block is being targeted is a simple syntactic property either way.

takikawa added a commit to takikawa/exception-handling that referenced this issue Aug 9, 2021
Accounting for the discussion in

  WebAssembly#176

adjust try_delegate so that it can target any block.
@takikawa
Copy link
Collaborator Author

takikawa commented Aug 9, 2021

It seems like there have been multiple solutions proposed here and a consensus wasn't reached quite yet, so I wanted to summarize the options.

  • Only allow delegation to try blocks and require producers to insert a catchless try in the function top-level to get the desired function delegation semantics.
  • Allow delegation to try blocks and the function top-level (which is then considered a catchless try in the semantics) --- the current proposed text in Clarify delegate targetting function-level block #177 matches this I think and should require little to no change in engines.
  • Generalize try-delegate to allow targeting any block (with similar semantics to delegating to a catchless try). I've updated [interpreter] Add support for validation and evaluation for exceptions #175 (interpreter) with this proposal (44e8abd). It's straightforward for the semantics but engines will need to change a bit.

Any others I missed? Is there agreement on proceeding with one of these options?

@aheejin
Copy link
Member

aheejin commented Aug 11, 2021

Sorry for the delayed reply. I don't think I have a specific preference here. Option 2 is the currently implemented one in the toolchain and the V8 & FF engines. But I admit that has some inconsistency to it. I think I am OK with any options here. Any more opinions on what you prefer or not prefer among these? If we allow delegate target any blocks, would that make any optimization or code transformation possibly harder?

And whichever one we choose, we should coordinate when we land the change in V8 and the toolchain not to break Adobe origin trial.

@rossberg
Copy link
Member

Personally, I'd prefer option 3, since it is the most flexible and most regular. Thanks @takikawa for trying it in the PR, LGTM.

@takikawa
Copy link
Collaborator Author

takikawa commented Aug 12, 2021

Any more opinions on what you prefer or not prefer among these?

I don't have a strong preference but I agree that option 3 seems the most regular.

If we allow delegate target any blocks, would that make any optimization or code transformation possibly harder?

I just tried implementing the change in SpiderMonkey's baseline compiler and it seems like it should be pretty straightforward, and with minimal overhead as far as I can tell (since it can be statically determined if the delegation to a non-try block skips to an outer try block or rethrows out of the function). I'm not sure if there any implications on the producer side.

@takikawa
Copy link
Collaborator Author

It seems like there are no opinions against moving forward with option 3 so far and some explicit support. @thibaudmichaud do you have any concerns with implementing option 3 in V8 or have any preferences for any of the others?

@conrad-watt
Copy link
Contributor

conrad-watt commented Aug 17, 2021

I have a weak preference against option 3, and in favour of option 2, simply because it feels strange to me to remove the remaining distinction between block and catchless try and cause EH considerations to bleed into MVP control flow constructs. I think of delegate as tunnelling to a catch handler(s) (and of try as implicitly having something like a trailing catch-rethrow for unhandled exceptions), so it doesn't feel irregular to me that delegate should be prevented from targetting a block.

EDIT: I also think that changing the function-level block to a catchless try wouldn't be irregular - the outer block was already merely a code-size convenience.

That being said, I won't hold to this argument if there would otherwise be consensus for option 3.

@tlively
Copy link
Member

tlively commented Aug 18, 2021

Maybe we can do a little straw poll to see where we stand:

😄 Option 1
🎉 Option 2
❤️ Option 3
👀 No strong opinion

@lars-t-hansen
Copy link
Contributor

FWIW, echoing @conrad-watt and also giving a bump to @aheejin's question about whether allowing the targeting of any block will make code transformations or optimizations any harder, option 2 seems more conservative (MVP-y), sufficiently flexible, and forces there to be more static information. Clearly it's possible to detect whether a block is targeted by delegate, so it's a minor consideration, but one-pass compilers, interpreters, and suchlike may in principle benefit from the extra information.

@aheejin
Copy link
Member

aheejin commented Aug 18, 2021

What @lars-t-hansen said was the reason I was cautious about Option 3. I was concerned about the possibility that the "bleeding" of exception through blocks can make code transformation tricky (even though I couldn't come up with a concrete example), and the other thing was, given that delegate was introduced to delegate an exception to a specific try, if it's targeting a block or loop, it is most likely to be an unintended bug anyway. But I also understand the inconsistency argument as well, which was why I am not too opinionated on one option.

While I don't have a strong opinion on one option, Option 1 can be a middle ground; it is consistent in that there's no exception for the function-level block. It will increase the instruction count (and thus the code size) by two instruction (try and end) for the functions that use it but the increase will be marginal at best and in most cases unnoticeable.

@conrad-watt
Copy link
Contributor

conrad-watt commented Aug 19, 2021

I should have mentioned option 1 in my previous post. To explicitly give my opinion now, I think it would strictly inferior to option 2, since it would require an almost identical type system extension (to track whether a label target can be delegated to). Generalising the function-level block to be a catchless try (i.e. going from 1 to 2) really shouldn't be an invasive change.

I would be fine with 2 or 3, with a slight preference for 2.

@takikawa
Copy link
Collaborator Author

takikawa commented Aug 23, 2021

So it seems like when factoring in @rossberg's preference for option 3, it's evenly tied between 2 and 3 in the straw poll above and there's not really interest in doing 1. Anyone else have any thoughts to tilt it either way?

For completeness sake, implementing option 2 in the reference interpreter (and thus the formal semantics) should look like this: takikawa@7cebbb1 The special handling of the body block is eliminated and it's treated as having a TryLabel in validation. Option 3 just removes TryLabel instead (44e8abd).

@ioannad
Copy link
Collaborator

ioannad commented Aug 26, 2021

Sorry for the late response, i was on long holidays.

I don't mind either option. I also think option 2 seems simpler to implement¹, but for the spec it's just a matter of validation I think, and of course a matter of simplicity as @rossberg pointed out.

Having said that, I don't really understand @conrad-watt's spec argument against option 3, because in my mind² try-delegate does a (re)throw from the body of its target (try) block anyway, instead of tunnelling the exception to the catches of its target.

But again, either option is fine by me.


¹. In fact option 2 is what RabaldrMonkey is doing already, and @lars-t-hansen's comment seems to suggest option 3 would have optimization implications there. In the in-progress BaldrMonkey implementation, option 3 could be done with throws from the target blocks, which was the plan anyway. But I don't know whether with option 3 there might be (future) optimizations that could become tricky there as well.

². This view is what is done in the spec formal-overview draft #143, which btw in fact still only implements option 1.

@rossberg
Copy link
Member

Option 2 is the one I like least, since it (a) is the least regular, and (b) puts more burden on the wider ecosystem instead of just users/producers of the feature. Option 1 and 3 avoid both these disadvantages, and option 3 even avoids any burden on the user side.

@conrad-watt, I also don't understand your argument. The fact that the empty form of one construct is equivalent to another construct is a common situation – for example, an empty block is just a nop, an empty br_table is just a br, and so on. Why should this be an argument for introducing irregularities?

@conrad-watt
Copy link
Contributor

conrad-watt commented Aug 26, 2021

My reasoning is coming from a different direction, since I don't see option 2 as causing any irregularities compared to either of the other options. So to me, the only remaining tie-breakers are marginal:

  • code size savings compared to option 1
  • stronger conceptual separation between the MVP control flow constructs and try compared to option 3 (possible manifestation with reference to @lars-t-hansen's comment: try might need additional eager book-keeping, especially in streaming compilers).

I agree that if I believed there were irregularities introduced by option 2, this would trump any of the above. I'm also happy enough with option 3 that I don't want to turn this into a big debate.

@rossberg
Copy link
Member

Option 2 is conflating functions with exception handling. That is, the irregularity is that it requires introducing a special case of block kind only for function bodies (as was done in @takikawa's original interpreter implementation), or type-checking a function body as if it was a handler (although it never is).

Furthermore, as mentioned above, this conflation affects relevant program equivalences and puts a burden on everybody who wants to do certain program transformations. In fact, it breaks existing inlining transformations, because they become incorrect for certain functions. I think such an avoidable lack of conservatism and orthogonality isn't justified by the minor convenience it provides for the users of this feature (who can trivially insert a try on their end under option 1, or don't even have to under option 3).

@conrad-watt
Copy link
Contributor

the irregularity is that it requires introducing a special case of block kind only for function bodies (as was done in @takikawa's original interpreter implementation),

I agree that introducing a special block case just for function bodies would be undesirable. If this was the only way to spec this behaviour, I would be against it.

or type-checking a function body as if it was a handler (although it never is)

We get to decide in the spec whether the implicit outer label of a function is a handler, in the same way that we decided that there should be an implicit outer label in the first place. We could say that it is a handler, in the same way that a catchless try is.

I can't give an informed opinion wrt. the concerns for toolchains and producers. I just think that at a semantic level, solution 2 isn't irregular.

@rossberg
Copy link
Member

rossberg commented Aug 26, 2021

We could say that it is a handler, in the same way that a catchless try is.

A catchless try is merely a degenerate case of a more general construct. The function body would always be such a degenerate case, so would be rather special. I don't think these situations are comparable.

@ioannad
Copy link
Collaborator

ioannad commented Aug 26, 2021

Even though nobody has explicitly voted for option 1, it does seem the most conservative, and it's still extensible to option 2 or 3, if we later decide that we would like to add more flexibility. Does anyone have anything against option 1?

@aheejin
Copy link
Member

aheejin commented Aug 31, 2021

I still don't have a strong preference, but @rossberg's inlining argument makes sense to me. If we choose Option 3, optimizations like inlining can be straightforward in tools such as Binaryen. (Currently we don't do much optimizations yet in Binaryen, and we disable inlining any function containing try-delegate for this reason.) If we don't result in a consensus, I could maybe switch to weak preference to Option 3.

@aheejin
Copy link
Member

aheejin commented Sep 2, 2021

If there is no more opinions or strong objections, I think we can resolve the discussion to Option 3. The engine implementations for V8 and FF have to change, but the engine implementors here said the changes would be minor, so I hope that is not a big problem. Thank you everyone for the discussion. I'll update the Explainer soon.

@aheejin aheejin closed this as completed Sep 2, 2021
@aheejin
Copy link
Member

aheejin commented Sep 2, 2021

Sorry, I think it'd be better to keep this open until I update the Explainer after all 😅

@aheejin aheejin reopened this Sep 2, 2021
@rossberg
Copy link
Member

rossberg commented Sep 2, 2021

Cool, thanks @aheejin!

@thibaudmichaud
Copy link
Collaborator

I don't have any concerns about implementing option 3 in V8. I would just like to propose that we also remove the validation error discussed in #146:

try
catch
  try
  delegate 0 ;; invalid
end

Compare it to:

try
catch
  block
    try
    delegate 0 ;; valid
  end
end

The second one is valid with option 3 IIUC. The first enclosing try block is the same, so it feels inconsistent that they have a different behavior.

@rossberg
Copy link
Member

rossberg commented Sep 3, 2021

@thibaudmichaud, I agree both should be equally valid. I believe this corresponds exactly to this comment I just made as well.

@aheejin
Copy link
Member

aheejin commented Sep 3, 2021

@thibaudmichaud I agree this is now valid too.

pull bot pushed a commit to Alan-love/v8 that referenced this issue Sep 6, 2021
Update the behavior of 'delegate' according to:
WebAssembly/exception-handling#176

Summary: delegate can target any block, which just rethrows to the next
outer try/catch.

R=clemensb@chromium.org

Bug: v8:8091
Change-Id: I967db9ab1cbb1a15b2c5e0a1a20f64fa19a3f769
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3140603
Commit-Queue: Thibaud Michaud <thibaudm@chromium.org>
Reviewed-by: Clemens Backes <clemensb@chromium.org>
Cr-Commit-Position: refs/heads/main@{#76677}
aheejin added a commit to aheejin/exception-handling that referenced this issue Sep 7, 2021
This adds the changes decided in WebAssembly#176 and adds a modified version from
WebAssembly#146 (comment)
for clarification. (The example is not the same by the way; the `catch`
from the outermost `try` has been moved)

Closes WebAssembly#176.
aheejin added a commit to aheejin/exception-handling that referenced this issue Sep 7, 2021
This adds the changes decided in WebAssembly#176 and adds a modified version from
WebAssembly#146 (comment)
for clarification. (The example is not the same by the way; the `catch`
from the outermost `try` has been moved)

Closes WebAssembly#176.
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Sep 9, 2021
The semantics of the try-delegate Wasm exception handling instruction
was recently changed in this spec discussion:

  WebAssembly/exception-handling#176

This patch adjusts compilation and validation to match the new
semantics, which allows delegate to target any block.

Differential Revision: https://phabricator.services.mozilla.com/D124424
aheejin added a commit that referenced this issue Sep 15, 2021
This adds the changes decided in #176 and adds a modified version from
#146 (comment)
for clarification. (The example is not the same by the way; the `catch`
from the outermost `try` has been moved)
aosmond pushed a commit to aosmond/gecko that referenced this issue Sep 18, 2021
The semantics of the try-delegate Wasm exception handling instruction
was recently changed in this spec discussion:

  WebAssembly/exception-handling#176

This patch adjusts compilation and validation to match the new
semantics, which allows delegate to target any block.

Differential Revision: https://phabricator.services.mozilla.com/D124424
takikawa added a commit to takikawa/wabt that referenced this issue Oct 27, 2021
The semantics was changed recently to remove the restriction to
only target try blocks or the function body block:

  WebAssembly/exception-handling#176
takikawa added a commit to takikawa/wabt that referenced this issue Oct 28, 2021
The semantics was changed recently to remove the restriction to
only target try blocks or the function body block:

  WebAssembly/exception-handling#176
aheejin pushed a commit to WebAssembly/wabt that referenced this issue Oct 29, 2021
This PR makes wabt's validation behavior for `try-delegate` match the spec and spec interpreter. The spec now allows the delegate to target any block, rather than just `try` blocks and the function body block.

The semantics was changed a bit ago as discussed in WebAssembly/exception-handling#176
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
9 participants