[PATCH] D45118: Linking debug (DWARF) sections from the WebAssembly object files

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added a comment.

Looking much better in general



================
Comment at: wasm/Symbols.h:186
+
+class CombinedSection : public SectionSymbol {
+public:
----------------
This extra symbol type seems a little strange to me... 

Could we put the getOutputSectionIndex/setOutputSectionIndex in the base SectionSymbol class so we don't need a separate CombinedSection?

Then we could want to call getOutputSectionIndex on all of the DefinedSection object too.  


================
Comment at: wasm/Writer.cpp:769
+      if (auto *S = dyn_cast<DefinedSection>(Sym)) {
+        StringRef Name = S->Section->getName();
+        if (!Name.startswith(".debug_") ||
----------------
You can just use S->getName() I think


================
Comment at: wasm/Writer.cpp:781
+        Sym->setOutputSymbolIndex(SymbolIndex);
+        DebugSymbolIndicies[Name] = SymbolIndex;
+
----------------
I don't see how you can use SymbolIndex here without incrementing it.


================
Comment at: wasm/Writer.cpp:785
+        auto P = CombinedSectionSymbols.try_emplace(
+            Name, Name, llvm::wasm::WASM_SYMBOL_BINDING_LOCAL, Name);
+        assert(P.second);
----------------
This loop seems like its doing some different things.

I wonder if we can find a way to build CombinedSectionSymbols in some other phase?  For example perhaps in  createCustomSections?  Since for each custom section we need such a symbol?


================
Comment at: wasm/Writer.cpp:790
+        SymtabEntries.emplace_back(OutputSym);
+        continue;
+      }
----------------
Can you replace these three lines with `Sym = &P.first->second` and avoid the continue?


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D45118