[PATCH] D37497: [WebAssembly] Fix getSymbolValue() for globals that are not 32-bit const values.

[Derek Schuff via Phabricator via llvm-commits]

dschuff added inline comments.


================
Comment at: lib/Object/WasmObjectFile.cpp:794
     const wasm::WasmGlobal& Global = Globals[GlobalIndex];
-    return Global.InitExpr.Value.Int32;
+    switch (Global.InitExpr.Opcode) {
+      case wasm::WASM_OPCODE_I32_CONST:
----------------
hintonda wrote:
> sbc100 wrote:
> > hintonda wrote:
> > > Is there any reason not to handle all 5 fields?
> > 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).
> Sounds reasonable, but could you add a comment or message to your assert to that effect?  Otherwise it's sorta confusing.
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.


https://reviews.llvm.org/D37497