[WebAssembly] Only treat imports/exports as symbols when reading relocatable object files

[sbc100]

Summary

This change only treats imported and exports functions and globals
as symbol table entries the object has a "linking" section (i.e. it is
relocatable object file).

In this case all globals much be of type I32 and initialised with i32.const.
This was previously being assumed but not checked for and was causing
a failure on big endian machines due to using the wrong value of then
union.

See: https://bugs.llvm.org/show_bug.cgi?id=34487

[dschuff]

lib/Object/WasmObjectFile.cpp

794 ↗

(On Diff #113935)

Is there any way this can happen other than bogus YAML files? (i.e. do we have binary-parsing code yet, that should also have some check?)

[sbc100]

• handle 64bit global exports

[sbc100]

lib/Object/WasmObjectFile.cpp

794 ↗

(On Diff #113935)

Hmm.. you are right, we could have other types there. I've made this more flexible. However, nm might not always give meaningful output for binaries not produced by llvm.. which seems reasonable.

[hintonda]

lib/Object/WasmObjectFile.cpp

794 ↗

(On Diff #113942)

Is there any reason not to handle all 5 fields?

[sbc100]

lib/Object/WasmObjectFile.cpp

794 ↗

(On Diff #113942)

We are running into a some dissonance here between wasm globals in general, and wasm globals as used by llvm as an object file format.

For clang-generated object files, all the global will be type Int32 and represent memory offsets. However, wasm globals in general can contain globals of other types. Specifically the 3 missing types here are Float32, Float64 and Global. In those cases I'm not sure it makes sense to return anything here (or at least I'm not sure how 'nm' would display it).

Now that I think about it, perhaps it would be better to say that only Int32 global make it into the symbol table at all, since other types of globals can't/don't represent symbols (C globals).

[hintonda]

lib/Object/WasmObjectFile.cpp

794 ↗

(On Diff #113942)

Sounds reasonable, but could you add a comment or message to your assert to that effect? Otherwise it's sorta confusing.

[dschuff]

lib/Object/WasmObjectFile.cpp

794 ↗

(On Diff #113942)

I think we should maybe go one step further, and say that unless we have some positive indication that the globals are used according to the object file spec, that they don't become symbols even if they have i32 type. Maybe the presence of a linking metatdata section or something like that?

Eventually we may have to go another step further if we want to support actual globals used by the program; i.e. some globals make up the symbol table and some do not. We'll want a way to know which ones are which. But for now, using the "linking" section seems like a good idea.

[sbc100]

• Don't treat imports/exports as symbols table unless linking

[sbc100]

• .

[sbc100]

OK makes sense. We now only create symbols from imports/exports when the 'linking' section is present.

[hintonda]

Minor nit...

lib/Object/WasmObjectFile.cpp

819 ↗

(On Diff #114044)

Could you move the comment into the assert (here and elsewhere)? i.e.:

assert(Global.InitExpr.Opcode == wasm::WASM_OPCODE_I32_CONST && "WasmSymbols are only create for I32_CONST globals");

[sbc100]

Could you take another look derek? This is currently causing test failures for big endian architectures. If this change is too big I can lang a more targeted one line change in the interim.

[dschuff]

otherwise LGTM

lib/Object/WasmObjectFile.cpp

307 ↗

(On Diff #114045)

Even though this is the only user, it might make sense to pull this new bit into its own function, since it's not strictly part of parsing the linking section.

325 ↗

(On Diff #114045)

Nit: could we make this an explicit break here? I feel like this is just asking to get broken (no pun intended) when we add another case.

[sbc100]

• feedback

llvm/trunk/include/llvm/Object/Wasm.h

private:
   Error parseDataSection(const uint8_t *Ptr, const uint8_t *End);
 
   // Custom section types
   Error parseNameSection(const uint8_t *Ptr, const uint8_t *End);
   Error parseLinkingSection(const uint8_t *Ptr, const uint8_t *End);
   Error parseRelocSection(StringRef Name, const uint8_t *Ptr,
                           const uint8_t *End);
 
+  void populateSymbolTable();
+
   wasm::WasmObjectHeader Header;
   std::vector<WasmSection> Sections;
   std::vector<wasm::WasmSignature> Signatures;
   std::vector<uint32_t> FunctionTypes;
   std::vector<wasm::WasmTable> Tables;
   std::vector<wasm::WasmLimits> Memories;
   std::vector<wasm::WasmGlobal> Globals;
   std::vector<wasm::WasmImport> Imports;
   std::vector<wasm::WasmExport> Exports;
   std::vector<wasm::WasmElemSegment> ElemSegments;
   std::vector<WasmSegment> DataSegments;
   std::vector<wasm::WasmFunction> Functions;
   std::vector<WasmSymbol> Symbols;
   ArrayRef<uint8_t> CodeSection;
   uint32_t StartFunction = -1;
   bool HasLinkingSection = false;
   wasm::WasmLinkingData LinkingData;
   uint32_t NumImportedGlobals = 0;
   uint32_t NumImportedFunctions = 0;
+  uint32_t ImportSection = 0;
+  uint32_t ExportSection = 0;
 
   StringMap<uint32_t> SymbolMap;
 };
 
 } // end namespace object
 
 inline raw_ostream &operator<<(raw_ostream &OS,
                                const object::WasmSymbol &Sym) {
   Sym.print(OS);
   return OS;
 }
 
 } // end namespace llvm
 
 #endif // LLVM_OBJECT_WASM_H

llvm/trunk/lib/Object/WasmObjectFile.cpp

Error WasmObjectFile::parseNameSection(const uint8_t *Ptr, const uint8_t *End) {
   }
 
   if (Ptr != End)
     return make_error<GenericBinaryError>("Name section ended prematurely",
                                           object_error::parse_failed);
   return Error::success();
 }
 
+void WasmObjectFile::populateSymbolTable() {
+  // Add imports to symbol table
+  size_t ImportIndex = 0;
+  for (const wasm::WasmImport& Import : Imports) {
+    switch (Import.Kind) {
+    case wasm::WASM_EXTERNAL_GLOBAL:
+      assert(Import.Global.Type == wasm::WASM_TYPE_I32);
+      SymbolMap.try_emplace(Import.Field, Symbols.size());
+      Symbols.emplace_back(Import.Field, WasmSymbol::SymbolType::GLOBAL_IMPORT,
+                           ImportSection, ImportIndex);
+      DEBUG(dbgs() << "Adding import: " << Symbols.back()
+                   << " sym index:" << Symbols.size() << "\n");
+      break;
+    case wasm::WASM_EXTERNAL_FUNCTION:
+      SymbolMap.try_emplace(Import.Field, Symbols.size());
+      Symbols.emplace_back(Import.Field,
+                           WasmSymbol::SymbolType::FUNCTION_IMPORT,
+                           ImportSection, ImportIndex);
+      DEBUG(dbgs() << "Adding import: " << Symbols.back()
+                   << " sym index:" << Symbols.size() << "\n");
+      break;
+    default:
+      break;
+    }
+    ImportIndex++;
+  }
+
+  // Add exports to symbol table
+  size_t ExportIndex = 0;
+  for (const wasm::WasmExport& Export : Exports) {
+    if (Export.Kind == wasm::WASM_EXTERNAL_FUNCTION ||
+        Export.Kind == wasm::WASM_EXTERNAL_GLOBAL) {
+      WasmSymbol::SymbolType ExportType =
+          Export.Kind == wasm::WASM_EXTERNAL_FUNCTION
+              ? WasmSymbol::SymbolType::FUNCTION_EXPORT
+              : WasmSymbol::SymbolType::GLOBAL_EXPORT;
+      auto Pair = SymbolMap.try_emplace(Export.Name, Symbols.size());
+      if (Pair.second) {
+        Symbols.emplace_back(Export.Name, ExportType,
+                             ExportSection, ExportIndex);
+        DEBUG(dbgs() << "Adding export: " << Symbols.back()
+                     << " sym index:" << Symbols.size() << "\n");
+      } else {
+        uint32_t SymIndex = Pair.first->second;
+        Symbols[SymIndex] =
+            WasmSymbol(Export.Name, ExportType, ExportSection, ExportIndex);
+        DEBUG(dbgs() << "Replacing existing symbol:  " << Symbols[SymIndex]
+                     << " sym index:" << SymIndex << "\n");
+      }
+    }
+    ExportIndex++;
+  }
+}
+
 Error WasmObjectFile::parseLinkingSection(const uint8_t *Ptr,
                                           const uint8_t *End) {
   HasLinkingSection = true;
+
+  // Only populate the symbol table with imports and exports if the object
+  // has a linking section (i.e. its a relocatable object file). Otherwise
+  // the global might not represent symbols at all.
+  populateSymbolTable();
+
   while (Ptr < End) {
     uint8_t Type = readVarint7(Ptr);
     uint32_t Size = readVaruint32(Ptr);
     const uint8_t *SubSectionEnd = Ptr + Size;
     switch (Type) {
     case wasm::WASM_SYMBOL_INFO: {
       uint32_t Count = readVaruint32(Ptr);
       while (Count--) {
Error WasmObjectFile::parseTypeSection(const uint8_t *Ptr, const uint8_t *End) {
   }
   if (Ptr != End)
     return make_error<GenericBinaryError>("Type section ended prematurely",
                                           object_error::parse_failed);
   return Error::success();
 }
 
 Error WasmObjectFile::parseImportSection(const uint8_t *Ptr, const uint8_t *End) {
+  ImportSection = Sections.size();
   uint32_t Count = readVaruint32(Ptr);
   Imports.reserve(Count);
   for (uint32_t i = 0; i < Count; i++) {
     wasm::WasmImport Im;
     Im.Module = readString(Ptr);
     Im.Field = readString(Ptr);
     Im.Kind = readUint8(Ptr);
     switch (Im.Kind) {
     case wasm::WASM_EXTERNAL_FUNCTION:
       NumImportedFunctions++;
       Im.SigIndex = readVaruint32(Ptr);
-      SymbolMap.try_emplace(Im.Field, Symbols.size());
-      Symbols.emplace_back(Im.Field, WasmSymbol::SymbolType::FUNCTION_IMPORT,
-                           Sections.size(), i);
-      DEBUG(dbgs() << "Adding import: " << Symbols.back()
-                   << " sym index:" << Symbols.size() << "\n");
       break;
     case wasm::WASM_EXTERNAL_GLOBAL:
       NumImportedGlobals++;
       Im.Global.Type = readVarint7(Ptr);
       Im.Global.Mutable = readVaruint1(Ptr);
-      SymbolMap.try_emplace(Im.Field, Symbols.size());
-      Symbols.emplace_back(Im.Field, WasmSymbol::SymbolType::GLOBAL_IMPORT,
-                           Sections.size(), i);
-      DEBUG(dbgs() << "Adding import: " << Symbols.back()
-                   << " sym index:" << Symbols.size() << "\n");
       break;
     case wasm::WASM_EXTERNAL_MEMORY:
       Im.Memory = readLimits(Ptr);
       break;
     case wasm::WASM_EXTERNAL_TABLE:
       Im.Table = readTable(Ptr);
-      if (Im.Table.ElemType != wasm::WASM_TYPE_ANYFUNC) {
+      if (Im.Table.ElemType != wasm::WASM_TYPE_ANYFUNC)
         return make_error<GenericBinaryError>("Invalid table element type",
                                               object_error::parse_failed);
-      }
       break;
     default:
       return make_error<GenericBinaryError>(
           "Unexpected import kind", object_error::parse_failed);
     }
     Imports.push_back(Im);
   }
   if (Ptr != End)
Error WasmObjectFile::parseGlobalSection(const uint8_t *Ptr, const uint8_t *End) {
   }
   if (Ptr != End)
     return make_error<GenericBinaryError>("Global section ended prematurely",
                                           object_error::parse_failed);
   return Error::success();
 }
 
 Error WasmObjectFile::parseExportSection(const uint8_t *Ptr, const uint8_t *End) {
+  ExportSection = Sections.size();
   uint32_t Count = readVaruint32(Ptr);
   Exports.reserve(Count);
   for (uint32_t i = 0; i < Count; i++) {
     wasm::WasmExport Ex;
     Ex.Name = readString(Ptr);
     Ex.Kind = readUint8(Ptr);
     Ex.Index = readVaruint32(Ptr);
-    WasmSymbol::SymbolType ExportType;
-    bool MakeSymbol = false;
     switch (Ex.Kind) {
     case wasm::WASM_EXTERNAL_FUNCTION:
-      ExportType = WasmSymbol::SymbolType::FUNCTION_EXPORT;
       if (Ex.Index >= FunctionTypes.size() + NumImportedFunctions)
         return make_error<GenericBinaryError>("Invalid function export",
                                               object_error::parse_failed);
-      MakeSymbol = true;
       break;
-    case wasm::WASM_EXTERNAL_GLOBAL:
-      ExportType = WasmSymbol::SymbolType::GLOBAL_EXPORT;
+    case wasm::WASM_EXTERNAL_GLOBAL: {
       if (Ex.Index >= Globals.size() + NumImportedGlobals)
         return make_error<GenericBinaryError>("Invalid global export",
                                               object_error::parse_failed);
-      MakeSymbol = true;
       break;
+    }
     case wasm::WASM_EXTERNAL_MEMORY:
     case wasm::WASM_EXTERNAL_TABLE:
       break;
     default:
       return make_error<GenericBinaryError>(
           "Unexpected export kind", object_error::parse_failed);
     }
-    if (MakeSymbol) {
-      auto Pair = SymbolMap.try_emplace(Ex.Name, Symbols.size());
-      if (Pair.second) {
-        Symbols.emplace_back(Ex.Name, ExportType,
-                             Sections.size(), i);
-        DEBUG(dbgs() << "Adding export: " << Symbols.back()
-                     << " sym index:" << Symbols.size() << "\n");
-      } else {
-        uint32_t SymIndex = Pair.first->second;
-        Symbols[SymIndex] = WasmSymbol(Ex.Name, ExportType, Sections.size(), i);
-        DEBUG(dbgs() << "Replacing existing symbol:  " << Symbols[SymIndex]
-                     << " sym index:" << SymIndex << "\n");
-      }
-    }
     Exports.push_back(Ex);
   }
   if (Ptr != End)
     return make_error<GenericBinaryError>("Export section ended prematurely",
                                           object_error::parse_failed);
   return Error::success();
 }
 
uint64_t WasmObjectFile::getSymbolValueImpl(DataRefImpl Symb) const {
   case WasmSymbol::SymbolType::GLOBAL_IMPORT:
     return 0;
   case WasmSymbol::SymbolType::FUNCTION_EXPORT:
     return Exports[Sym.ElementIndex].Index;
   case WasmSymbol::SymbolType::GLOBAL_EXPORT: {
     uint32_t GlobalIndex = Exports[Sym.ElementIndex].Index - NumImportedGlobals;
     assert(GlobalIndex < Globals.size());
     const wasm::WasmGlobal& Global = Globals[GlobalIndex];
+    // WasmSymbols correspond only to I32_CONST globals
+    assert(Global.InitExpr.Opcode == wasm::WASM_OPCODE_I32_CONST);
     return Global.InitExpr.Value.Int32;
   }
   case WasmSymbol::SymbolType::DEBUG_FUNCTION_NAME:
     return Sym.ElementIndex;
   }
   llvm_unreachable("invalid symbol type");
 }
 

llvm/trunk/lib/ObjectYAML/WasmYAML.cpp

static void sectionMapping(IO &IO, WasmYAML::NameSection &Section) {
   IO.mapOptional("FunctionNames", Section.FunctionNames);
 }
 
 static void sectionMapping(IO &IO, WasmYAML::LinkingSection &Section) {
   commonSectionMapping(IO, Section);
   IO.mapRequired("Name", Section.Name);
   IO.mapRequired("DataSize", Section.DataSize);
   IO.mapRequired("DataAlignment", Section.DataAlignment);
-  IO.mapRequired("SymbolInfo", Section.SymbolInfos);
+  IO.mapOptional("SymbolInfo", Section.SymbolInfos);
 }
 
 static void sectionMapping(IO &IO, WasmYAML::CustomSection &Section) {
   commonSectionMapping(IO, Section);
   IO.mapRequired("Name", Section.Name);
   IO.mapRequired("Payload", Section.Payload);
 }
 

llvm/trunk/test/MC/WebAssembly/unnamed-data.ll

 ; CHECK-NEXT:         Offset:          
 ; CHECK-NEXT:           Opcode:          I32_CONST
 ; CHECK-NEXT:           Value:           0
 ; CHECK-NEXT:         Content:         68656C6C6F00776F726C640000000000000000000000000006000000
 ; CHECK-NEXT:   - Type:            CUSTOM
 ; CHECK-NEXT:     Name:            linking
 ; CHECK-NEXT:     DataSize:        28
 ; CHECK-NEXT:     DataAlignment:   8
-; CHECK-NEXT:     SymbolInfo:      
 ; CHECK-NEXT: ...

llvm/trunk/test/tools/llvm-nm/wasm/exports.yaml

Globals:
       - Type:        I32
         Mutable:     false
         InitExpr:
           Opcode:          I64_CONST
           Value:           32
       - Type:        I32
         Mutable:     false
         InitExpr:
-          Opcode:          I64_CONST
+          Opcode:          I32_CONST
           Value:           64
       - Type:        I32
         Mutable:     false
         InitExpr:
-          Opcode:          I64_CONST
+          Opcode:          I32_CONST
           Value:           1024
   - Type:            IMPORT
     Imports:
       - Module:          env
         Field:           fimport
         Kind:            FUNCTION
         SigIndex:        0
       - Module:          env
         Field:           gimport
         Kind:            GLOBAL
         GlobalType:      I32
         GlobalMutable:   false
   - Type:            EXPORT
     Exports:
       - Name:            foo
         Kind:            FUNCTION
         Index:           0x00000004
       - Name:            bar
         Kind:            GLOBAL
         Index:           0x00000003
+  - Type:            CUSTOM
+    Name:            "linking"
+    DataSize:        0
+    DataAlignment:   0
 
 # CHECK:      00000400 D bar
 # CHECK-NEXT:          U fimport
 # CHECK-NEXT: 00000004 T foo
 # CHECK-NEXT:          U gimport

llvm/trunk/test/tools/llvm-nm/wasm/imports.yaml

Imports:
         Field:           foo
         Kind:            FUNCTION
         SigIndex:        0
       - Module:          env
         Field:           bar
         Kind:            GLOBAL
         GlobalType:      I32
         GlobalMutable:   false
+  - Type:            CUSTOM
+    Name:            "linking"
+    DataSize:        0
+    DataAlignment:   0
 
 # CHECK:    U bar
 # CHECK:    U foo

llvm/trunk/test/tools/llvm-nm/wasm/weak-symbols.yaml

Globals:
       - Type:        I32
         Mutable:     false
         InitExpr:
           Opcode:          I64_CONST
           Value:           32
       - Type:        I32
         Mutable:     false
         InitExpr:
-          Opcode:          I64_CONST
+          Opcode:          I32_CONST
           Value:           64
       - Type:        I32
         Mutable:     false
         InitExpr:
-          Opcode:          I64_CONST
+          Opcode:          I32_CONST
           Value:           1024
   - Type:            EXPORT
     Exports:
       - Name:            weak_global_func
         Kind:            FUNCTION
         Index:           0x00000004
       - Name:            weak_global_data
         Kind:            GLOBAL

llvm/trunk/test/tools/llvm-objdump/WebAssembly/symbol-table.test

 RUN: llvm-objdump -t %p/../Inputs/trivial.obj.wasm | FileCheck %s
 
 CHECK:      SYMBOL TABLE:
+CHECK-NEXT: 00000000 l     F name puts
+CHECK-NEXT: 00000001 l     F name SomeOtherFunction
+CHECK-NEXT: 00000002 l     F name main
 CHECK-NEXT: 00000000 l     F IMPORT puts
 CHECK-NEXT: 00000000 l     F IMPORT SomeOtherFunction
 CHECK-NEXT: 00000002 g     F EXPORT main
 CHECK-NEXT: 00000010 g       EXPORT var
-CHECK-NEXT: 00000000 l     F name puts
-CHECK-NEXT: 00000001 l     F name SomeOtherFunction
-CHECK-NEXT: 00000002 l     F name main
 

llvm/trunk/test/tools/llvm-readobj/symbols.test

 ELF-NEXT:     Type: Object (0x1)
 ELF-NEXT:     Other: 0
 ELF-NEXT:     Section: .rodata.str1.1 (0x5)
 ELF-NEXT:   }
 
 WASM:      Symbols [
 WASM-NEXT:   Symbol {
 WASM-NEXT:     Name: puts
-WASM-NEXT:     Type: FUNCTION_IMPORT (0x0)
+WASM-NEXT:     Type: DEBUG_FUNCTION_NAME (0x4)
 WASM-NEXT:     Flags: 0x0
 WASM-NEXT:   }
 WASM-NEXT:   Symbol {
 WASM-NEXT:     Name: SomeOtherFunction
-WASM-NEXT:     Type: FUNCTION_IMPORT (0x0)
+WASM-NEXT:     Type: DEBUG_FUNCTION_NAME (0x4)
 WASM-NEXT:     Flags: 0x0
 WASM-NEXT:   }
 WASM-NEXT:   Symbol {
 WASM-NEXT:     Name: main
-WASM-NEXT:     Type: FUNCTION_EXPORT (0x1)
+WASM-NEXT:     Type: DEBUG_FUNCTION_NAME (0x4)
 WASM-NEXT:     Flags: 0x0
 WASM-NEXT:   }
 WASM-NEXT:   Symbol {
 WASM-NEXT:     Name: puts
-WASM-NEXT:     Type: DEBUG_FUNCTION_NAME (0x4)
+WASM-NEXT:     Type: FUNCTION_IMPORT (0x0)
 WASM-NEXT:     Flags: 0x0
 WASM-NEXT:   }
 WASM-NEXT:   Symbol {
 WASM-NEXT:     Name: SomeOtherFunction
-WASM-NEXT:     Type: DEBUG_FUNCTION_NAME (0x4)
+WASM-NEXT:     Type: FUNCTION_IMPORT (0x0)
 WASM-NEXT:     Flags: 0x0
 WASM-NEXT:   }
 WASM-NEXT:   Symbol {
 WASM-NEXT:     Name: main
-WASM-NEXT:     Type: DEBUG_FUNCTION_NAME (0x4)
+WASM-NEXT:     Type: FUNCTION_EXPORT (0x1)
 WASM-NEXT:     Flags: 0x0
 WASM-NEXT:   }
 WASM-NEXT: ]