[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

445

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/InputChunks.h

 };
 
 // Represents a single wasm function within and input file.  These are
 // combined to create the final output CODE section.
 class InputFunction : public InputChunk {
 public:
   InputFunction(const WasmSignature &S, const WasmFunction &Func,
                 const ObjFile &F)
-      : InputChunk(F), Signature(S), Function(Func) {}
+      : InputChunk(F), Signature(S), WrittenToNameSec(false), Function(Func) {}
 
   uint32_t getSize() const override { return Function.Size; }
   const uint8_t *getData() const override {
     return File.CodeSection->Content.data() + getInputSectionOffset();
   }
 
   uint32_t getOutputIndex() const { return OutputIndex.getValue(); };
   bool hasOutputIndex() const { return OutputIndex.hasValue(); };
   void setOutputIndex(uint32_t Index) {
     assert(!hasOutputIndex());
     OutputIndex = Index;
   };
 
   const WasmSignature &Signature;
 
+  unsigned WrittenToNameSec : 1;
+
 protected:
   uint32_t getInputSectionOffset() const override {
     return Function.CodeSectionOffset;
   }
   const WasmFunction &Function;
   llvm::Optional<uint32_t> OutputIndex;
 };
 
 } // namespace wasm
 } // namespace lld
 
 #endif // LLD_WASM_INPUT_CHUNKS_H

wasm/Symbols.h

public:
   // Returns the virtual address of a defined global.
   // Only works for globals, not functions.
   uint32_t getVirtualAddress() const;
 
   void setVirtualAddress(uint32_t VA);
 
   void update(Kind K, InputFile *F = nullptr, uint32_t Flags = 0,
               const InputSegment *Segment = nullptr,
-              const InputFunction *Function = nullptr,
-              uint32_t Address = UINT32_MAX);
+              InputFunction *Function = nullptr, uint32_t Address = UINT32_MAX);
 
   void setArchiveSymbol(const Archive::Symbol &Sym) { ArchiveSymbol = Sym; }
   const Archive::Symbol &getArchiveSymbol() { return ArchiveSymbol; }
+  InputFunction *getFunction() { return Function; }
 
   // This bit is used by Writer::writeNameSection() 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;
   InputFile *File = nullptr;
   const InputSegment *Segment = nullptr;
-  const InputFunction *Function = nullptr;
+  InputFunction *Function = nullptr;
   llvm::Optional<uint32_t> OutputIndex;
   llvm::Optional<uint32_t> TableIndex;
   const WasmSignature *FunctionType = nullptr;
 };
 
 } // namespace wasm
 
 // Returns a symbol name for an error message.
 std::string toString(const wasm::Symbol &Sym);
 std::string toString(wasm::Symbol::Kind Kind);
 
 } // namespace lld
 
 #endif

wasm/Symbols.cpp

 
 void Symbol::setTableIndex(uint32_t Index) {
   DEBUG(dbgs() << "setTableIndex " << Name << " -> " << Index << "\n");
   assert(!TableIndex.hasValue());
   TableIndex = Index;
 }
 
 void Symbol::update(Kind K, InputFile *F, uint32_t Flags_,
-                    const InputSegment *Seg, const InputFunction *Func,
+                    const InputSegment *Seg, InputFunction *Func,
                     uint32_t Address) {
   SymbolKind = K;
   File = F;
   Flags = Flags_;
   Segment = Seg;
   Function = Func;
   if (Address != UINT32_MAX)
     setVirtualAddress(Address);

wasm/Writer.cpp

void Writer::createNameSection() {
   // Create an array of all function sorted by function index space
   std::vector<const Symbol *> Names;
 
   for (ObjFile *File : Symtab->ObjectFiles) {
     Names.reserve(Names.size() + File->getSymbols().size());
     for (Symbol *S : File->getSymbols()) {
       if (!S->isFunction() || S->isWeak() || S->WrittenToNameSec)
         continue;
+      // We also need to guard against two different symbols (two different
+      // names) for the same wasm function.  While this is possible (aliases)
+      // it is not legal in the "name" section.
+      InputFunction *Function = S->getFunction();
+      if (Function) {
+        if (Function->WrittenToNameSec)
+          continue;
+        Function->WrittenToNameSec = true;
+      }
       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) {