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

Modified the throw context example with concrete types but not concrete values. #219

Merged

Conversation

ioannad
Copy link
Collaborator

@ioannad ioannad commented Sep 1, 2022

Addresses @aheejin's request in a previous review comment:
#180 (comment)

Originally I wrote this using

  • val_{i32} = (i32.const 1),
  • val_{f32} = (f32.const 2.0), and
  • val_{i64} = (i64.const 3),

but the example seemed then really long.
To keep the example relatively short I switched to the current version.

…e values.

Addresses @aheejin's request in a previous review comment:
WebAssembly#180 (comment)

Originally I wrote this using
- `val_{i32} = (i32.const 1)`,
- `val_{f32} = (f32.const 2.0)`, and
- `val_{i64} = (i64.const 3)`,

but the example seemed then really long.
To keep the example relatively short I switched to the current version.
@ioannad
Copy link
Collaborator Author

ioannad commented Sep 1, 2022

@aheejin is the example clear enough now or do you think it's better to use the concrete values, so (i32.const 1) instead of val_{i32} etc?

@ioannad ioannad requested a review from aheejin September 1, 2022 14:55
@ioannad
Copy link
Collaborator Author

ioannad commented Sep 1, 2022

@rossberg incidentally, this also addresses your previous comment about using "let ... be .. ".

Copy link
Member

@aheejin aheejin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM % comments. The concrete example is a lot easier to understand. Thanks!

document/core/exec/runtime.rst Outdated Show resolved Hide resolved
Comment on lines 762 to 763
In other words, when a throw occurs, normal execution halts and exceptional execution begins, until the throw
is the continuation (i.e., in the place of a :math:`\_`) of a throw context in a catching try block.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not strictly related to this PR, but I'm not sure if I understand this sentence.

  • What does a hole represent? (I know this is a basic question, but "block context" says it is "the next step of computation is taking place".. Is that also the case here? Which next step are we talking about?)
  • This sentence sounds like we have two separate moments of "exception execution begins" and "(until) the throw is the continuation". I thought the exception execution begins when the throw is executed... Are they two different things?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way this is a general question, so please don't consider this question as a blocker for landing this PR.

Copy link
Collaborator Author

@ioannad ioannad Sep 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sentence sounds like we have two separate moments of "exception execution begins" and "(until) the throw is the continuation". I thought the exception execution begins when the throw is executed... Are they two different things?

@aheejin this is exactly what I meant, so by "exceptional execution" I meant the execution of the throw, so the popping of values and labels until the first catching try block is reached. I was only trying to describe the execution of a throw instruction, but I see now I overcomplicated it, making it seem as if there are more than one execution steps during "exceptional execution".

About the hole, I think of the hole as the position of the stack pointer of the Wasm stack machine, but I see how mentioning the hole here can be confusing as well. Thank you for pointing this out!

Rephrasing these two lines to:

In other words, a thrown exception is caught when it's the continuation of a throw context in a catching try block, i.e., it's inside a catching try block, which is the innermost with respect to the throw.

Is it clearer like this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when it's the continuation of a throw context in a catching try block,

What's 'it' here? It looks to be "a thrown exception" looking at the first part of the sentence, but I'm not sure what it means that an exception is a continuation of a throw context.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By "the thrown exception" I meant the instruction that throws the exception, i.e., val* (throw a).

I guess it's better to call it "the exception throw" instead, reformulating.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(done)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is now entirely removed, after this discussion.

document/core/exec/runtime.rst Outdated Show resolved Hide resolved
document/core/exec/runtime.rst Outdated Show resolved Hide resolved
document/core/exec/runtime.rst Outdated Show resolved Hide resolved
document/core/exec/runtime.rst Outdated Show resolved Hide resolved
document/core/exec/runtime.rst Outdated Show resolved Hide resolved
document/core/exec/runtime.rst Outdated Show resolved Hide resolved
ioannad and others added 2 commits September 16, 2022 14:59
Co-authored-by: Andreas Rossberg <rossberg@mpi-sws.org>
@aheejin aheejin mentioned this pull request Oct 8, 2022
…to core-spec-throw-contexts-concrete-example

# Conflicts:
#	document/core/exec/runtime.rst
@ioannad
Copy link
Collaborator Author

ioannad commented Oct 13, 2022

All review comments should be addressed now, pending for approval.

@ioannad ioannad merged commit 7150909 into WebAssembly:main Nov 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants