Fix binary parser of declarative element segments

[tanishiking]

The parser was incorrectly handling the parsing of declarative element segments (those with the elemkind of declarative).
According to the spec, the init of the declarative element should be a vec(expr) if the prefix is 7.
https://webassembly.github.io/spec/core/binary/modules.html#element-section
However, binaryen was simply reading a single u32LEB value for each
expression instead of parsing a expression regardless usesExpressions = true.

This commit updates the WasmBinaryReader::readElementSegments function
to correctly parse the expressions for declarative element segments by
calling readExpression instead of getU32LEB if usesExpressions = true.

Resolves the parsing exception:
"[parse exception: bad section size, started at ... not being equal to new position ...]"

Related discussion: tanishiking/scala-wasm#136

[kripken]

I'm not very familiar with this part of the spec, but it looks like there are broken tests on CI here so perhaps additional changes are needed?

[tanishiking]

Thanks! Indeed, it seems like the problem is, we have to switch the init is vec(expr) or vec(funcidx) based on the the prefix is 3 or 7 (and we didn't). https://webassembly.github.io/spec/core/binary/modules.html#element-section

And, also some tests need to be fixed. I'll take a look

[tanishiking]

I think I could fix the issue, but in my local environment some tests are still fail because of the following error message

/Users/tanishiking/ghq/github.com/tanishiking/binaryen/test/lit/fuzz-types.test:31:15: error: found empty check string with prefix 'CHECK:'
;; CHECK-NEXT:
              ^

error: command failed with exit status: 2

which seems some kind of problem in my local setup(?) and unrelated to the change (actually it fails even in main branch) 😃

[kripken]

Maybe the local error is due to the lit version? Installing it this way may help:

binaryen/requirements-dev.txt

Line 4 in 06cbe01

# Install with `pip3 install -r requirements-dev.txt`

The code here looks right to me. Please add a test for this so we don't regress it. The test could be under test/lit/ if there isn't a specific obvious file to add to.

[tanishiking]

Thanks @kripken !

Now, I'm struggling to test this change because

• The S-expr parser in binaryen (and the binary parser too) does not keep declared elements in the IR

  • binaryen/src/wasm/wasm-s-parser.cpp

    Lines 3921 to 3924 in 9b61eb3

    	if (s[i]->isStr() && s[i]->str() == DECLARE) {
    	// We don't store declared segments in the IR
    	return;
    	}

• When generating output, it calculates the necessary declarations using the getFunctionsNeedingElemDeclare function, and then outputs those in Text output and Binary output.
• Therefore, whether we include declared element segments in the input WAST or binary in test cases, binaryen will ignore the input declared elements and generate the calculated ones in the output.

That's why, it is not possible to test parsing of declared element segments based on the output of binaryen. A possible approach would be to use another tool (e.g., the reference interpreter?) to generate a binary with declared elements and test if binaryen can parse that.

Are there any test cases that parses wasm binary/WAT generated by other tools?

Ah, it looks like .fromBinary is the thing

[tlively]

Note that the SExpressionWasmBuilder is deprecated and about to be removed in #6607. The parser your tests are using is implemented in the src/parser directory. It still doesn't preserve declarative segments in the IR, though.

This is a case where checking in a binary test would be appropriate. Other such tests live in the test/lit/binary directory.

[tanishiking]

Thanks, I added a test for wasm-dis that verifies the binary parser parses the declarative element segment that uses expression.
36bb145

[tlively]

Can you move the test to test/lit/binary and follow the pattern used by other tests there? We're generally trying to move tests to lit over time because it is faster and requires less maintenance than our bespoke test runners.

[tanishiking]

Sure, moved the test to test/lit/binary 02efecd

[tlively]

Thanks!