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

Text format for inline data in memories #5

Closed
alexcrichton opened this issue Aug 4, 2020 · 7 comments
Closed

Text format for inline data in memories #5

alexcrichton opened this issue Aug 4, 2020 · 7 comments

Comments

@alexcrichton
Copy link
Contributor

Currently the text format allows for memories to be defined with data inline:

(memory (data "..."))

where the limits are automatically calculated and this "elaborates" to a memory declaration and a data segment. Currently the text format in the Overview.md I think doesn't have an update to account for this, and I presume it'll likely change to:

(memory i64 (data "..."))

for 64-bit memories?

A very minor issue, but wanted to make sure it was logged!

binji added a commit that referenced this issue Aug 5, 2020
Includes changes for bulk memory instructions and memory abbreviations.

See #5 and #6.
@binji binji mentioned this issue Aug 5, 2020
binji added a commit that referenced this issue Aug 5, 2020
Includes changes for bulk memory instructions and memory abbreviations.

See #5 and #6.
@aardappel
Copy link
Contributor

Yes, I thought that was the most logical change too. Except we already have:

(memory (shared i64 1 1))`

for example, where the limits, and thus the i64 goes inside the list. What I've implemented for Binaryen follows the same scheme, i.e. (memory (data i64 "...")).

Then there is export / import which might come before the limits.

We should not do it differently for shared and data since that would be both inconsistent and hard to parse.

Alternatively we can always put the i64 as the first thing after memory no matter what, i.e. even before an export or a name, but then it is disconnected from the limits which doesn't seem great. You could put it after the export / name, but before data, which is again a bit trickier to parse.

@binji
Copy link
Member

binji commented Sep 9, 2020

Hm, where is (memory (shared i64 1 1)) syntax used? I see (memory 1 1 shared) typically. The inline export always comes first in a definition, so we shouldn't change it for this case.

@binji
Copy link
Member

binji commented Sep 10, 2020

Hm, yeah that doesn't match the expected format. See the reference interpreter, for example:

memory_type :
  | limits { MemoryType ($1, Unshared) }
  | limits SHARED { MemoryType ($1, Shared) }

@aardappel
Copy link
Contributor

Ok, maybe I should rework Binaryen to remove this special form as well, since it is hard to parse supporting multiple of these forms.

So the final order we want is: name (and import export) -> index type -> limits or data -> "shared" optionally ?

@binji
Copy link
Member

binji commented Sep 10, 2020

yes, that sounds right, though some of the combinations perhaps not. For example, if we allow it, shared should probably come before the data in the sugared form, though that's not currently implemented anywhere, AFAIK.

@aardappel
Copy link
Contributor

Given that there's ~100 occurrences of (shared in Binaryen tests, and that it appears to be a Binaryen specific bug, I'll leave that to be solved in a future commit (rather than reworking it in my in-progress one).

aardappel added a commit to aardappel/wabt that referenced this issue Oct 15, 2020
These uncovered some things the previous tests didn't!
Also required the switching of the location of the index as discussed in WebAssembly/memory64#5
Also one small .py change that ensures the new tests have consistent posix paths.
aardappel added a commit to aardappel/wabt that referenced this issue Oct 26, 2020
These uncovered some things the previous tests didn't!
Also required the switching of the location of the index as discussed in WebAssembly/memory64#5
Also one small .py change that ensures the new tests have consistent posix paths.
aardappel added a commit to WebAssembly/wabt that referenced this issue Oct 26, 2020
These uncovered some things the previous tests didn't!
Also required the switching of the location of the index as discussed in WebAssembly/memory64#5
Also one small .py change that ensures the new tests have consistent posix paths.
@sbc100 sbc100 closed this as completed May 11, 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

No branches or pull requests

4 participants