search

WebAssembly / annotations

Proposal for Custom Annotation Syntax in the Text Format
https://WebAssembly.github.io/annotations/
Other
19 stars 10 forks source link

No test cases for `@custom` sections #15

Closed lars-t-hansen closed 1 year ago

lars-t-hansen commented 2 years ago

I've found no test cases for @custom sections, meaning a large piece of the new feature - really the more complicated part - is not covered by tests. I realize that it's hard to test this because we would really be testing the structure of the binary resulting from parsing the text, but all the same it seems problematic not to have any tests. (Branch hinting has the same problem.)

lars-t-hansen commented 2 years ago

For that matter, it doesn't look like the spec interpreter has any implementation of the @custom sections.

rossberg commented 2 years ago

Yes, the spec interpreter does not interpret custom sections at all. By their definition of being custom, no confirming implementation needs to be able to handle them, so it would be wrong to require any such tests to pass in any interesting way.

The same has always been true for the name section in the binary format. Not sure how to resolve that tension, we'd need a notion of "optional" tests, that an implementation can choose to want to pass.

lars-t-hansen commented 2 years ago

Yes, the spec interpreter does not interpret custom sections at all. By their definition of being custom, no confirming implementation needs to be able to handle them, so it would be wrong to require any such tests to pass in any interesting way.

I see the point, but this does not mean that the spec interpreter could not define a custom format of its own and could then have tests for that. Those tests could then test the (before S) etc clauses. Conforming implementations that do support some custom sections could perhaps create tests by adapting and augmenting those tests. It's not perfect but would be an improvement on the current situation. (I suspect this is pretty much what you're suggesting in your remark about "optional" tests.)

rossberg commented 2 years ago

Yes, I recall that there was some earlier discussion about setting up infrastructure around the test suite for something like this, in the context of some other custom section proposal (can't find it right now).

I'm very much in favour of doing this, but it's a fairly general problem and non-trivial infrastructure work, so that I would prefer to keep it separate from this proposal, especially since the situation isn't new but already exists in the MVP. Do you think that's justifiable?

lars-t-hansen commented 2 years ago

This is certainly related: https://github.com/WebAssembly/branch-hinting/issues/13

I'm sure I would be inclined to let the issue go if there existed a burning need to ship this proposal, but is there? (Not intended as a rhetorical question; I'm curious about the urgency & utility of the proposal.)

lars-t-hansen commented 2 years ago

Really https://github.com/WebAssembly/design/issues/1424 I guess.

alexcrichton commented 2 years ago

Reasonly-simple tests may be possible where the test looks something like:

(assert_roundtrip (module (@custom "...")))

which converts text-to-binary, then converts binary-to-text, then finally converts text-to-binary again, asserting that the two binary forms are equivalent. That would exercise a piece of functionality not used in the test suite today, though, a binary-to-text conversion. That being said I would expect that most engines already have one of those in the same manner as they've all got a text-to-binary to deal with the existing test suite.

rossberg commented 2 years ago

@lars-t-hansen, fair question. Technically, there is no urgency, although annotations keep coming up in various places, and it be good to settle them sooner rather than later.

@alexcrichton, well, the problem remains that we cannot require any implementation to pass such a test, since understanding custom sections is all optional. Moreover, the spec is pretty explicit that no implementation is allowed to reject malformed custom sections, so we cannot have any negative tests for them either.

So, we really need infrastructure to keep running such "voluntary" tests separate from mandatory ones, and probably also have some special failure mode for negative tests. All in all, quite a bit of design and engineering is involved, I fear.

yuri91 commented 2 years ago

I am trying to solve the testing issue for proposals like this and branch hinting. I opened an issue in the design repo since I would like to solve it generally, but in my proof of concept I added support in the interpreter for the @custom annotation. I am interested to know what you guys think about the approach. Design issue: https://github.com/WebAssembly/design/issues/1445

rossberg commented 1 year ago

Closing via #17.