This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by sbc100 on Sep 5 2017, 4:54 PM.

Details

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

Event Timeline

sbc100 created this revision.Sep 5 2017, 4:54 PM
sbc100 added a subscriber: llvm-commits.
dschuff added inline comments.Sep 5 2017, 5:03 PM
lib/Object/WasmObjectFile.cpp
826

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 updated this revision to Diff 113942.Sep 5 2017, 5:44 PM
  • handle 64bit global exports
sbc100 retitled this revision from [WebAssembly] Ensure that Wasm global that represent memory addresses are 32-bit const values to [WebAssembly] Fix getSymbolValue() for globals that are not 32-bit const values..Sep 5 2017, 5:45 PM
sbc100 added inline comments.
lib/Object/WasmObjectFile.cpp
826

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 added inline comments.
lib/Object/WasmObjectFile.cpp
828

Is there any reason not to handle all 5 fields?

sbc100 added inline comments.Sep 5 2017, 6:12 PM
lib/Object/WasmObjectFile.cpp
828

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 added inline comments.Sep 5 2017, 6:38 PM
lib/Object/WasmObjectFile.cpp
828

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

dschuff added inline comments.Sep 5 2017, 9:03 PM
lib/Object/WasmObjectFile.cpp
828

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 updated this revision to Diff 114044.Sep 6 2017, 12:11 PM
  • Don't treat imports/exports as symbols table unless linking
sbc100 retitled this revision from [WebAssembly] Fix getSymbolValue() for globals that are not 32-bit const values. to [WebAssembly] Only treat imports/exports as symbols when reading relocatable object files.Sep 6 2017, 12:16 PM
sbc100 edited the summary of this revision. (Show Details)

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

Minor nit...

lib/Object/WasmObjectFile.cpp
827

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");

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 accepted this revision.Sep 6 2017, 1:38 PM

otherwise LGTM

lib/Object/WasmObjectFile.cpp
361

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.

379

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.

This revision is now accepted and ready to land.Sep 6 2017, 1:38 PM
sbc100 updated this revision to Diff 114080.Sep 6 2017, 3:04 PM
  • feedback
This revision was automatically updated to reflect the committed changes.