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

Expand tests for array.new_data #562

Merged

Conversation

fitzgen
Copy link
Contributor

@fitzgen fitzgen commented Sep 27, 2024

I noticed that the existing tests didn't exercise out-of-bounds indexing of the data segment, so I added a test for that.

At the same time, since I was already creating a dedicated .wast file for testing array.new_data, I also added a few general tests to round the file out. Some of these might technically duplicate things that already happen to be tested in array.wast, but I think that's probably fine, and that it is good to have dedicated tests in this new file. I can remove them if we'd rather not have these potential-duplicate tests.

Noticed that the existing tests didn't exercise out-of-bounds indexing of the
data segment, so I added a test for that. At the same time, since I was already
creating a dedicated `.wast` file for testing `array.new_data`, I also added a
few general tests to round the file out. Some of these might technically
duplicate things that already happen to be tested in `array.wast`, but I think
that's probably fine, and that it is good to have dedicated tests in this new
file. I can remove them if we'd rather not have these potential-duplicate tests.
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

test/core/gc/array_new_data.wast Show resolved Hide resolved
test/core/gc/array_new_data.wast Outdated Show resolved Hide resolved
test/core/gc/array_new_data.wast Show resolved Hide resolved
@fitzgen
Copy link
Contributor Author

fitzgen commented Sep 28, 2024

Thanks for review, I've pushed updates.

@rossberg rossberg merged commit a765a6d into WebAssembly:main Sep 28, 2024
1 check passed
@fitzgen fitzgen deleted the test-for-out-of-bounds-array-new-data branch September 28, 2024 20:26
fitzgen added a commit to fitzgen/gc that referenced this pull request Sep 28, 2024
Similar to WebAssembly#562 but for `array.new_elem`.

I opted to exercise both expression-style elements and the old MVP-style
function-index elements, as they have slightly different representations in
Wasmtime and that means we end up doing the indexing in two different code paths
depending on which type of element segment we have. Figured that other engines
might have similar code paths so it's good to test both.
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.

2 participants