Skip to content
This repository has been archived by the owner on Nov 3, 2021. It is now read-only.

[test] Fix table index encoding #154

Merged
merged 1 commit into from Oct 12, 2020
Merged

[test] Fix table index encoding #154

merged 1 commit into from Oct 12, 2020

Conversation

gahaas
Copy link
Contributor

@gahaas gahaas commented Jun 4, 2020

The encoding of the table index in binary-leb128.wast is incorrect with the bulk-memory extensions, see #153. I saw and fixed the issue first in the reference types proposal (see WebAssembly/reference-types#95), but apparently it also exists here.

The encoding of the table index in binary-leb128.wast is incorrect with the bulk-memory extensions, see #153. I saw and fixed the issue first in the reference types proposal (see WebAssembly/reference-types#95), but apparently it also exists here.
@@ -33,9 +33,10 @@
"\00asm" "\01\00\00\00"
"\04\04\01" ;; Table section with 1 entry
"\70\00\00" ;; no max, minimum 0, funcref
"\09\07\01" ;; Element section with 1 entry
"\09\09\01" ;; Element section with 1 entry
Copy link
Member

Choose a reason for hiding this comment

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

Just so that we're clear: this is doing the 2nd alternative

or at least making an explicit decision about this backward-incompatible binary representation change and removing (?) corresponding tests

from my issue description, right?

Because this changes the test itself rather than the spec, it basically acknowledges that new layout is a breaking change, but we think it's fine and won't break anything in the wild?

Copy link
Member

Choose a reason for hiding this comment

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

@gahaas @binji Ping. Was there any resolution here?

Copy link
Member

Choose a reason for hiding this comment

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

Since this change was already made in WebAssembly/reference-types#95, it makes sense to do the same here.

@RReverser
Copy link
Member

RReverser commented Aug 21, 2020

Just for the reference so that it doesn't get lost, there're 4 identical tests with the same problem in various places of the test suite; they should be deduplicated:

---- tests\testsuite\binary-leb128.wast:24:2 ----
Expected a valid module definition, but got an error
Parsed part: [00, 61, 73, 6D, 01, 00, 00, 00, 05, 03, 01, 00, 00, 0B, 07, 01, 80, 00, 41, 00, 0B, 00]
Unparsed part: []
Error: Could not recognise discriminant 0x80 for type DataInit

---- tests\testsuite\proposals\bulk-memory-operations\binary.wast:119:2 ----
Expected a valid module definition, but got an error
Parsed part: [00, 61, 73, 6D, 01, 00, 00, 00, 05, 03, 01, 00, 00, 0B, 07, 01, 80, 00, 41, 00, 0B, 00]
Unparsed part: []
Error: Could not recognise discriminant 0x80 for type DataInit

---- tests\testsuite\proposals\reference-types\binary-leb128.wast:24:2 ----
Expected a valid module definition, but got an error
Parsed part: [00, 61, 73, 6D, 01, 00, 00, 00, 05, 03, 01, 00, 00, 0B, 07, 01, 80, 00, 41, 00, 0B, 00]
Unparsed part: []
Error: Could not recognise discriminant 0x80 for type DataInit

---- tests\testsuite\proposals\reference-types\binary.wast:137:2 ----
Expected a valid module definition, but got an error
Parsed part: [00, 61, 73, 6D, 01, 00, 00, 00, 05, 03, 01, 00, 00, 0B, 07, 01, 80, 00, 41, 00, 0B, 00]
Unparsed part: []
Error: Could not recognise discriminant 0x80 for type DataInit

(Er, it's actually 3 identical tests, because binary-leb128.wast:24:2 is the same test copied from upstream into a reference-types proposal)

@RReverser
Copy link
Member

...Ping? :)

@gahaas gahaas merged commit 736bda2 into master Oct 12, 2020
@gahaas gahaas deleted the gahaas-patch-1 branch October 12, 2020 07:26
@RReverser
Copy link
Member

@gahaas Can you take care of the duplicates mentioned above too?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants