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

[Tim Northover via Phabricator via llvm-commits]

t.p.northover added a comment.

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.



================
Comment at: include/llvm/MC/MCSectionWasm.h:27-28
+
+/// This represents a section on linux, lots of unix variants and some bare
+/// metal systems.
+class MCSectionWasm final : public MCSection {
----------------
Stale comment.


================
Comment at: lib/MC/MCWasmStreamer.cpp:85
+  // them. This makes writing matching .o files easier.
+  if (Attribute == MCSA_IndirectSymbol) {
+    // Note that we intentionally cannot use the symbol data here; this is
----------------
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.


================
Comment at: lib/Target/WebAssembly/MCTargetDesc/WebAssemblyAsmBackend.cpp:101-102
+                                            MCObjectWriter *OW) const {
+  if (Count == 0)
+    return true;
+
----------------
Is this really necessary? It's basically exactly what the for loop is doing below.


================
Comment at: lib/Target/WebAssembly/MCTargetDesc/WebAssemblyAsmBackend.cpp:116
+
+  unsigned NumBytes = (Info.TargetSize + 7) / 8;
+  if (Value == 0)
----------------
llvm::alignTo does this.


Repository:
  rL LLVM

https://reviews.llvm.org/D26722