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: Update spectest module definition #695

Merged
merged 2 commits into from
Jan 21, 2021
Merged

test: Update spectest module definition #695

merged 2 commits into from
Jan 21, 2021

Conversation

gumb0
Copy link
Collaborator

@gumb0 gumb0 commented Jan 21, 2021

Spectest predefined module now has a proper spec: WebAssembly/spec#1280

There were a couple of members missing which are not used anyway, so this change is only cosmetic.

@codecov
Copy link

codecov bot commented Jan 21, 2021

Codecov Report

Merging #695 (103f177) into master (c46166b) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #695   +/-   ##
=======================================
  Coverage   99.30%   99.30%           
=======================================
  Files          72       72           
  Lines       10312    10312           
=======================================
  Hits        10240    10240           
  Misses         72       72           
Flag Coverage Δ
spectests 91.50% <ø> (ø)
unittests 99.30% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
test/spectests/spectests.cpp 98.42% <ø> (ø)

@gumb0 gumb0 requested review from chfast and axic January 21, 2021 15:44
// https://github.com/WebAssembly/spec/blob/e3a3c12f51221daaa901691e674d7ff990029122/interpreter/host/spectest.ml#L33
// https://github.com/WebAssembly/spec/blob/f85f6e6b70fff9ee0f6ba21ebcdad2183bd67874/test/harness/sync_index.js#L79
// spectest module definition:
// https://github.com/WebAssembly/spec/blob/master/interpreter/README.md#spectest-host-module
Copy link
Member

Choose a reason for hiding this comment

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

Should we link to a commit hash or a tag? I think it is fine probably as is.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed to commit hash

/* wat2wasm
(module
(global (export "global_i32") i32 (i32.const 666))
Copy link
Member

Choose a reason for hiding this comment

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

Do you think this new ordering is dependent on? And if so do you think wabt produced the correct expected output?

If not, is this just to show it is a copy from a spec?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's just to show correspondence to the spec. Ordering of sections in the module is strict, all members are referred by names only in spectests, and it's not even required for module to be Wasm.

Copy link
Member

Choose a reason for hiding this comment

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

Ordering of sections in the module is strict

The sections are strictly ordered, but the wat->wasm translator could decide how it lays out members within those, e.g. the ordering of functions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well many out unit tests assume that wabt puts them in the order as defined in wat.

@gumb0 gumb0 merged commit 932bb18 into master Jan 21, 2021
@gumb0 gumb0 deleted the spectest-module branch January 21, 2021 17:20
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