[PATCH] D68751: [lld][WebAssembly] Where possible handle signature mismatches via an adaptor function

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: lld/wasm/SymbolTable.cpp:592
+// types don't match the adaptor creation will tail.
+bool SymbolTable::replaceWithAdaptorFunction(FunctionSymbol *sym,
+                                             FunctionSymbol *target) {
----------------
sbc100 wrote:
> ruiu wrote:
> > sbc100 wrote:
> > > ruiu wrote:
> > > > 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.
> > > 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.
> > 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?
> 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.
Thank you for the detailed explanation. That helped me a lot. Could you add that here as a comment?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68751/new/

https://reviews.llvm.org/D68751