[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

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

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

Is there any reason not to handle all 5 fields?

[sbc100]

lib/Object/WasmObjectFile.cpp

794

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

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

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

795

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

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

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

lib/Object/WasmObjectFile.cpp

return make_error<GenericBinaryError>("Name section ended prematurely",
                                           object_error::parse_failed);
   return Error::success();
 }
 
 Error WasmObjectFile::parseLinkingSection(const uint8_t *Ptr,
                                           const uint8_t *End) {
   HasLinkingSection = true;
   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--) {
         StringRef Symbol = readString(Ptr);
         DEBUG(dbgs() << "reading syminfo: " << Symbol << "\n");
         uint32_t Flags = readVaruint32(Ptr);
         auto iter = SymbolMap.find(Symbol);
         if (iter == SymbolMap.end()) {
           return make_error<GenericBinaryError>(
               "Invalid symbol name in linking section: " + Symbol,
               object_error::parse_failed);
         }
         uint32_t SymIndex = iter->second;
         assert(SymIndex < Symbols.size());
         Symbols[SymIndex].Flags = Flags;
         DEBUG(dbgs() << "Set symbol flags index:"
                      << SymIndex << " name:"
                      << Symbols[SymIndex].Name << " exptected:"
                      << Symbol << " flags: " << Flags << "\n");
       }
       break;
     }
     case wasm::WASM_DATA_SIZE:
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];
+    switch (Global.InitExpr.Opcode) {
+      case wasm::WASM_OPCODE_I32_CONST:
         return Global.InitExpr.Value.Int32;
+      case wasm::WASM_OPCODE_I64_CONST:
+        return Global.InitExpr.Value.Int64;
+      default:
+        return 0;
+    }
   }
   case WasmSymbol::SymbolType::DEBUG_FUNCTION_NAME:
     return Sym.ElementIndex;
   }
   llvm_unreachable("invalid symbol type");
 }
 
 uint32_t WasmObjectFile::getSymbolAlignment(DataRefImpl Symb) const {