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

[Michael Spencer via llvm-commits]

Bigcheese added a comment.

There's a lot more code than needed here to pass the tests. Most of the section parsing code isn't actually used.

I'm wondering how you envision mapping between the binary tools and wasm. What should each of the major options of objdump print? Should ar create archives with symbol tables? Is wasm an object file format, executable format, or both?

I'd like to know what the plan is so I can review the design with that in mind.



================
Comment at: lib/Object/ObjectFile.cpp:16
 #include "llvm/Object/MachO.h"
+#include "llvm/Object/Wasm.h"
 #include "llvm/Object/ObjectFile.h"
----------------
Sort alphabetically.


================
Comment at: lib/Object/WasmObjectFile.cpp:45
+  int32_t Result = 0;
+  memcpy(&Result, Ptr, sizeof(Result));
+  Ptr += sizeof(Result);
----------------
This is ignoring the endianess.


================
Comment at: lib/Object/WasmObjectFile.cpp:52
+  int64_t Result = 0;
+  memcpy(&Result, Ptr, sizeof(Result));
+  Ptr += sizeof(Result);
----------------
This is ignoring the endianess.


================
Comment at: lib/Object/WasmObjectFile.cpp:119
+
+Error readSection(wasm::WasmSection &Section, const uint8_t *&Ptr,
+                  const uint8_t *Start) {
----------------
Why not return `WasmSection` by value?


================
Comment at: lib/Object/WasmObjectFile.cpp:434
+  switch (S.Type) {
+    ECase(USER);
+    ECase(TYPE);
----------------
Isn't the name of the section for USER sections a string from the file?


================
Comment at: tools/llvm-objdump/WasmDump.cpp:30
+
+  for (auto &Section : File->sections()) {
+    const wasm::WasmSection* WasmSec = File->getWasmSection(Section);
----------------
The section table for other formats isn't considered part of the file header.


https://reviews.llvm.org/D26172