search

WebAssembly / extended-name-section

Other
11 stars 4 forks source link

Adding tests #2

Open sbc100 opened 4 years ago

sbc100 commented 4 years ago

Since initial support was added to binaryen (#1) I decided to add preliminary support to wabt : https://github.com/WebAssembly/wabt/pull/1554

I wonder if its time to add some tests here and try to move this proposal forward to the next stage?

tlively commented 7 months ago

@rossberg, do we have precedent for how we would even test the extended name section?

yuri91 commented 7 months ago

@tlively in the annotations proposal the reference interpreter is extended to handle arbitrary custom sections (and annotations).

here is the handler for the name section: https://github.com/WebAssembly/annotations/blob/main/interpreter/custom/handler_name.ml

and corresponding tests: https://github.com/WebAssembly/annotations/blob/main/test/custom/name/name_annot.wast

a custom handler can do any validation it wishes, and in the tests one can write a regular (module ...) block, (for tests that should pass parsing and validation), and assert_malformed_custom / assert_invalid_custom for tests that should fail.

The tests for name are currently quite basic and should probably include binary modules and not only text ones, but you get the idea.

The ones for branch hinting are here: https://github.com/WebAssembly/branch-hinting/blob/main/test/custom/metadata.code.branch_hint/branch_hint.wast

What is needed is a way to represent the section in the text format. This can be either a corresponding annotation, or the generic @custom annotation.

If we are going to remove the @name annotation, then we can still use @custom since the name section does not depend on the binary representation of the rest of the module.

rossberg commented 7 months ago

What @yuri91 said, i.e., not until now, but there is infrastructure for validating and round-tripping custom sections in the annotations repo. So perhaps it's easiest to rebase on that proposal.

However, that repo is a bit outdated, and I'm currently in the process of syncing it with upstream (which is a bit painful since we switched parsers upstream). Will let you know here when I'm done.

rossberg commented 7 months ago

Okay, that was really painful, but this repo is now up to date with upstream.

tlively commented 7 months ago

Thank you, both!