This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Fix bug is function signature checking
ClosedPublic

Authored by sbc100 on Feb 16 2018, 11:10 AM.

Details

Summary

This bug effected undefined symbols that were resolved by
existing defined symbols. We were skipping the signature
check in this case.

Diff Detail

Repository
rLLD LLVM Linker

Event Timeline

sbc100 created this revision.Feb 16 2018, 11:10 AM
Harbormaster completed remote builds in B15083: Diff 134661.
Harbormaster completed remote builds in B15084: Diff 134662.
ruiu added inline comments.Feb 16 2018, 11:19 AM
wasm/Symbols.h
97

This class have setFunctionType and getFunctionType, but this variance doesn't seem to be guaranteed

Obj.setFunctionType(X);
assert(X == Obj.getFunctionType());

because a value given to setFunctionType is ignored if this class have a non-null Chunk. Isn't that confusing?

sbc100 added inline comments.
wasm/Symbols.h
97

setFunctionType has an assert(!Chunk) which i think covers that case.

Function-bitcast stuff looks good to me

ruiu added a comment.Feb 16 2018, 11:41 AM

If we can guarantee the invariance by data structure itself, that's better than guaranteeing the same thing by code (which is asserts). Does it make sense to read function type on symbol instantiation time?

sbc100 added a comment.EditedFeb 16 2018, 11:43 AM

If we can guarantee the invariance by data structure itself, that's better than guaranteeing the same thing by code (which is asserts). Does it make sense to read function type on symbol instantiation time?

Oh I see, so getFunction() can then just return the member, rather than looking at the chunk. Sure I think that makes sense. Can we do that a separate change though?

ruiu accepted this revision.Feb 16 2018, 11:44 AM

LGTM. Yes you can. :)

This revision is now accepted and ready to land.Feb 16 2018, 11:44 AM
This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.
ncw added a comment.Feb 16 2018, 12:46 PM

That's great, thanks for splitting it out. The longer-term fix I think is to just set FunctionType always (for defined and undefined symbols), rather than have the confusion of a member that's sometimes meaningful and sometimes not; less error-prone to keep the values of all the members in sync with each other (where meaningful).