This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Export globals when linking with -r/--relocatable
ClosedPublic

Authored by sbc100 on Dec 5 2017, 2:46 PM.

Details

Summary

This change cleans up the way wasm exports and globals
are generated, particualrly for -r/--relocatable where
globals need to be created and exported in order for
output relocations which reference them.

Remove the need for a per file GlobalIndexOffset and
instead set the output index for each symbol directly.
This simplifies the code in several places.

Diff Detail

Repository
rL LLVM

Event Timeline

sbc100 created this revision.Dec 5 2017, 2:46 PM
sbc100 edited the summary of this revision. (Show Details)Dec 5 2017, 2:51 PM
sbc100 edited the summary of this revision. (Show Details)
sbc100 retitled this revision from [WebAssembly] Fix symbol exports under -r/--relocatable to [WebAssembly] Export globals when linking with -r/--relocatable.Dec 5 2017, 2:54 PM
sbc100 added reviewers: ruiu, ncw.
sbc100 added a comment.EditedDec 5 2017, 2:58 PM

@ncw , this is change I've be wroking on for a while now that might be help (or an annoying merge conflict!) for your work on function stripping. It basically does for globals what I imagine we would want to do for functions (i.e. eliminates the per file index offset). Sorry if it doesn't merge well with your work, but hopefully there are things that they share and could make your partch smaller?

ncw accepted this revision.Dec 6 2017, 5:27 AM

This change looks good, it's pretty much the same direction I took with the function-pointer-table.

I didn't try tackling global exports in my COMDAT work, because I don't really understand the use-case for relocatable output, and don't really know how it should work.

I don't mind a few merge conflicts, this should be fine. I can't see anything to reuse in the COMDAT work, it's more a case of doing matching changes elsewhere (for segments and table entries).

wasm/Writer.cpp
283 ↗(On Diff #125620)

Can we now get rid of the ugly WrittenToSymtab variable? It's state that doesn't properly belong on the Sym at all - much better to have a local vector like you've done.

This revision is now accepted and ready to land.Dec 6 2017, 5:27 AM
sbc100 added a comment.Dec 6 2017, 4:59 PM

@ruiu, do you prefer that I wait for your LGTM on all lld changes? I'm assuming you would be ok with small-ish wasm-only changes being peer-reviewed rather than requiring owner reviewed? e(assuming you consider yourself the owner of lld?)

ruiu edited edge metadata.Dec 6 2017, 5:04 PM

You don't need to wait for my LGTM for small-ish changes. I want to continue reviewing wasm lld patches to (1) keep wasm in line with other ports, (2) keep wasm lld as fast as possible, and (3) prevent wasm from making mistakes that other binary formats made in their history. But that's mostly about high-level design.

sbc100 added a comment.Dec 6 2017, 5:18 PM

In case it wasn't clear I was asking about this change in particular too. Want me to wait on your LG?

ruiu added a comment.Dec 6 2017, 5:21 PM

If I'm not too slow, give me a few hours to review patches. If I didn't respond timely, please go ahead without my LG. Does this sound good?

I'll do post-commit review in the latter case.

sbc100 added a comment.Dec 6 2017, 5:25 PM

Sorry, didn't mean to imply you were going slow. I just wasn't sure if I was waiting on you at all in this case since I got another LGTM. I can wait :)

ruiu accepted this revision.Dec 6 2017, 5:31 PM

LGTM :)

This revision was automatically updated to reflect the committed changes.
ncw added a comment.Dec 8 2017, 2:30 PM

I'm not at my computer, sorry if I'm wrong, but I think this change leaves an unused variable - Writer::NumGlobals if I remember correctly.