This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Improve support for WebAssembly binary format
ClosedPublic

Authored by sbc100 on Mar 17 2017, 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.

Diff Detail

Repository
rL LLVM

Event Timeline

sbc100 created this revision.Mar 17 2017, 1:37 PM
sbc100 updated this revision to Diff 92192.Mar 17 2017, 1:47 PM
  • refactor section parsing
dschuff added inline comments.Mar 17 2017, 4:09 PM
include/llvm/Object/Wasm.h
62 ↗(On Diff #92192)

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

include/llvm/Support/Wasm.h
48 ↗(On Diff #92192)

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

lib/Object/WasmObjectFile.cpp
77 ↗(On Diff #92192)

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.

159–160 ↗(On Diff #92192)

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

243 ↗(On Diff #92192)

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?

282 ↗(On Diff #92192)

else if? or switch?

286 ↗(On Diff #92192)

maybe TODO handle table/memory imports

363 ↗(On Diff #92192)

should we validate the start function?

397 ↗(On Diff #92192)

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.

458 ↗(On Diff #92192)

should this be SF_Global | SF_Undefined?

lib/ObjectYAML/WasmYAML.cpp
171 ↗(On Diff #92192)

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

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

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.

458 ↗(On Diff #92192)

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
171 ↗(On Diff #92192)

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.

davide edited edge metadata.Mar 21 2017, 12:17 AM

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 ↗(On Diff #92393)

not a big fan of trivial getters/setters.

lib/Object/WasmObjectFile.cpp
41–44 ↗(On Diff #92393)

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 ↗(On Diff #92393)

unrelated change I assume?

sbc100 updated this revision to Diff 92551.Mar 21 2017, 2:41 PM
  • revert some parts to keep change smalle
sbc100 edited the summary of this revision. (Show Details)Mar 21 2017, 2:48 PM
sbc100 edited the summary of this revision. (Show Details)
sbc100 updated this revision to Diff 92553.Mar 21 2017, 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 ↗(On Diff #92393)

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 ↗(On Diff #92393)

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

davide added inline comments.Mar 23 2017, 1:27 PM
test/tools/llvm-readobj/sections.test
526–534 ↗(On Diff #92553)

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–65 ↗(On Diff #92553)

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

tools/yaml2obj/yaml2obj.cpp
85 ↗(On Diff #92393)

Yes, please.

sbc100 updated this revision to Diff 93005.Mar 24 2017, 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.Mar 24 2017, 2:01 PM
test/tools/llvm-readobj/sections.test
526–534 ↗(On Diff #92553)

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–65 ↗(On Diff #92553)

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 ↗(On Diff #92393)
davide added inline comments.Mar 24 2017, 2:57 PM
tools/llvm-readobj/WasmDumper.cpp
58–65 ↗(On Diff #92553)

to return a reference, that is.

sbc100 added inline comments.Mar 28 2017, 1:40 PM
tools/llvm-readobj/WasmDumper.cpp
58–65 ↗(On Diff #92553)

Did you see my earlier response?

sbc100 added inline comments.Mar 30 2017, 9:29 AM
tools/llvm-readobj/WasmDumper.cpp
58–65 ↗(On Diff #92553)

Are you ok with my rational for making this change? or do you want me to revert to returning a pointer?

Otherwise I think I addressed all the feedback.

davide accepted this revision.Mar 30 2017, 12:42 PM

Yes, this looks good enough (style bugs/cleanups et similia, we can iterate on them when the patch is in-tree).

This revision is now accepted and ready to land.Mar 30 2017, 12:42 PM
This revision was automatically updated to reflect the committed changes.