Skip to content
This repository was archived by the owner on Apr 25, 2025. It is now read-only.

Should if_except pop the exception off the stack? #44

Closed
eholk opened this issue Feb 15, 2018 · 6 comments
Closed

Should if_except pop the exception off the stack? #44

eholk opened this issue Feb 15, 2018 · 6 comments

Comments

@eholk
Copy link
Contributor

eholk commented Feb 15, 2018

For the behavior of if_except, the current proposal states:

The conditional query of an exception succeeds when the exception on the top of
the stack is an instance of the corresponding tagged exception type (defined by
except_index).

If the query succeeds, the data values (associated with the type signature of
the exception class) are extracted and pushed onto the stack, and control
transfers to the instructions in the then block.

If the query fails, it either enters the else block, or transfer control to the
end of the if_except block if there is no else block.

It does not explicitly state what happens to the exception object on the stack. I would interpret this to mean that the exception object remains on the stack in both the then and the else branches. Is this the behavior we want?

There are a couple of other options that I think could be reasonable choices and are worth discussing.

Option 0: Status Quo

This is the option that matches my read of the current proposal. The exception stays on the stack in both the then and else branches of if_except. I'm calling it Option 0 to make it easy to refer to in discussions.

Having the exception available in the then branch is possibly convenient for rethrowing the exception. However, it will be buried by the exception arguments and the producer will have to pop these off before getting access to the exception, meaning it may not actually be all that useful.

Having the exception available in the else branch makes it more compact to chain multiple tests, such as:

if_except 0:
 ... then 0 ...
else:
  if_except 1:
    ... then 1 ...
  end
end

Option 1: if_except Always Pops the Exception

In this case, the producer would have to explicitly save the exception to a local if it wants to refer to it multiple times. For example:

tee_local 0
if_except 0:
 ... then 0 ...
else:
  get_local 0
  if_except 1:
    ... then 1 ...
  end
end

This seems more consistent with the rest of the Wasm instructions, which generally remove any arguments used from the stack.

The downside is that every chained if_except will need a get_local, although chained if_excepts are likely to be rare, I expect.

Option 2: Pop in Then Branch, Leave in Else Branch

This feels natural to me. The if_except instruction attempts to unpack the exception, and if it exceeds then it consumes the exception and leaves the contents of the exception on the stack. In the else case, things are left unchanged since it failed to unpack the exception.

Care will be needed to make sure the types balance on the two branches, and the one armed if_except case seems weird under this strategy. This option does allow chaining like Option 0 does.

Option 3: Multiple Matches

The main advantage for leaving the exception on the stack is to allow chaining. Another option would be to make if_except act more like a case statement. For example:

if_except 0:
  ... then 0 ...
else_except 1:
  ... then 1 ...
else_except 2:
  ... then 2 ...
else:
  ... else ...
end

If we had this kind of format then it would make most since to me to always have if_except pop the exception, and if multiple copies are needed then locals can be used.


What does everyone think? My choice would be for Option 3, with Option 2 as a second choice. Once we seem to have some consensus here, I can write up a PR to clarify which option is used.

@aheejin
Copy link
Member

aheejin commented Feb 15, 2018

Both Option 1 and 3 look find to me. And for C++ they are gonna be the same. So I guess we can make else_except post-MVP (Level 2 or 3), if we choose Option 3.

The reasons I don't like Option 0 and 2:

  • Option 0
    • Not popping an argument is inconsistent with other instruction.
    • Even if we don't pop the exception object, it is unlikely it can be reused in a later rethrow. If it is used by a rethrow that's outside of the catch block, we have to pass it through a local anyway, and even if a rethrow is within a catch block, the rethrow can be anywhere in further nested blocks, in which case we can't reuse the exception object on the stack anyway.
  • Option 2
    • Popping the exception object only in if_except but not else seems inconsistent.
    • We always have to insert an additional drop instruction in the else side, which means one more instruction.

@eholk
Copy link
Contributor Author

eholk commented Feb 16, 2018

One of the things @aheejin and I were talking about was that Option 3 is really just Option 1 + else_except. Therefore, in the interest of keeping the MVP minimal, I'd vote to go with Option 1 and add the option of adding else_except to one of the Level 1+N proposals.

@rossberg
Copy link
Member

I was assuming 1. Agreed that it is the simplest and most coherent. I don't see the need for 3, as long as we don't have that for if either.

@binji
Copy link
Member

binji commented Feb 21, 2018

This section says "The exception value is not popped when queried."

It seems like this should be changed since everyone seems to prefer option 1.

@KarlSchimpf
Copy link
Contributor

I've updated the PR to use option 1.

@eholk
Copy link
Contributor Author

eholk commented Mar 12, 2018

Since #49 merged, I'm going to mark this as closed.

@eholk eholk closed this as completed Mar 12, 2018
ioannad pushed a commit to ioannad/exception-handling that referenced this issue Jun 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants