Implement custom section reading/writing

[dzfrias]

Draft PR for #1376.

Not fully finished yet, still needs a few things:

• wat2wasm support
• Handling of special custom sections that BinaryReader has callbacks for:
  • name
  • dylink0
  • dylink
  • reloc
  • target_features
  • linking
  • code_metadata
• Tests
• Review for consistency with rest of codebase style

I'm not super familiar with all the quirks/idioms of C++, and so I generally followed along with the rest of the codebase. That being said, I could've overlooked some important nuances in this initial draft.

Continuing my conversation with @keithw, I'm still not sure about the best way to handle the custom sections BinaryReader has callbacks for. Implementation wise, the backtracking method would be much easier. I go more in-depth on the topic in the second section of this comment.

[dzfrias]

I used the backtracking method for reading the special custom sections handled by BinaryReader already. Let me know if there are any problems with this implementation.

[dzfrias]

I had to update one of the test inputs to conform with the syntax of the @custom annotation.

Let me know if I missed something in this PR!

[keithw]

This is looking really nice! Could you please add some roundtrip tests (including for custom sections that use a "place")? I'd also love to see a binary test that shows a custom "name" section for binary names that can't be represented as IDs in the text format, a la WebAssembly/spec#617

It's sort of unfortunate that the annotations proposal itself is never going to supply us with tests for @custom annotations (for reasons discussed at WebAssembly/annotations#15). And while I wish this were enough to let us round-trip an LLVM-generated relocatable object from binary to text and back to binary with the linking and relocation sections still valid, BinaryWriter doesn't necessarily write the binary exactly the same (e.g. it may drop the DataCount section). :-(

[dzfrias]

Could you please add some roundtrip tests (including for custom sections that use a "place")?

I'll get working on this! I might have to change a few things, because WatWriter writes each custom section at the end of the module, so a completely lossless roundtrip isn't possible with my current implementation. edit: On a second look at the other roundtrip tests, is it necessary to have a completely lossless conversion into the Module? I'm not sure now if that was what you intended in your original comment. Right now, we lose location information, as WastParser ignores the after/before (...) part of the syntax. No data is lost, but the location of the section when it's written by BinaryWriter is not the same.

I'd also love to see a binary test that shows a custom "name" section for binary names that can't be represented as IDs in the text format, a la WebAssembly/spec#617

Sounds good!

[dzfrias]

Is it okay that this is in ParseBindVarOpt? I'm not sure if the @name annotation is valid in every place that it's used.

[keithw]

On a second look at the other roundtrip tests, is it necessary to have a completely lossless conversion into the Module?

I don't think this is required -- I just meant something like the other roundtrip tests, where it tracks the expected output (so we can tell if it changes in a future commit).

[dzfrias]

I wrote a roundtrip test for custom sections. I also roundtripped the @name annotation. Let me know if there's anything else I should do!

[keithw]

Hi, I'm sorry to go back and forth on this. When I commented earlier I had stupidly forgotten that representing a custom "name" section requires a different kind of annotation (@name) rather than something with @custom. I think to be consistent with our general practice around here, we should probably make this PR just add @custom, and defer handling of @name annotations to a future PR. I'm a little nervous trying to review both of those changes at the same time (especially given the lack of tests). The WastParser is not the simplest code so it's probably better to keep the PRs as logically separated as we can...

In general I think it's looking good and I'll be excited to have this merged.

[dzfrias]

No problem; I reverted those commits. I'll open a PR for the @name stuff once this one is merged!

[keithw]

lgtm % comments

@sbc100 do you want to weigh in?

[keithw]

Okay, merging -- thank you for contributing this! I'm already enjoying seeing the custom sections from LLVM-produced modules in the wasm2wat output.