Reflect spec changes in delegate immediate operand #178
Conversation
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
requested review from
takikawa,
thibaudmichaud,
rossberg and
ioannad
| `delegate` can only target `catch`/`catch_all`s that are below the current | ||
| instruction, i.e., it can only delegate downwards. It also targets all | ||
| `catch`/`catch_all`s of a `try` as a whole and does not target individual | ||
| `catch`/`catch_all` within a `try`. Here is another example: |
There was a problem hiding this comment.
Nit: Delegate is no longer targeting catch after this change, it simply targets a block. I would perhaps rephrase this accordingly and say that all properties of Wasm's structured control flow labels apply, i.e., you can only target labels of outer blocks.
There was a problem hiding this comment.
True, but the example below is, the delegate targets $l1, which is outside of the instruction because the label is attached to the try, but because it is below catch so it ends up not targeting $l1 but rather $l0, and this was what I tried to explain. Do you think simply describing that delegate target labels of outer blocks can explain this case?
There was a problem hiding this comment.
@rossberg Do you still think this needs fixing?
There was a problem hiding this comment.
I would say s.th like this:
"""
Like branches, delegate can only target outer blocks, and effectively rethrows the exception in that block. Consequently, delegating to a specific catch or catch_all handler requires targeting the respective label from within the associated try block. Delegating to a label from within a catch block does delegate the exception to the next enclosing handler -- analogous to performing a throw within a catch block, that handler is no longer active at that point.
"""
Co-authored-by: Thibaud Michaud <thibaudmichaud.1@gmail.com> Co-authored-by: Andreas Rossberg <rossberg@mpi-sws.org>
| ``` | ||
| In case `foo` throws, `delegate 1` here delegates the exception handling to the | ||
| caller, i.e., the exception escapes the current function. If the immediate is | ||
| greater than the depth of the outermost block + 1, it is a validation failure. |
There was a problem hiding this comment.
Should this be "greater than or equal to"? (or "depth of the outermost block" without +1?)
There was a problem hiding this comment.
Hmm, yeah, "the depth of the outermost block" is confusing. I tried to rephrase it to
If the immediate is greater than the number of block nesting including the implicit function-level block, it is a validation failure.
Do you think it's better?
There was a problem hiding this comment.
I think there's still some potential for confusion if someone interprets "number of block nesting" as counting from 1 and not 0. (In the example right above here you might count nestings of try and function body as 2, but immediates >=2 are not allowed here) Perhaps it could explicitly say it's counting from zero?
There was a problem hiding this comment.
Sorry, yeah, it should change to "greater than equal to". Intuitively the level of nesting is a number > 0 when there's any, and that's what I meant too. I changed it to
If the immediate is greater than or equal to the number of block nesting including the implicit function-level block, it is a validation failure. In this example, any number equal to or greater than 2 is now allowed.
There was a problem hiding this comment.
Oh... now allowed -> not allowed...
|
@takikawa I'm not very familiar with what the status of newly added CI is, but is it expected to fail? This PR doesn't modify anything with the interpreter or the spec tests though. |
|
@aheejin Yes, it should be ok that these two things (ref interpreter & core spec) are failing, and it should be ok to just merge. Sorry for the confusion. The issue with the interpreter CI is that the interpreter changes haven't been merged yet so the exception tests don't pass (BTW, did you or anyone else want to look at that PR more or should I merge it as it has one approval already?), but it should be ok after it's merged. The core spec fails most likely because it's out of sync with the upstream spec. This should be fixed once it's rebased. |
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
catchfrom the outermost
tryhas been moved)Closes #176.