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

[Davide Italiano via llvm-commits]

davide added inline comments.


================
Comment at: include/llvm/Object/Wasm.h:31
+  const wasm::WasmSection *getWasmSection(const SectionRef &Section) const;
+  static inline bool classof(const Binary *v) { return v->isWasm(); }
+
----------------
I think `inline` inside class definition is not necessary (check elsewhere).


================
Comment at: include/llvm/Object/Wasm.h:34
+protected:
+  // Overrides from ObjectFile.
+  void moveSymbolNext(DataRefImpl &Symb) const override;
----------------
This comment is too trivial to be useful. Please remove.


================
Comment at: lib/Object/WasmObjectFile.cpp:117-119
+uint32_t WasmObjectFile::getSymbolFlags(DataRefImpl Symb) const {
+  llvm_unreachable("not yet implemented");
+  return 0;
----------------
sbc100 wrote:
> 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?
> 
No, leave it as is. Hopefully they won't be `unreachable` for long.


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


================
Comment at: lib/Object/WasmObjectFile.cpp:71
+  if (Header.Magic != StringRef("\0asm", 4)) {
+    Err = make_error<GenericBinaryError>("Bad magic number",
+                                         object_error::parse_failed);
----------------
Can't this just be a `StringError` instead of a `GenericBinaryError`? 


https://reviews.llvm.org/D26172