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

Merge mutable global proposal into spec #814

Merged
merged 2 commits into from
Jun 6, 2018
Merged

Merge mutable global proposal into spec #814

merged 2 commits into from
Jun 6, 2018

Conversation

binji
Copy link
Member

@binji binji commented Jun 6, 2018

We discussed this in the June 6 Working Group meeting and decided to merge the mutable-global proposal into v1 of the spec.

Since this proposal doesn't actually change that much, I thought it might be nicer to squash the changes from the mutable-global repo it into one patch. This means we lose the change history here, but it's still available in the mutable global repo, so I think it's OK. What does everyone think?

@binji binji requested review from rossberg and littledan June 6, 2018 18:48
Copy link
Member

@rossberg rossberg left a comment

Choose a reason for hiding this comment

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

LGTM, just one small mistake I discovered.

* Then the export description is valid with :ref:`external type <syntax-externtype>` :math:`\ETGLOBAL~C.\CGLOBALS[x]`.

.. math::
\frac{
C.\CGLOBALS[x] = \MCONST~t
C.\CGLOBALS[x] = \ETGLOBAL~\globaltype
Copy link
Member

Choose a reason for hiding this comment

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

No \ETGLOBAL here, CGLOBALS contains plain globaltypes.

@rossberg
Copy link
Member

rossberg commented Jun 6, 2018

Squashing sounds okay to me in this case, but we should probably avoid in most other cases. I suggest at least including a link to the other repo in the commit message.

@binji binji force-pushed the merge-mut-global branch from 2a06b22 to 9782879 Compare June 6, 2018 19:17
@binji binji merged commit d8f775e into master Jun 6, 2018
@binji binji deleted the merge-mut-global branch June 6, 2018 21:31
@pepyakin
Copy link
Contributor

This merge signifies the first merged feature into the spec interpreter, right? I wonder if it is desirable to provide flags to turn off such features?

@rossberg
Copy link
Member

@pepyakin, what would that be useful for? We only merge stuff that has been voted into the standard by the WG, so at that point it can be regarded an official feature (and in this case we even decided to slip it into 1.0 as a late addition). The rest is dealt with by versioning of the spec repo, I would argue.

@cretz
Copy link

cretz commented Jun 15, 2018

The rest is dealt with by versioning of the spec repo, I would argue.

That would probably suffice for common use cases. To avoid fragmentation, WASM impls need to be able to say they conform to something. After this merge, my impl is now non-compliant. No biggie for this one change, but I hope this is a special case and spec changes in general are part of a separate version.

@kmiller68
Copy link

@cretz IIRC, the long term idea is that WebAssembly will operate much like the JS standard does and implementations can claim they fully support some past year's version of the standard. e.g. Wasm v2.0/Wasm 2019.

@pepyakin
Copy link
Contributor

pepyakin commented Jun 15, 2018

That's true. However, there are a few problems with this.

This view presumes a linear adoption of the features by the embedders. So if feature A is accepted and then feature B is accepted then implementors will implement them in that order. But that might not be true since some implementations will not support all the features.

Another problem is the spec interpreter, and the testsuite can evolve not only regarding features. Bugs can be fixed in the spec interpreter, and new tests might be added.

Edit: However, I'm not arguing about the mutable globals propolsal. But about the general mechanism. Although I have the same problem as @cretz.

@kmiller68
Copy link

It's true that implementations may not implement all features from a future version of the spec. By it's nature wasm does not have a good way to test existence of some feature without embedder support. On the web (at least in the near term) this will have to be resolved by feature testing. Other embedders might choose some other mechanism.

I don't think the spec interpreter nor its tests are expected to be the source of truth. On the web, for instance, I doubt any implementation would take a bug fix that broke websites, even if that meant differing from the spec repo.

@pepyakin
Copy link
Contributor

I don't think the spec interpreter nor its tests are expected to be the source of truth.

If it is not, what is the point then? I'm not sure I follow.

Let me clarify what my case is. We are using Wasm as a VM for smart-contract execution in a blockchain. It's quite an unusual embedding. It doesn't tolerate any non-determinism (yes, we need to deal with existing non-determinism in Wasm, but that's quite simple). Thus we are not going to implement any feature that might introduce new sources of non-determinism, such as GC (assuming weak refs), threads, etc.
On the other hand, we do want to implement other features, such as mutable globals.

Another unusual aspect is that all side-effects produced by the execution cement the logic. If there is a bug that affected a result of the execution, and if the result of this execution is widely accepted, then we need to treat this bug as a part of consensus which is very awkward. Because of this, there is high demand for the high-quality specification and the testsuite.

So far, we have successfully been using the testsuite to verify our implementation. I must say it is an invaluable tool. The same goes for the spec interpreter: we have been fuzzing our implementation against it.

Having linear versioning, and probably other implementors with similar requirements, will prevent us from using the spec and the testsuite directly.

@rossberg
Copy link
Member

rossberg commented Jun 18, 2018

The spec repo (document, test suite, interpreter) is supposed to be consistent and tests and interpreter thus reflect the source of truth in terms of what's the official standard. An inconsistency between the three would be a bug that, if not obvious, would need to be resolved by the CG.

I think @kmiller68 is referring to a different sort of inconsistency, where the spec prescribes one thing but all implementations actually do something else. That is a situation that unfortunately is fairly common in JavaScript, because it is a much more complicated language, with a more hand-wavy definition, where many features have been standardised after the fact. I don't see much risk that we run into a situation like that with Wasm (at least not for the core). But if we did, we might want to decide to change the source of truth, i.e., the spec.

With regard to the test suite in particular, I don't think we will ever need to make breaking changes (other than removing some negative tests when we add features). The test suite is agreed upon by all major implementations, so the above scenario cannot really arise.

@rossberg
Copy link
Member

rossberg commented Jun 18, 2018

As for versioning, the idea is that the master branch always has all the latest features. But once we actually release a standard, we will tag it or create a branch. After 1.0 is out you should hence always be able to follow a fixed version of your choice and expect it to not grow additional features.

Until 1.0 is fixed the situation naturally is a bit more volatile, but in reality we had a feature freeze long ago. We made an exception for mutable globals because it was a fairly small extension and there was an increasing risk of creating incompatibilities in the JS API if we did it later. See the meeting notes of the May 15 CG meeting.

satabin added a commit to satabin/swam that referenced this pull request Aug 7, 2018
This is a late addition to the 1.0 version of the specification
(see WebAssembly/spec#814). I took this opportunity to try associating
the monad to the interface instances directly. I am not sure this is the
best way to do, but let’s try this.
satabin added a commit to satabin/swam that referenced this pull request Aug 7, 2018
This is a late addition to the 1.0 version of the specification
(see WebAssembly/spec#814). I took this opportunity to try associating
the monad to the interface instances directly. I am not sure this is the
best way to do, but let’s try this.
yblein added a commit to yblein/rust-wasm that referenced this pull request Oct 27, 2019
kateinoigakukun added a commit to kateinoigakukun/wasmi that referenced this pull request Jan 3, 2020
kateinoigakukun added a commit to kateinoigakukun/wasmi that referenced this pull request Jan 6, 2020
kateinoigakukun added a commit to kateinoigakukun/wasmi that referenced this pull request Jan 6, 2020
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