[PATCH] D56684: [WebAssembly] Fixed objdump not parsing function headers.

[Wouter van Oortmerssen via Phabricator via llvm-commits]

aardappel marked 9 inline comments as done.
aardappel added a comment.

LLD fix: https://reviews.llvm.org/D56687



================
Comment at: include/llvm/Object/Wasm.h:225
   wasm::WasmFunction &getDefinedFunction(uint32_t Index);
+  const wasm::WasmFunction &getDefinedFunction(uint32_t Index) const;
   wasm::WasmGlobal &getDefinedGlobal(uint32_t Index);
----------------
sbc100 wrote:
> Do we still need to non-const version?
Yup I checked, it is in use.


================
Comment at: lib/MC/MCParser/WasmAsmParser.cpp:96
+    auto WS = getContext().getWasmSection(Name, SectionKind::getText());
+    getStreamer().SwitchSection(WS);
     return false;
----------------
sbc100 wrote:
> This seems unrelated?  Separate change maybe?
This change is required for the tests to work (see commit message)


================
Comment at: lib/Object/WasmObjectFile.cpp:1016
+  assert(isDefinedFunctionIndex(Index));
+  return Functions[Index - NumImportedFunctions];
+}
----------------
sbc100 wrote:
> Seems strange to have to duplicate the body here.
Afaik having one call the other requires a const_cast which is a bit awkward?


================
Comment at: lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:302
+    if (Type == "except_ref")
+      return wasm::ValType::EXCEPT_REF;
     return Optional<wasm::ValType>();
----------------
sbc100 wrote:
> Separate change?
I personally feel small "drive by" fixes are ok. I could have also left this type out of the test but felt this was better.


================
Comment at: lib/Target/WebAssembly/Disassembler/LLVMBuild.txt:22
 parent = WebAssembly
-required_libraries = MCDisassembler WebAssemblyInfo Support
+required_libraries = MCDisassembler WebAssemblyInfo WebAssemblyAsmPrinter Support
 add_to_library_groups = WebAssembly
----------------
sbc100 wrote:
> What made this necessary?
The disassembler calls `WebAssembly::anyTypeToString` which I didn't want to duplicate. And the `WebAssemblyAsmPrinter` library is really small, just a single small .cpp


================
Comment at: lib/Target/WebAssembly/Disassembler/WebAssemblyDisassembler.cpp:133
+      return MCDisassembler::Fail;
+    outs() << "        # " << FunctionCount << " functions in section.";
+  } else {
----------------
sbc100 wrote:
> But will any symbol ever point to this address?
objdump automatically generates a symbol at address 0 if there isn't one. Rather than adding more wasm specifics into the main objdump code to prevent this, I made use of that fact to have it parse just the function count.


================
Comment at: lib/Target/WebAssembly/Disassembler/WebAssemblyDisassembler.cpp:181
     int64_t PrefixedOpc;
-    if (!nextLEB(PrefixedOpc, Bytes, Size))
+    if (!nextLEB(PrefixedOpc, Bytes, Size, false))
       return MCDisassembler::Fail;
----------------
sbc100 wrote:
> Separate bug fix?
This isn't even a bug, it's just a refactoring (making an argument non-default).


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56684/new/

https://reviews.llvm.org/D56684