Skip to content

PR for JSPI#2199

Open
fgmccabe wants to merge 32 commits into
mainfrom
jspi
Open

PR for JSPI#2199
fgmccabe wants to merge 32 commits into
mainfrom
jspi

Conversation

@fgmccabe

Copy link
Copy Markdown
Collaborator

PR that represents merging of JSPI into main spec.

RossTate and others added 30 commits September 19, 2024 10:46
Added link to V8 design portfolio for stack switching
Also tightened up language obout requiring traps when promise/suspending not
correctly lined up.
Some of the changes that follow from reviewer's remarks.

Co-authored-by: Shu-yu Guo <syg@chromium.org>
Make it clear that a 'long jump' in the meta-interpreter is being affected.
Updated test suite for jspi to reflect new API
Merge remote-tracking branch 'spec/wasm-3.0'
Some minor tweaks in response to reviewer's remarks.
Co-authored-by: Shu-yu Guo <syg@chromium.org>
Co-authored-by: Shu-yu Guo <syg@chromium.org>
Co-authored-by: Shu-yu Guo <syg@chromium.org>
use 'marker' suffix to better distinguish Suspending objects from functions
Small changes, responding to remarks by reviewer.
Small tweak:
  Add an explanation for the role of the map
Big tweak:
  Arrange to throw a JS RuntimeError rather than trap when JSPI usage not properly balanced.
Tidy up punctuation.
Clean up throwing exceptions from JS (a little bit)
Use the new standard exception type SuspendError if a problem arises with trying to suspend.
Comment thread document/js-api/index.bs Outdated
Co-authored-by: Francis McCabe <fmccabe@gmail.com>

@eqrion eqrion left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is a tricky one, and I'm still learning the spec interactions for WHATWG/TC39 so forgive me if I messed some of these comments up. I'd like to take another look at this.

Comment on lines -6 to 13
.. |WasmDraft| replace:: |pagelink|
.. _WasmDraft: |pagelink|
.. |WasmDraft| replace:: https://webassembly.github.io/js-promise-integration/core/
.. _WasmDraft: https://webassembly.github.io/js-promise-integration/core/

.. |WasmIssues| replace:: |issuelink|
.. _WasmIssues: |issuelink|
.. |WasmIssues| replace:: https://github.com/webassembly/js-promise-integration/issues/
.. _WasmIssues: https://github.com/webassembly/js-promise-integration/issues/

.. |IEEE754| replace:: IEEE 754
.. |IEEE754| replace:: IEEE 754-2019
.. _IEEE754: https://ieeexplore.ieee.org/document/8766229

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think we want to revert these changes.

Comment thread document/core/index.rst

.. only:: html

| Release |release|

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Revert this change.

Comment thread README.md
![Spectec Status](https://github.com/WebAssembly/spec/actions/workflows/ci-spectec.yml/badge.svg)
![Interpreter Status](https://github.com/WebAssembly/spec/actions/workflows/ci-interpreter.yml/badge.svg)
![Spec doc Status](https://github.com/WebAssembly/spec/actions/workflows/ci-spec.yml/badge.svg)
[![Build Status](https://travis-ci.org/WebAssembly/js-promise-integration.svg?branch=main)](https://travis-ci.org/WebAssembly/js-promise-integration)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Revert this change too.

Comment thread document/js-api/index.bs
Comment on lines 1286 to 1333

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This looks like an accidental removal?

Comment thread document/js-api/index.bs
Comment on lines +113 to +114
text: evaluation_suspend; url: appendix/embedding.html#embed-evaluation-suspend
text: evaluation_resume; url: appendix/embedding.html#embed-evaluation-resume

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

These are broken links. Did you intend to use them?

Comment thread document/js-api/index.bs
1. Set the [=surrounding agent=]'s [=associated store=] to |store|.
1. If |result| is [=error=], throw a WebAssembly {{RuntimeError}} exception, unless otherwise indicated by <a href="#errors">the WebAssembly error mapping</a>.
1. Otherwise, if |result| is of the form [=throw=] exnaddr,
1. [=Reject=] |promise| with |result|.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

|result| here is a throw exnaddr abstract value, and we're rejecting a promise with it. I think we need to convert the exnaddr into a JS value first.

Comment thread document/js-api/index.bs
Comment on lines +1503 to +1504
1. Otherwise, return the result of performing [=coerce a JavaScript return=] on |resultTypes| and |awaitResult|.
1. Otherwise, return the result of performing [=coerce a JavaScript return=] on |resultTypes| and |ret|.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Both of these pass args in the reverse order the op expects.

Comment thread document/js-api/index.bs
1. If [$IsCallable$](|wasmFunc|) is false, throw a {{TypeError}}.
1. If |wasmFunc| does not have a \[[FunctionAddress]] internal slot, throw a {{TypeError}}.
1. Let |builder| be a new [=AbstractClosure=] with no parameters that captures |wasmFunc| and performs [=run a Promising function=] when called.
1. Returns the result of [=CreateBuiltinFunction=](|builder|,1,"", « »).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
1. Returns the result of [=CreateBuiltinFunction=](|builder|,1,"", « »).
1. Return the result of [=CreateBuiltinFunction=](|builder|,1,"", « »).

Comment thread document/js-api/index.bs
Comment on lines +1462 to +1465
1. Let |suspendingProto| be \[[%WebAssembly.Suspending.prototype%]].
1. Let |susp| be the result of [$OrdinaryObjectCreate$](|suspendingProto|,«\[[WrappedFunction]]»).
1. Set |susp|.\[[WrappedFunction]] to |jsFun|.
1. Return |susp|.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

WebIDL will automatically create an object for us that is wired up with the correct prototype (and other slots such as the internal platform object brand). The other WebIDL constructors in this spec just initialize the slots on the this object.

Comment thread document/js-api/index.bs
1. Let [|parameters|] → [|results|] be |functype|.
1. If any type in |parameters| or |results| [=matches/valtype|matches=] [=v128=] or [=exnref=], throw a {{TypeError}}.

1. If |parameters| or |results| contain [=v128=] or [=exnref=], throw a {{TypeError}}.
Note: the above error is thrown each time the \[[Call]] method is invoked.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Did you mean to delete this?

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.

5 participants