[WebAssembly] Improve support for WebAssembly binary format
Needs ReviewPublic

Authored by sbc100 on Fri, Mar 17, 1:37 PM.

Details

Summary

Mostly this change adds support converting to and from
YAML which will allow us to write more test cases for
the WebAssembly MC and lld ports.

Better support for objdump, readelf, and nm will be in
followup CLs.

I had to update the two wasm test binaries because they
used the old style 'name' section which is no longer
supported.

sbc100 created this revision.Fri, Mar 17, 1:37 PM
sbc100 updated this revision to Diff 92192.Fri, Mar 17, 1:47 PM
  • refactor section parsing
dschuff added inline comments.Fri, Mar 17, 4:09 PM
include/llvm/Object/Wasm.h
70

any particular reason these are all const copies and not const ref?

include/llvm/Support/Wasm.h
120

I think the kinds are enum'd as unsigned currently? (and export below)

lib/Object/WasmObjectFile.cpp
82

Might be worth having a set of functions (or template?) like
uint32_t readVarUint32(const uint8_t *&)
for all the various sizes defined in the encoding spec, which use readLEB128 under the hood and check that the decoded value fits in the expected type.

202–204

readSection vs parseSection is kind of confusing now. maybe this should be named something like readSectionHeader now?

347

For these "count followed by records" type sections, should we validate that we have reached the end when count reaches 0, and that we haven't overflowed length before it does?

386

else if? or switch?

390

maybe TODO handle table/memory imports

467

should we validate the start function?

501

if this can happen because of bad input (as opposed to some program invariant that you try to maintain elsewhere) it should be some error rather than an assert.

608

should this be SF_Global | SF_Undefined?

lib/ObjectYAML/WasmYAML.cpp
172

any way to return an error or exit instead of just asserting?

sbc100 updated this revision to Diff 92238.Fri, Mar 17, 5:23 PM
sbc100 marked 6 inline comments as done.
  • cleanup based on feedback
sbc100 updated this revision to Diff 92393.Mon, Mar 20, 3:58 PM
  • add support for reading and dumping relocations
sbc100 marked an inline comment as done.Mon, Mar 20, 3:59 PM
sbc100 added inline comments.
lib/Object/WasmObjectFile.cpp
347

I added a check for early ending of each section. I could add more robustness against malformed files if you like, but I don't think its very important here. I'm not sure how much checking of the other object readers going.

608

I think SF_Global signals that this symbol is available outside the module, i.e. exported. In this case the its being imported, not exported.

lib/ObjectYAML/WasmYAML.cpp
172

I couldn't find a way no. ELFYAML.cpp uses asserts for this kind of thing too. I guess the assumption is that the yaml object in memory is correct by construction.

Some initial comments inline. Meta comment, I think this patch can be split in multiple ones, (e.g. you can have yaml2asm separately).
That would make the review easier/quicker, and so would be my life as reviewer.

include/llvm/Object/Wasm.h
44–46

not a big fan of trivial getters/setters.

lib/Object/WasmObjectFile.cpp
41–44

can you make these variables instead of macros?

tools/llvm-nm/llvm-nm.cpp
891–894 ↗(On Diff #92393)

ternary

tools/yaml2obj/yaml2obj.cpp
85

unrelated change I assume?

sbc100 updated this revision to Diff 92551.Tue, Mar 21, 2:41 PM
  • revert some parts to keep change smalle
sbc100 edited the summary of this revision. (Show Details)Tue, Mar 21, 2:48 PM
sbc100 edited the summary of this revision. (Show Details)
sbc100 updated this revision to Diff 92553.Tue, Mar 21, 2:49 PM
sbc100 marked an inline comment as done.
  • remove trivial accessors

I agree this change was a little large. I removed the parts that I could. Now its pretty much only overs obj2yaml and yaml2obj.

lib/Object/WasmObjectFile.cpp
41–44

I named them this way to match the INT_MIN/INT_MAX macros that work for builtin types. If I make them variables presubably I should also rename to to different style? Something like Varint7Max? Is that preferable?

tools/llvm-nm/llvm-nm.cpp
891–894 ↗(On Diff #92393)

I removed this from this change, but I will fix in the followup.

tools/yaml2obj/yaml2obj.cpp
85

Yes, this makes the error consistent with obj2yaml. Would you like to make a separate change for this?

davide added inline comments.Thu, Mar 23, 1:27 PM
test/tools/llvm-readobj/sections.test
526–534

As you're changing the binaries you're checking in, you may want to update instructions on how to regenerate those in the future (assuming the binaries changed).

tools/llvm-readobj/WasmDumper.cpp
58–66

you're changing getWasmSection to return a pointer. That's good, but can be done separately.

tools/yaml2obj/yaml2obj.cpp
85

Yes, please.

sbc100 updated this revision to Diff 93005.Fri, Mar 24, 2:01 PM
sbc100 marked an inline comment as done.
  • add index to types in yaml
  • Merge remote-tracking branch 'origin/master' into add_wasm_object_type
  • Split out unrelated change
sbc100 added inline comments.Fri, Mar 24, 2:01 PM
test/tools/llvm-readobj/sections.test
526–534

The instructions for regenerating the object are in test/tools/llvm-readobj/file-headers.test. (maybe there is better place?)

tools/llvm-readobj/WasmDumper.cpp
58–66

I also changed the return type of this function (from wasm::WasmSection to WasmSection which is defined in include/Object/Wasm.h). I could make it return a pointer again but then it would be inconsistent with all the new methods I added. Are you sure that is preferable in this case?

tools/yaml2obj/yaml2obj.cpp
85
davide added inline comments.Fri, Mar 24, 2:57 PM
tools/llvm-readobj/WasmDumper.cpp
58–66

to return a reference, that is.

sbc100 added inline comments.Tue, Mar 28, 1:40 PM
tools/llvm-readobj/WasmDumper.cpp
58–66

Did you see my earlier response?