Page MenuHomePhabricator

[WebAssembly] Add wasm support for llvm-readobj
ClosedPublic

Authored by sbc100 on Dec 2 2016, 10:16 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

sbc100 updated this revision to Diff 80091.Dec 2 2016, 10:16 AM
sbc100 retitled this revision from to [WebAssembly] Add wasm support for llvm-readobj.
sbc100 updated this object.
sbc100 added a subscriber: llvm-commits.
sbc100 updated this revision to Diff 80122.Dec 2 2016, 1:01 PM
  • Run clang-format
dschuff added inline comments.Dec 2 2016, 1:23 PM
tools/llvm-readobj/WasmDumper.cpp
26 ↗(On Diff #80091)

I guess these are now "CUSTOM" sections.

sbc100 updated this revision to Diff 80474.Dec 6 2016, 1:48 PM
sbc100 marked an inline comment as done.
  • Rename sections to
  • Merge remote-tracking branch 'origin/master' into wasm_readobj_support
  • Merge remote-tracking branch 'origin/master' into wasm_readobj_support
tools/llvm-readobj/WasmDumper.cpp
26 ↗(On Diff #80091)

Sure, I was going to make that a seperate CL but will do that now.

sbc100 updated this revision to Diff 80475.Dec 6 2016, 1:48 PM
  • clang-format
dschuff added inline comments.Dec 6 2016, 2:10 PM
tools/llvm-readobj/WasmDumper.cpp
57 ↗(On Diff #80475)

I wonder if we should have a field in the output for the name as well. Output for user sections wouldn't be very interesting without it, and it would be consistent with other formats which have names for every section. Obviously only user sections have interesting names that are different from their types, but it wouldn't hurt to also print "TABLE" or "EXPORT" or whatever for the standard sections as well.

dschuff accepted this revision.Dec 6 2016, 2:25 PM
dschuff added a reviewer: dschuff.
dschuff added inline comments.
tools/llvm-readobj/WasmDumper.cpp
57 ↗(On Diff #80475)

Come to think of it, we should do that in llvm-objdump as well, so maybe that can just be a separate change.

This revision is now accepted and ready to land.Dec 6 2016, 2:25 PM
sbc100 updated this revision to Diff 82171.Dec 20 2016, 3:51 PM
sbc100 edited edge metadata.
  • Merge remote-tracking branch 'origin/master' into wasm_readobj_support
  • Dump custom section name
sbc100 marked an inline comment as done.Dec 20 2016, 3:52 PM

Any other comments on this?

sbc100 added a comment.Jan 6 2017, 7:04 AM

Ping. Can we land this Derek?

I'd prefer to get at least a brief look by @Bigcheese or someone (suggestions, if not you?) that knows this part of the code better than I do, since we're creating something new here. Let's see if we can get a response now that people may be coming back from vacation.

ping. Any comments from @Bigcheese or perhaps @davide ?

davide added inline comments.Jan 24 2017, 7:41 AM
test/tools/llvm-readobj/file-headers.test
29–31 ↗(On Diff #82171)

Please add a comment exaplining how you created the source file. That is:
-> compiler/linker/any tool used invocation & source code. Thanks!
This is useful in case we need to regenerate the input object file.

tools/llvm-readobj/WasmDumper.cpp
1 ↗(On Diff #82171)

You may want to mention WASM Object file dumping utility ?

85–88 ↗(On Diff #82171)

Can this always happen? You enter here under Obj->isWasm().
If there's a legitimate where this can happen, I would add a test (if you don't mind).
Also, not entirely sure if unsupported_obj_file_format is the best diagnostic we can emit. Maybe broken_file? (I suspect this is consistent with what other dumpers are doing, if so, leave it as is but please add a test to exercise this code).

Bigcheese accepted this revision.Jan 25 2017, 3:23 PM

lgtm with davide's concerns resolved.

sbc100 updated this revision to Diff 86343.Jan 30 2017, 2:13 PM
  • Run clang-format
  • Rename sections to
  • clang-format
  • Dump custom section name
  • update comment
  • Add rebuild instructions for trivial.obj.wasm
sbc100 marked an inline comment as done.Jan 30 2017, 2:15 PM
sbc100 added inline comments.
tools/llvm-readobj/WasmDumper.cpp
85–88 ↗(On Diff #82171)

This is exactly what the MachO and COFF dumpers do yes, and basically the same as the ELF dumper.

You are correct that in practice there is no way this can happen, at least not by running the tool itself. In light of this I don't see how I can write a test to tickle this, and I think perhaps an assert would be more appropriate here?

davide added inline comments.Jan 30 2017, 2:17 PM
tools/llvm-readobj/WasmDumper.cpp
85–88 ↗(On Diff #82171)

Yes, that's what I would do.
Probably better not cargo culting dubious code from other drivers.

davide accepted this revision.Jan 30 2017, 2:18 PM

LGTM with the modification suggested in createWasmDumper (replace with an assertion).

Another minor comment (and thanks for your patience).

lib/Object/WasmObjectFile.cpp
86–89 ↗(On Diff #86343)

Also, clang-format the patch before submitting. This looks a little bit weirdly formatted but it could be just phab.

sbc100 updated this revision to Diff 86354.Jan 30 2017, 2:39 PM
  • switch to assert()
sbc100 marked 2 inline comments as done.Jan 30 2017, 2:41 PM
sbc100 added inline comments.
lib/Object/WasmObjectFile.cpp
86–89 ↗(On Diff #86343)

This is what clang-format wants apparently.

Feel free to submit.

This revision was automatically updated to reflect the committed changes.