Page MenuHomePhabricator

[WebAssembly] Add first class symbol table to wasm objects
ClosedPublic

Authored by sbc100 on Feb 9 2018, 3:14 PM.

Details

Summary

The syminfo subsection is now replaced with the symtab subsection that lists all symbols.

This is combination of two patchs by Nicholas Wilson:

  1. https://reviews.llvm.org/D41954
  2. https://reviews.llvm.org/D42495

Along with a few local modifications:
Once change I made was to add the UNDEFINED bit to the binary format to avoid the
extra byte used when writing data symbols. Although this bit is redundant for other
symbols types (i.e. undefined can be implied if a function or global is a wasm import)

  • I prefer to be explicit and consistent and not have derived flags.
  • Some field renaming.
  • Some reverting of unrelated minor changes.
  • No test output differences.

Diff Detail

Repository
rL LLVM

Event Timeline

sbc100 created this revision.Feb 9 2018, 3:14 PM
sbc100 edited the summary of this revision. (Show Details)Feb 9 2018, 3:19 PM
sbc100 edited the summary of this revision. (Show Details)
sbc100 updated this revision to Diff 133699.Feb 9 2018, 3:22 PM
  • revert part
sbc100 edited the summary of this revision. (Show Details)Feb 9 2018, 3:43 PM
sbc100 updated this revision to Diff 133707.Feb 9 2018, 3:43 PM
sbc100 edited the summary of this revision. (Show Details)
  • cleanup
sbc100 edited the summary of this revision. (Show Details)Feb 9 2018, 3:43 PM
sbc100 added reviewers: sunfish, ncw.
sbc100 retitled this revision from [WebAssembly] Add first claass symbol table to wasm objects to [WebAssembly] Add first class symbol table to wasm objects.
sbc100 edited the summary of this revision. (Show Details)
ncw accepted this revision.Feb 12 2018, 3:01 AM

Great! All looks familiar, we've been rehashing this code for a fortnight so there shouldn't be much left to pick over.

You say this diff is a combination of two reviews - but I don't think it is, it looks like the second patch only. I'm sure you've got the rest of the changes there, they're just not showing up in Phabricator.

I've made some picky comments, but really I'd happy for it to be committed as-is. Just a few formatting things I think I may have fixed in later revisions - maybe you took your branch off a couple of revisions back from the last update I made? Doesn't matter.

include/llvm/BinaryFormat/Wasm.h
259 ↗(On Diff #133707)

Should this be renumbered down to 0x8 or 0x10, to use the next available bit, now that it's written out?

include/llvm/MC/MCSymbolWasm.h
23 ↗(On Diff #133707)

I thought we weren't duplicating this enum, can't it be a wasm::WasmSymbolType? Would also save the function in WasmObjectWriter to translate between the two enums.

include/llvm/Object/Wasm.h
95 ↗(On Diff #133707)

Could be else if (isDefined())

lib/MC/WasmObjectWriter.cpp
1016 ↗(On Diff #133707)

When I got rid of the Optional<WasmDataRef>, I made it so that only defined data symbols get put in DataLocations. So actually this is not only reading from DataLocations, it's inserting into it.

To avoid that, in my code I avoiding doing the insertion:

if (!WS.isData())
  Info.ElementIndex = WasmIndices[&WS];
else if (WS.isDefined())
  Info.DataRef = DataLocations[&WS];
1073–1077 ↗(On Diff #133707)

I actually simplified this in a later commit - it could go in here. I don't like all the special-casing of the stack pointer!

Import.Type = WS.getGlobalType().Type;
Import.IsMutable = WS.getGlobalType().Mutable;
1143 ↗(On Diff #133707)

I'm sure I renamed this from WasmIndex to just Index at your request...

lib/Object/WasmObjectFile.cpp
419 ↗(On Diff #133707)

Should these be checked in all builds, not just debug builds with assertions? Otherwise it's just inconsistent with the index check on the line above - in fact it could just be

if (!isValidFunctionIndex(Info.ElementIndex) ||
    IsDefined != isDefinedFunctionIndex(Info.ElementIndex))
  return make_error<>("invalid function symbol index", ...);
This revision is now accepted and ready to land.Feb 12 2018, 3:01 AM
sbc100 updated this revision to Diff 133912.Feb 12 2018, 12:10 PM
  • rebase on origin/master
sbc100 updated this revision to Diff 133930.Feb 12 2018, 2:10 PM
sbc100 marked 4 inline comments as done.
  • .
  • feedback
sbc100 updated this revision to Diff 134068.Feb 13 2018, 9:59 AM
  • rebase
sbc100 edited the summary of this revision. (Show Details)Feb 13 2018, 10:15 AM
sbc100 added inline comments.Feb 13 2018, 4:03 PM
include/llvm/BinaryFormat/Wasm.h
259 ↗(On Diff #133707)

I thought maybe it would be good idea to leave gap in case we want another bit for WASM_SYMBOL_VISIBILITY_MASK... but maybe in that case we should have make the existing masks bigger to start with.

Should I add an extra bit to BINDING_MASK and VISIBILITY_MASK for future use? Maybe no need?

include/llvm/MC/MCSymbolWasm.h
23 ↗(On Diff #133707)

Hmm.. I guess I didn't get the lastest version of your change :(

Ok, I'n happy with this and the LLD side change (https://reviews.llvm.org/D43264). Any last feed back before land?

ncw accepted this revision.Feb 14 2018, 5:15 AM

No extra feedback from me!

include/llvm/BinaryFormat/Wasm.h
259 ↗(On Diff #133707)

No preference. Using 0xc for VISIBILITY_MASK and then putting UNDEFINED at 0x10 would make sense to me, or renumbering them all to have four bits. Don't mind.

sbc100 updated this revision to Diff 134287.Feb 14 2018, 11:50 AM
  • clang-format
sbc100 updated this revision to Diff 134459.Feb 15 2018, 10:55 AM
  • update undefined bit value
sbc100 updated this revision to Diff 134463.Feb 15 2018, 11:08 AM
  • update tests
sbc100 updated this revision to Diff 134480.Feb 15 2018, 11:39 AM
  • shorten yaml names
This revision was automatically updated to reflect the committed changes.