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.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
- 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.
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? |
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? |
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. |
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.
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 |
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. |
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. |
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.