Page MenuHomePhabricator

[WebAssembly] Add skeleton MC support for the Wasm container format
ClosedPublic

Authored by sunfish on Nov 15 2016, 5:35 PM.

Details

Summary

As we discussed at the LLVM developer meeting, here's the first patch adding the first level of infrastructure for writing Wasm container files via MC. It's quite incomplete, as I've pulled out most of the actual encoding functionality in order to keep the patch size manageable.

Diff Detail

Repository
rL LLVM

Event Timeline

sunfish updated this revision to Diff 78112.Nov 15 2016, 5:35 PM
sunfish retitled this revision from to [WebAssembly] Add skeleton MC support for the Wasm container format.
sunfish updated this object.
sunfish added a reviewer: lgerbarg.
sunfish set the repository for this revision to rL LLVM.
sunfish updated this revision to Diff 80195.Dec 3 2016, 4:43 PM
  • rebase on trunk
  • instead of converting WebAssembly from ELF to Wasm binary format, add Wasm as an option, leaving ELF enabled and the default. This allows existing ELF-based use cases to continue working for now.
pcc added a subscriber: pcc.Dec 3 2016, 5:02 PM

Just a couple of drive by comments.

lib/MC/WasmObjectWriter.cpp
114 ↗(On Diff #80195)

It looks like this code was just copied from the ELF object writer. Do you need section symbol support in the wasm object format? Section symbols cause bugs such as http://lists.llvm.org/pipermail/llvm-dev/2016-March/097064.html so you might want to consider not supporting them.

130 ↗(On Diff #80195)

Same here: is symbol versioning a necessary feature of the wasm object format?

sunfish updated this revision to Diff 80342.Dec 5 2016, 4:12 PM
sunfish marked 2 inline comments as done.
  • remove section symbols
  • misc. cleanups
sunfish added inline comments.Dec 5 2016, 4:13 PM
lib/MC/WasmObjectWriter.cpp
114 ↗(On Diff #80195)

Ah, thanks for pointing that out about section symbols. I think you're right that we don't need section symbols, at least for now, so it seems safe to omit them.

130 ↗(On Diff #80195)

Symbol versioning is desirable functionality. We don't have to do it like ELF does, but that seemed like a reasonable place to start. Are you aware of similar bugs involving ELF symbol versioning?

pcc added inline comments.Dec 5 2016, 7:39 PM
lib/MC/WasmObjectWriter.cpp
130 ↗(On Diff #80195)

Not specifically bugs of that kind, but bear in mind that symbol versioning as implemented in ELF is rather complex and requires co-operation from the linker and the dynamic loader, so I'd imagine that an implementation that uses .symver would require spec changes, and if you're going to make a spec change you might want to consider a simpler design first.

For example, you might consider just having version numbers rather than names, and letting the versions be symbol attributes rather than being represented using aliases.

sunfish updated this revision to Diff 84029.Jan 11 2017, 2:58 PM

Your suggestion to move symbol versioning out of the symbol names makes sense. I've removed those parts of the patch now.

Also, this rebases the patch on trunk.

rnk added a subscriber: rnk.Feb 1 2017, 10:41 AM

This doesn't have any tests. Is this enough to get llvm-mc to make an object file, or does it need more work?

All the code for actually writing any bytes has been factored out into followup patches -- WasmObjectWriter::writeObject and WasmObjectWriter::writeHeader are empty -- so there's nothing to test yet.

In in favor of landing this. I think with one more +1 you'll be good to go.

lib/MC/MCWasmStreamer.cpp
188 ↗(On Diff #84029)

We should sink all of these calls to unreachable into MCStreamer, and tweak the message to be something like:

this directive only supported on COFF targets
lib/MC/WasmObjectWriter.cpp
79 ↗(On Diff #84029)

seems unused

t.p.northover edited edge metadata.Feb 9 2017, 1:18 PM

Some minor comments, but I couldn't see anything immediately wrong with these patches. It's a bit difficult to see the wood for the trees though, with some bits being implemented, some semi-stubs and some complete assertion failures.

include/llvm/MC/MCSectionWasm.h
27–28 ↗(On Diff #84029)

Stale comment.

lib/MC/MCWasmStreamer.cpp
85 ↗(On Diff #84029)

I have a strong suspicion that only MachO has indirect symbols. Certainly when I put an llvm_unreachable in the ELF equivalent of this block nothing broke.

lib/Target/WebAssembly/MCTargetDesc/WebAssemblyAsmBackend.cpp
101–102 ↗(On Diff #84029)

Is this really necessary? It's basically exactly what the for loop is doing below.

116 ↗(On Diff #84029)

llvm::alignTo does this.

sunfish updated this revision to Diff 87887.Feb 9 2017, 3:13 PM

Address review comments.

sunfish marked 5 inline comments as done.Feb 9 2017, 3:19 PM
sunfish added inline comments.
lib/MC/MCWasmStreamer.cpp
85 ↗(On Diff #84029)

Sounds good. I removed the indirect symbols code and added a similar assert.

lib/MC/WasmObjectWriter.cpp
79 ↗(On Diff #84029)

removed

lib/Target/WebAssembly/MCTargetDesc/WebAssemblyAsmBackend.cpp
101–102 ↗(On Diff #84029)

removed

116 ↗(On Diff #84029)

Updated to use alignTo.

sunfish marked 4 inline comments as done.Feb 9 2017, 3:39 PM

Some minor comments, but I couldn't see anything immediately wrong with these patches. It's a bit difficult to see the wood for the trees though, with some bits being implemented, some semi-stubs and some complete assertion failures.

Thanks for taking a look! I am hoping to fill in all of the pieces in a series of incremental patches on top of this one. This patch is the base that everything else will build on, so it seemed to make sense to do this first. But I'd be happy to take a different approach if it'd help.

Friendly ping :-).

t.p.northover accepted this revision.Feb 17 2017, 9:29 AM

Sorry about the delay. I think this looks good now, so go for it!

This revision is now accepted and ready to land.Feb 17 2017, 9:29 AM
This revision was automatically updated to reflect the committed changes.