[WebAssembly] Don't allow functions to be named more than once

[sbc100]

Summary

Even though a function can have multiple names in the
linking standards (i.e. due to alias), there can only
be one name for a given function in the NAME section.

[sbc100]

• cleanup

[sbc100]

typo

[sbc100]

clang-format

[ruiu]

wasm/Writer.cpp

447

In lld we carefully avoid using hash table because hash table is slow. In general, we lookup the hash table (the SymbolTable) only once for a name, and don't insert symbol names into any other hash tables. Can you avoid doing that here?

[sbc100]

• avoid extra hash

[sbc100]

• add comment

[ruiu]

LGTM

[ncw]

I've just rewritten the name handling in D41955. What I did there was to associate the name with the Function (not the Symbol) since that's conceptually what the "names" section is (the debugger's name for the function body itself).

My solution was to simply iterate over DefinedFunctions when creating the "names" section in LLD. The names are attached to functions, either from their export name, or from their name specified in the names section. Writer::createNameSection is therefore shorter, and makes more sense: the "names" section is really all about iterating over the Functions, not the Symbols.

Another case of two people tackling the same problem at once!

[sbc100]

In D41975#974359, @ncw wrote:

I've just rewritten the name handling in D41955. What I did there was to associate the name with the Function (not the Symbol) since that's conceptually what the "names" section is (the debugger's name for the function body itself).

My solution was to simply iterate over DefinedFunctions when creating the "names" section in LLD. The names are attached to functions, either from their export name, or from their name specified in the names section. Writer::createNameSection is therefore shorter, and makes more sense: the "names" section is really all about iterating over the Functions, not the Symbols.

Another case of two people tackling the same problem at once!

I think are you right. One thing to note is that the names section can name imported functions, although its seems a lot less useful to do so. Anyway, if its OK with you I'll land this small change now and your change can build on this. This change also increases test coverage so you can be come confident when you refactor.

[ncw]

Fine by me then!

Although the names section can name imports (oddly...) I don't think clang does this when it writes out object files; I don't see any reason for LLD to do that either. Imports are already named in a canonical way by the import name! I've preserved the behaviour just for the time being in my changes.

test/wasm/alias.ll

+; RUN: llc -mtriple=wasm32-unknown-unknown-wasm -filetype=obj -o %t.o %s
+; RUN: lld -flavor wasm %t.o -o %t.wasm
+; RUN: obj2yaml %t.wasm | FileCheck %s
+
+@start_alias = alias i32 (), i32 ()* @_start
+
+; Function Attrs: nounwind uwtable
+define i32 @_start() local_unnamed_addr #1 {
+entry:
+  ret i32 0
+}
+
+; CHECK:      --- !WASM
+; CHECK-NEXT: FileHeader:      
+; CHECK-NEXT:   Version:         0x00000001
+; CHECK-NEXT: Sections:        
+; CHECK-NEXT:   - Type:            TYPE
+; CHECK-NEXT:     Signatures:      
+; CHECK-NEXT:       - Index:           0
+; CHECK-NEXT:         ReturnType:      I32
+; CHECK-NEXT:         ParamTypes:      
+; CHECK-NEXT:   - Type:            FUNCTION
+; CHECK-NEXT:     FunctionTypes:   [ 0 ]
+; CHECK-NEXT:   - Type:            TABLE
+; CHECK-NEXT:     Tables:          
+; CHECK-NEXT:       - ElemType:        ANYFUNC
+; CHECK-NEXT:         Limits:          
+; CHECK-NEXT:           Flags:           [ HAS_MAX ]
+; CHECK-NEXT:           Initial:         0x00000001
+; CHECK-NEXT:           Maximum:         0x00000001
+; CHECK-NEXT:   - Type:            MEMORY
+; CHECK-NEXT:     Memories:        
+; CHECK-NEXT:       - Initial:         0x00000002
+; CHECK-NEXT:   - Type:            GLOBAL
+; CHECK-NEXT:     Globals:         
+; CHECK-NEXT:       - Index:           0
+; CHECK-NEXT:         Type:            I32
+; CHECK-NEXT:         Mutable:         true
+; CHECK-NEXT:         InitExpr:        
+; CHECK-NEXT:           Opcode:          I32_CONST
+; CHECK-NEXT:           Value:           66560
+; CHECK-NEXT:   - Type:            EXPORT
+; CHECK-NEXT:     Exports:         
+; CHECK-NEXT:       - Name:            memory
+; CHECK-NEXT:         Kind:            MEMORY
+; CHECK-NEXT:         Index:           0
+; CHECK-NEXT:       - Name:            _start
+; CHECK-NEXT:         Kind:            FUNCTION
+; CHECK-NEXT:         Index:           0
+; CHECK-NEXT:       - Name:            start_alias
+; CHECK-NEXT:         Kind:            FUNCTION
+; CHECK-NEXT:         Index:           0
+; CHECK-NEXT:   - Type:            CODE
+; CHECK-NEXT:     Functions:       
+; CHECK-NEXT:       - Index:           0
+; CHECK-NEXT:         Locals:          
+; CHECK-NEXT:         Body:            41000B
+; CHECK-NEXT:   - Type:            CUSTOM
+; CHECK-NEXT:     Name:            linking
+; CHECK-NEXT:     DataSize:        0
+; CHECK-NEXT:   - Type:            CUSTOM
+; CHECK-NEXT:     Name:            name
+; CHECK-NEXT:     FunctionNames:   
+; CHECK-NEXT:       - Index:           0
+; CHECK-NEXT:         Name:            start_alias
+; CHECK-NEXT: ...

wasm/Symbols.h

enum Kind {
     UndefinedFunctionKind,
     UndefinedGlobalKind,
 
     LastDefinedKind = DefinedGlobalKind,
     InvalidKind,
   };
 
   Symbol(StringRef Name, uint32_t Flags)
-      : WrittenToSymtab(0), WrittenToNameSec(0), Flags(Flags), Name(Name) {}
+      : WrittenToSymtab(0), Flags(Flags), Name(Name) {}
 
   Kind getKind() const { return SymbolKind; }
 
   bool isLazy() const { return SymbolKind == LazyKind; }
   bool isDefined() const { return SymbolKind <= LastDefinedKind; }
   bool isUndefined() const {
     return SymbolKind == UndefinedGlobalKind ||
            SymbolKind == UndefinedFunctionKind;
public:
   void update(Kind K, InputFile *F = nullptr, uint32_t Flags = 0,
               const InputSegment *Segment = nullptr,
               const InputFunction *Function = nullptr,
               uint32_t Address = UINT32_MAX);
 
   void setArchiveSymbol(const Archive::Symbol &Sym) { ArchiveSymbol = Sym; }
   const Archive::Symbol &getArchiveSymbol() { return ArchiveSymbol; }
 
-  // This bit is used by Writer::writeNameSection() to prevent
-  // symbols from being written to the symbol table more than once.
+  // This bit is used to prevent symbols from being written to the symbol
+  // table more than once.
   unsigned WrittenToSymtab : 1;
-  unsigned WrittenToNameSec : 1;
 
 protected:
   uint32_t Flags;
   uint32_t VirtualAddress = 0;
 
   StringRef Name;
   Archive::Symbol ArchiveSymbol = {nullptr, 0, 0};
   Kind SymbolKind = InvalidKind;

wasm/Writer.cpp

 #include "InputChunks.h"
 #include "OutputSections.h"
 #include "OutputSegment.h"
 #include "SymbolTable.h"
 #include "WriterUtils.h"
 #include "lld/Common/ErrorHandler.h"
 #include "lld/Common/Memory.h"
 #include "lld/Common/Threads.h"
+#include "llvm/ADT/DenseSet.h"
 #include "llvm/Support/FileOutputBuffer.h"
 #include "llvm/Support/Format.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/LEB128.h"
 
 #include <cstdarg>
 
 #define DEBUG_TYPE "lld"
if (!InitFunctions.empty()) {
     SubSection.writeToStream(OS);
   }
 }
 
 // Create the custom "name" section containing debug symbol names.
 void Writer::createNameSection() {
   // Create an array of all function sorted by function index space
   std::vector<const Symbol *> Names;
+  llvm::DenseSet<uint32_t> Seen;
 
   for (ObjFile *File : Symtab->ObjectFiles) {
     Names.reserve(Names.size() + File->getSymbols().size());
     for (Symbol *S : File->getSymbols()) {
-      if (!S->isFunction() || S->isWeak() || S->WrittenToNameSec)
+      if (!S->isFunction() || S->isWeak())
+        continue;
+      if (!Seen.insert(S->getOutputIndex()).second)
         continue;
-      S->WrittenToNameSec = true;
       Names.emplace_back(S);
     }
   }
 
   SyntheticSection *Section = createSyntheticSection(WASM_SEC_CUSTOM, "name");
 
   std::sort(Names.begin(), Names.end(), [](const Symbol *A, const Symbol *B) {
     return A->getOutputIndex() < B->getOutputIndex();