This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Relax signature checking for undefined functions that are not called directly
ClosedPublic

Authored by sbc100 on May 20 2019, 9:58 AM.

Details

Summary

When function signatures don't match and the undefined function is not
called directly (i.e. only has its address taken) we don't issue a
warning or create a runtime thunk for the undefined function.

Instead in this case we simply use the defined version of the function.
This is possible since checking signatures of dynamic calls happens
at runtime so any invalid usage will still result in a runtime error.

This is needed to allow C++ programs to link without generating
warnings. Its not uncommon in C++ for vtables to be populated by
function address whee the signature of the function is not known in the
compilation unit. In this case clang delcares the method as void(void)
and relies on the vtable caller casting the data back to the correct
signature.

Fixes: https://bugs.llvm.org/show_bug.cgi?id=40412

Diff Detail

Repository
rL LLVM

Event Timeline

sbc100 created this revision.May 20 2019, 9:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 20 2019, 9:58 AM
sbc100 updated this revision to Diff 200322.May 20 2019, 10:04 AM
  • comments

I don't know the lld internals well, but the basic approach here sounds right.

sbc100 updated this revision to Diff 200327.May 20 2019, 10:17 AM

reserve -> resize

sbc100 updated this revision to Diff 200945.May 23 2019, 6:04 AM
  • comments

@ruiu thanks for all the reviews of late. Any chance you could take a quick look at this one today too? Otherwise perhaps @dschuff can lg?

dschuff accepted this revision.May 24 2019, 11:20 AM

Otherwise LGTM

lld/wasm/InputFiles.cpp
280 ↗(On Diff #200945)

Since this is a validation of external input, should we use report_fatal_error or some equivalent instead of assert?

337 ↗(On Diff #200945)

exclued?

This revision is now accepted and ready to land.May 24 2019, 11:20 AM
sbc100 updated this revision to Diff 201350.May 24 2019, 3:31 PM
sbc100 marked 2 inline comments as done.
  • feedback
lld/wasm/InputFiles.cpp
280 ↗(On Diff #200945)

This should already have been validated by libObject so I think this is ok .g

This revision was automatically updated to reflect the committed changes.
ruiu added inline comments.May 27 2019, 3:14 AM
lld/trunk/wasm/InputFiles.h
78

I'm not sure if I understand the code correctly. It seems like a bit indicating whether a symbol is called directly or not should logically belong to each Symbol object rather than a side table inside a file object, no? Let's say one file contains an undefined symbol, the other file contains a defined symbol, and the undefined symbol is called directly. If the linker merges the two symbols, the _defined_ symbol is called directly, isn't it?

sbc100 marked an inline comment as done.May 28 2019, 1:40 PM
sbc100 added inline comments.
lld/trunk/wasm/InputFiles.h
78

This information is tracked in the symbol, but we only need to track it for undefined functions.

This data structure is used only temporarily so that the symbols can be constructed correctly. I've made followup that should make this more obvious: https://reviews.llvm.org/D62548.