Page MenuHomePhabricator

[lld][WebAssebmly] Preserve custom import attributes with LTO
ClosedPublic

Authored by sbc100 on Oct 14 2019, 4:21 PM.

Details

Summary

Undefined symbols in WebAssembly can come with custom import-module
and import-field attributes. However when reading symbols from
bitcode object files during LTO those curtom attributes are not
available.

Once we compile the LTO object and read in the symbol table from the
object file we have access to these custom attributes. In this case,
when undefined symbols are added and a symbol already exists in the
SymbolTable we can't simple return it, we may need to update the
symbol's attributes.

Fixes: PR43211

Diff Detail

Event Timeline

sbc100 created this revision.Oct 14 2019, 4:21 PM
sbc100 updated this revision to Diff 224938.Oct 14 2019, 5:06 PM
  • revert debugging
  • cleanup
ruiu added a comment.Oct 15 2019, 3:08 AM

It seems that the patch description is too terse. Please explain the bug that this patch is fixing in the commit message.

lld/wasm/Driver.cpp
580 ↗(On Diff #224938)

Can you add a comment to explain what this wrapper provides?

lld/wasm/SymbolTable.cpp
396

Can you add a comment to explain what this function is supposed to do?

sbc100 updated this revision to Diff 225124.Oct 15 2019, 3:03 PM
  • feedback
sbc100 updated this revision to Diff 225125.Oct 15 2019, 3:05 PM
  • feedback
sbc100 marked 2 inline comments as done.Oct 15 2019, 3:06 PM

Sorry, I've added a lot more information to the description and in comments.

sbc100 edited the summary of this revision. (Show Details)Oct 15 2019, 3:07 PM
sbc100 retitled this revision from [lld][WebAssebmly] Fix for LTO with custom import attributes to [lld][WebAssebmly] Preserve custom import attributes with LTO.Oct 15 2019, 3:07 PM
Harbormaster completed remote builds in B39606: Diff 225125.
ruiu added inline comments.Tue, Oct 15, 8:37 PM
lld/wasm/Driver.cpp
581 ↗(On Diff #225125)

Is this code tested? (i.e. is there any test that will fail if we undo this change?)

lld/wasm/SymbolTable.cpp
457

nit: auto* -> auto *

sbc100 updated this revision to Diff 225156.Tue, Oct 15, 9:40 PM
  • clang-format
sbc100 updated this revision to Diff 225226.Wed, Oct 16, 8:02 AM
  • revert part
sbc100 marked an inline comment as done.Wed, Oct 16, 8:03 AM
sbc100 added inline comments.
lld/wasm/Driver.cpp
581 ↗(On Diff #225125)

Hmmm, you are right, this change was needed as part of an earlier version of this change. Reverting it works fine.

sbc100 updated this revision to Diff 225231.Wed, Oct 16, 8:19 AM
  • revert
ruiu accepted this revision.Wed, Oct 16, 8:31 PM

LGTM

This revision is now accepted and ready to land.Wed, Oct 16, 8:31 PM
This revision was automatically updated to reflect the committed changes.