Page MenuHomePhabricator

[lld][WebAssembly] Where possible handle signature mismatches via an adaptor function
AbandonedPublic

Authored by sbc100 on Oct 9 2019, 5:53 PM.

Details

Summary

This is similar to what we do at the bitcode level with the
WebAssemblyFixFunctionBitcasts pass but implemented at the object
file level.

Previously when we had caller and callee disagree about the function
signature we generated a warning replaced the call with a call to
a dummy function that contained only unreachable.

After this change we still generate the warning but will also then
try to generate an adapter function that will add or remove arguments
such that call can still possible succeed.

Event Timeline

sbc100 created this revision.Oct 9 2019, 5:53 PM
ruiu added a comment.Oct 9 2019, 7:55 PM

I'm not sure if there's a standard for the wasm object files and how the linker works, but if there's any, could you add a pointer to the document to the comment?

lld/wasm/SymbolTable.cpp
592

This function seems to enables to call an external function with a less number of arguments. I wonder what is the use case of this -- as long as you have a correct header file for functions, compilers can tell users that the number of parameters doesn't match.

sbc100 marked an inline comment as done.Oct 9 2019, 8:18 PM
lld/wasm/SymbolTable.cpp
592

Yes, normally this shouldn't happen, but sadly there are some cases in C when it does.

The motivating case here is crt1.c calling the main function from _start. We want to be able support both 3 argument and 2 arguments forms. With native linking you can simply link these two together it will kind of "just work". With wasm, before this change we end up generating a linker warning and _start ends up calling stub function, so the program doesn't work.

This change fixes that use case.

ruiu added inline comments.Oct 9 2019, 8:45 PM
lld/wasm/SymbolTable.cpp
592

I'm curious if it makes sense to limit this functionality only to "main" if there's no other use cases, so that we don't accidentally link a wrong program as if it were a correct one. What do you think?

sbc100 marked an inline comment as done.Oct 9 2019, 9:25 PM
sbc100 added inline comments.
lld/wasm/SymbolTable.cpp
592

We have seen other examples of code that does this. I remember another case in the mucl c library, although I think most of them are actual bugs that should be fixed in the code. One example is cmake which tests for the presence of a function by compiling a simple test file containing a call the the function, but the function always declared with no args. e.g. it tests for printf by compiling and linking a call to a no-arg version of printf.

We already successfully link such broken programs which is why cmake works today (via replaceWithUnreachable). This change just takes it one step further and allows some such programs to also execute.

With this change we still do give a warning (and that be be turned into an error) so we are still encouraging developers to fix their code. But we want to be permissive here to allow as much legacy code as possible o be compiled and run.

ruiu added inline comments.Oct 9 2019, 9:28 PM
lld/wasm/SymbolTable.cpp
592

Thank you for the detailed explanation. That helped me a lot. Could you add that here as a comment?

sbc100 updated this revision to Diff 224340.Oct 10 2019, 7:38 AM
  • add docs
sbc100 updated this revision to Diff 224695.Oct 11 2019, 3:11 PM
  • rebase

friendly ping..

Add @sunfish who suggested maybe we don't this might be necessary since the issue with the zero argument main is already handled at compile time.

I still think this change is somewhat nicer, and it means we can drop the magic handling of main from the compiler.

ruiu added a comment.Oct 15 2019, 3:25 AM

Do you want to land this or abandon? I think I'm fine either relying on a compiler magic or landing this patch (and remove the compiler hack), but I'm not very happy about landing this and leaving the compiler hack, so I guess you need to make your mind.

Do you want to land this or abandon? I think I'm fine either relying on a compiler magic or landing this patch (and remove the compiler hack), but I'm not very happy about landing this and leaving the compiler hack, so I guess you need to make your mind.

That sounds reasonable. We can take some time to consider which option is better and either abandon this change to land it along with the removal of the compiler hack.

sbc100 abandoned this revision.Fri, Nov 8, 3:47 PM

I'm going at abandon this for now.