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

[Davide Italiano via llvm-commits]

davide 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;
----------------
Any reason why you're inlining this method here but not all the other trivial ones? (`getArch()` et similia)


================
Comment at: include/llvm/Support/Wasm.h:38-49
+  WASM_SEC_USER = 0,
+  WASM_SEC_TYPE = 1,
+  WASM_SEC_IMPORT = 2,
+  WASM_SEC_FUNCTION = 3,
+  WASM_SEC_TABLE = 4,
+  WASM_SEC_MEMORY = 5,
+  WASM_SEC_GLOBAL = 6,
----------------
Adding comment about what these variables represent (as we do, e.g. in `ELF.h`) would be nice.


================
Comment at: lib/Object/WasmObjectFile.cpp:117-119
+uint32_t WasmObjectFile::getSymbolFlags(DataRefImpl Symb) const {
+  llvm_unreachable("not yet implemented");
+  return 0;
----------------
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.


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


https://reviews.llvm.org/D26172