[PATCH] D26172: [WebAssembly] Add llvm-objdump support for wasm file format

[Sam Clegg via llvm-commits]

sbc100 added inline comments.


================
Comment at: include/llvm/Object/Wasm.h:88
+  unsigned getArch() const override;
+  SubtargetFeatures getFeatures() const override { return SubtargetFeatures(); }
+  bool isRelocatableObject() const override;
----------------
davide wrote:
> Any reason why you're inlining this method here but not all the other trivial ones? (`getArch()` et similia)
Looks like I cargo-culted this from MachO.h and COFF.h which both have this strangeness.   Will move to cpp file.


================
Comment at: lib/Object/WasmObjectFile.cpp:117-119
+uint32_t WasmObjectFile::getSymbolFlags(DataRefImpl Symb) const {
+  llvm_unreachable("not yet implemented");
+  return 0;
----------------
davide wrote:
> Can any of these `unreachable` be triggered unvoluntarily by the user? (indirectly, e.g. via objdump?). If so, they should be converted to `report_fatal_error`. Otherwise it's fine, but please add the name of the function to the unreachable string.
None of the objdump flags seem to tickle any of these unreachables.

I don't see any precedent for including the name of the unreachable function for unimplemented functions (See `git grep -i "llvm_unreachable.*unimp"`.  Want me to go ahead and do this anyway?



================
Comment at: lib/Object/WasmObjectFile.cpp:210
+                  S.Content.size());
+  return std::error_code();
+}
----------------
davide wrote:
> Can this fail? e.g. empty section contents?
No, sections cannot be empty.


https://reviews.llvm.org/D26172