This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] MC: Fix for address taken aliases
ClosedPublic

Authored by sbc100 on Dec 21 2017, 1:08 PM.

Details

Summary

Previously, taking the address for an alias would
result in:
"Symbol not found in table index space"

Increase test coverage for weak aliases.

This code should be more efficient too as it avoids building the
IsAddressTaken set.

Diff Detail

Repository
rL LLVM

Event Timeline

sbc100 created this revision.Dec 21 2017, 1:08 PM
sbc100 edited the summary of this revision. (Show Details)Dec 21 2017, 1:22 PM
ncw added a comment.Dec 21 2017, 3:47 PM

Looks like it should work. One thing I wonder about is the size of the table? It looks like we're now writing out a table entry for each relocation, which is (a tiny amount of) bloat and makes function pointers not compare equal (which is irrelevant for the object file which can't run anyway). It was neater before to be writing out the table entry only once per symbol.

In https://bugs.llvm.org/show_bug.cgi?id=35625, I'd suggested augmenting the alias-loop (it could actually be pulled out into a helper function rather than copy-and-pasted, oops). Then at least you'd only have one table entry per-symbol, even if the "ideal" would be one table entry per function.

sbc100 updated this revision to Diff 127967.Dec 21 2017, 4:14 PM
  • don't duplicate table entries
In D41510#962735, @ncw wrote:

Looks like it should work. One thing I wonder about is the size of the table? It looks like we're now writing out a table entry for each relocation, which is (a tiny amount of) bloat and makes function pointers not compare equal (which is irrelevant for the object file which can't run anyway). It was neater before to be writing out the table entry only once per symbol.

In https://bugs.llvm.org/show_bug.cgi?id=35625, I'd suggested augmenting the alias-loop (it could actually be pulled out into a helper function rather than copy-and-pasted, oops). Then at least you'd only have one table entry per-symbol, even if the "ideal" would be one table entry per function.

Oops, you are totally right. I forgot to add the extra check.

In D41510#962735, @ncw wrote:

Looks like it should work. One thing I wonder about is the size of the table? It looks like we're now writing out a table entry for each relocation, which is (a tiny amount of) bloat and makes function pointers not compare equal (which is irrelevant for the object file which can't run anyway). It was neater before to be writing out the table entry only once per symbol.

In https://bugs.llvm.org/show_bug.cgi?id=35625, I'd suggested augmenting the alias-loop (it could actually be pulled out into a helper function rather than copy-and-pasted, oops). Then at least you'd only have one table entry per-symbol, even if the "ideal" would be one table entry per function.

It makes sense to me to build the indirect table via single pass through the relocations since that tells us precisely which functions have had their address taken.

ncw accepted this revision.EditedDec 22 2017, 12:40 AM

Thanks, you're right that's nicer. I'll leave the bugzilla issue open for now until the Linking.md issue is closed as well.

(The lambda could become a proper member function, just to keep the length down on these huge functions that need splitting.)

This revision is now accepted and ready to land.Dec 22 2017, 12:40 AM
This revision was automatically updated to reflect the committed changes.
ncw added inline comments.Dec 23 2017, 4:36 AM
llvm/trunk/lib/MC/WasmObjectWriter.cpp
1285

Having just approved this, I'm wondering why we have to handle the MEMORY_ADDR relocations? OK, so I can see that IsAddressTaken used to handle them - but was for address-taken globals? Since we're now inside as isFunction() block, I'd have thought that only TABLE_INDEX relocations can target the symbol.

sbc100 added inline comments.Dec 24 2017, 4:50 AM
llvm/trunk/lib/MC/WasmObjectWriter.cpp
1285

I think are you right. IsAddressTaken was built to include globals and functions but only ever used for functions, so these cases can be removed I think.

ncw added inline comments.Jan 17 2018, 6:34 AM
llvm/trunk/lib/MC/WasmObjectWriter.cpp
1285

For the record: the unused switch cases were (will be) removed in D42095