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] Remove malformed tests that become valid with memory64 #1648

Merged

Conversation

backes
Copy link
Member

@backes backes commented May 4, 2023

With memory64, memory offsets will be decoded as u64. An engine implementing this will fail tests that test that we reject such module. Thus remove those tests from the main spec repo now; they should be added back in the memory64 repo, but changed to assert_trap tests.

With memory64, memory offsets will be decoded as u64. An engine
implementing this will fail tests that test that we reject such module.
Thus remove those tests from the main spec repo now; they should be
added back in the memory64 repo, but changed to `assert_trap` tests.
@backes backes requested a review from sbc100 May 4, 2023 14:10
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.

Instead of removing the tests, can you comment them out with a TODO to reactivate them with assert_trap in the Memory64 proposal?

Also, if you could then downstream this to the mem64 repo and address the TODO there that would be awesome. ;)

@backes
Copy link
Member Author

backes commented May 10, 2023

Thanks, Andreas, that makes sense. First part done, will address the second part now (in the memory64 repo).

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.

Thanks!

@rossberg rossberg merged commit b72a1aa into WebAssembly:main May 10, 2023
@backes
Copy link
Member Author

backes commented May 10, 2023

Sorry, I just noticed that something is off here when preparing the change in the memory64 repo. The tests were moved from binary.wast to binary-leb128.wast in #1019, but then came back when merging the bulk instructions & reference types proposals (#1287).

That would mean that we should indeed remove the tests from binary.wast, and maybe just already merge those few changes to binary-leb128.wast to make it fail both before and after memory64.

Not sure how to check if there were more such merge errors in #1287.

@sbc100
Copy link
Member

sbc100 commented May 10, 2023

The memory64 repo is currently quite out-of-date with upstream I'm afraid. I've been struggling to find time to resolve the merge conflicts (my OCaml skills is not too sharp yet) to get it up-to-date.

@backes
Copy link
Member Author

backes commented May 10, 2023

@sbc100 I guess that's fine for now, and is not what's causing the problems.

I uploaded #1649 to remove the duplicated tests (merge error in #1287) and #1650 to make the tests memory64-ready, which I think is better than commenting them out.

@backes backes deleted the remove-binary-tests-passing-in-memory64 branch May 10, 2023 14:49
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.

3 participants