[PATCH] D105539: [llvm-objdump][WebAssembly] Fix llvm-objdump on files without symbols

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added a comment.

Thanks for look into this!  I've been meaning fix this forever but didn't realize the solution could be this simple.  I'm excited to starting objdump in more of our test code once this lands.

Do you think it would make sense to do this at the libObject level rather than at the objdump level?   i.e. have libObject guarantee that each function has a symbol?  Or is it really only objdump and benefits from these fake symbols?



================
Comment at: llvm/test/tools/llvm-objdump/wasm/executable-without-symbols.test:1
+RUN: llvm-objdump -d %p/Inputs/main.wasm | FileCheck %s
+
----------------
If we can we should try to find a way to avoid checking in the binary here.

Also, it would be good to have a test for a file that also lacks a name sections, in which case I would expect to see either empty names, or synthetic/fake names (which do you think make the most sense).


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:724
 static void
-addDynamicElfSymbols(const ObjectFile *Obj,
-                     std::map<SectionRef, SectionSymbolsTy> &AllSymbols) {
----------------
Can we keep this function as is, and call it from `addFallbackSymbols` instead?    This would keep the diff smaller and avoid extra nesting.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:746
+    // all functions, to prevent the disassembler from misparsing the length and
+    // locals headers.
+    StringRef Name = Function.DebugName;
----------------
Perhaps assert `Function->SymbolName.empty()` then?


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:748
+    StringRef Name = Function.DebugName;
+    Symbols.emplace_back(Address, Name, ELF::STT_FUNC);
+  }
----------------
Looking at `createSymbolInfo` it looks like we always use `STT_NOTYPE` for wasm symbols.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1160
+  if (AllSymbols.empty())
+    addFallbackSymbols(Obj, AllSymbols);
 
----------------
I think maybe we should leave these two lines alone and add a separate block:

```
if (Obj->isWam())
   addMissingWasmCodeSymbols(..);
```

The reasons is because we want to ensure that every function has a symbol don't we?   So we might want to handle the case where some functions have symbols and some don't?




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105539