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

Test largest valid initial and max size for memory64 in memory64.wast #106

Merged
merged 1 commit into from
Nov 13, 2024

Conversation

evicy
Copy link
Contributor

@evicy evicy commented Nov 13, 2024

No description provided.

Copy link
Member

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

@sbc100 sbc100 merged commit d283b37 into WebAssembly:main Nov 13, 2024
1 check passed
@bvisness
Copy link
Collaborator

Does this really actually work? Similar to my comment here, I don't think it's wise to actually try allocating memories with the largest possible initial size. (That said, we clearly were missing the test for the largest possible maximum size.)

@sbc100
Copy link
Member

sbc100 commented Nov 13, 2024

Ah I was assuming this was just check that the module validates, but if its really trying to allocate a memory with 0x1_0000_0000_0000 pages that seems unlikely to work in any environment.

@bvisness
Copy link
Collaborator

bvisness commented Nov 13, 2024

We could use the new module definition form that only defines but does not instantiate the module. Regardless, since (module foo) generally does instantiate the module (per the interpreter readme) I think we clearly shouldn't naively do this, #104, or #105.

I can almost guarantee that most spec test runners in wasm runtimes would explode on these tests as written. Why the spec interpreter doesn't fail on this one is a mystery to me.

@rossberg
Copy link
Member

Right, I think module definition is what you want for these. In addition to definitions, it's also worth testing the validity of large bounds on imports. Again, with module definition you can test this without actually needing suitable imports to link with.

@backes
Copy link
Member

backes commented Nov 14, 2024

Agreed, let's switch the first added line to module definition. The second added line should be fine.

@evicy
Copy link
Contributor Author

evicy commented Nov 14, 2024

Thanks for noting this! I didn't realize that this also instantiates the module.
Just to note, PR 108 fixed 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