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

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added a comment.

Generally LGTM.

Previously objdump had listed "function index" for address of function symbols because in the wasm world the address of a function is its index.   But I guess listing code section offset is fine too.

This change will require a corresponding lld-side change.



================
Comment at: include/llvm/MC/MCDisassembler/MCDisassembler.h:91
+  ///                   byte of the symbol.
+  /// \param Bytes    - A reference to the actual bytes of the instruction.
+  /// \param VStream  - The stream to print warnings and diagnostic messages on.
----------------
actual bytes at the symbol location?


================
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);
----------------
Do we still need to non-const version?


================
Comment at: lib/MC/MCParser/WasmAsmParser.cpp:96
+    auto WS = getContext().getWasmSection(Name, SectionKind::getText());
+    getStreamer().SwitchSection(WS);
     return false;
----------------
This seems unrelated?  Separate change maybe?


================
Comment at: lib/Object/WasmObjectFile.cpp:1016
+  assert(isDefinedFunctionIndex(Index));
+  return Functions[Index - NumImportedFunctions];
+}
----------------
Seems strange to have to duplicate the body here.


================
Comment at: lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:302
+    if (Type == "except_ref")
+      return wasm::ValType::EXCEPT_REF;
     return Optional<wasm::ValType>();
----------------
Separate change?


================
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
----------------
What made this necessary?


================
Comment at: lib/Target/WebAssembly/Disassembler/WebAssemblyDisassembler.cpp:133
+      return MCDisassembler::Fail;
+    outs() << "        # " << FunctionCount << " functions in section.";
+  } else {
----------------
But will any symbol ever point to this address?


================
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;
----------------
Separate bug fix?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56684