[PATCH] D43264: [WebAssembly] Add explicit symbol table

[Rui Ueyama via Phabricator via llvm-commits]

ruiu accepted this revision.
ruiu added a comment.

LGTM

There are a few things that hasn't been addressed, but we can address them later if we still want to do that.



================
Comment at: wasm/SymbolTable.cpp:252-264
   if (WasInserted) {
-    if (IsFunction)
-      replaceSymbol<UndefinedFunction>(S, Name, Flags, F, Type);
-    else
+    switch (Type) {
+    case WASM_SYMBOL_TYPE_FUNCTION:
+      replaceSymbol<UndefinedFunction>(S, Name, Flags, F, FunctionType);
+      break;
+    case WASM_SYMBOL_TYPE_GLOBAL:
+      replaceSymbol<UndefinedGlobal>(S, Name, Flags, F, GlobalType);
----------------
Overall, in the symbol table, we should have three functions for Function, GLobal and Data, instead of combining them into one function. I believe that'd be easier to read.


================
Comment at: wasm/Symbols.h:174-176
   DefinedData(StringRef Name, uint32_t Flags, InputFile *F = nullptr,
-              InputSegment *Segment = nullptr, uint32_t Address = 0)
+              InputSegment *Segment = nullptr, uint32_t Offset = 0,
+              uint32_t Size = 0)
----------------
That's a bit alarming that this function has so many parameters with default arguments. I generally do not prefer default parameters much because, for every optional parameter, it essentially defines a new overloaded function behind the scene. I'll try to address that later.


================
Comment at: wasm/Writer.cpp:400
 
-  std::vector<std::pair<StringRef, uint32_t>> SymbolInfo;
-  auto addSymInfo = [&](const Symbol *Sym, StringRef ExternalName) {
-    uint32_t Flags =
-        (Sym->isLocal() ? WASM_SYMBOL_BINDING_LOCAL :
-         Sym->isWeak() ? WASM_SYMBOL_BINDING_WEAK : 0) |
-        (Sym->isHidden() ? WASM_SYMBOL_VISIBILITY_HIDDEN : 0);
-    if (Flags)
-      SymbolInfo.emplace_back(ExternalName, Flags);
-  };
-  // (Imports can't have internal linkage, their names don't need to be budged.)
-  for (const Symbol *Sym : ImportedFunctions)
-    addSymInfo(Sym, Sym->getName());
-  for (const Symbol *Sym : ImportedGlobals)
-    addSymInfo(Sym, Sym->getName());
-  for (const WasmExportEntry &E : ExportedSymbols)
-    addSymInfo(E.Sym, E.FieldName);
-  if (!SymbolInfo.empty()) {
-    SubSection SubSection(WASM_SYMBOL_INFO);
-    writeUleb128(SubSection.getStream(), SymbolInfo.size(), "num sym info");
-    for (auto Pair: SymbolInfo) {
-      writeStr(SubSection.getStream(), Pair.first, "sym name");
-      writeUleb128(SubSection.getStream(), Pair.second, "sym flags");
+  if (!SymtabEntries.empty()) {
+    SubSection SubSection(WASM_SYMBOL_TABLE);
----------------
We should split this function because it grows organically to the point that it's too large.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D43264