[PATCH] D26722: [WebAssembly] Add skeleton MC support for the Wasm container format

[Dan Gohman via Phabricator via llvm-commits]

sunfish added inline comments.


================
Comment at: lib/MC/WasmObjectWriter.cpp:114
+                                                const MCAsmLayout &Layout) {
+  // Section symbols are used as definitions for undefined symbols with matching
+  // names. If there are multiple sections with the same name, the first one is
----------------
pcc wrote:
> 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.
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.


================
Comment at: lib/MC/WasmObjectWriter.cpp:130
+
+  // The presence of symbol versions causes undefined symbols and
+  // versions declared with @@@ to be renamed.
----------------
pcc wrote:
> Same here: is symbol versioning a necessary feature of the wasm object format?
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?


Repository:
  rL LLVM

https://reviews.llvm.org/D26722