[PATCH] D40559: Run Wasm entrypoint on load

[Nicholas Wilson via Phabricator via llvm-commits]

ncw added a comment.

OK, I can split this review into two, one for `--undefined` and one for the `--entry` changes (I'll do it tomorrow), and I could open a third review for the `--no-entry` support.

I'm not too bothered by the `--entry ''` syntax, mainly because I expect 99% of people to be happy with the default. When we get the default right, who _wouldn't_ want to run static initialisers? I think the default should be some new name, eg `_start_wasm`, to differentiate its behaviour from the standard `_start` function, rather than redefine the behaviour of `_start` on the wasm platform to not include running main. (This would smooth integration with upstream Musl, and give developers the option of passing `--entry _start` if they _do_ want to run main, but if we actually change `_start` then there's no way to specify that you actually want the traditional behaviour.)

In https://reviews.llvm.org/D40559#939945, @ruiu wrote:

> That being said, I don't think you want to copy that hacky behavior to a shiny new file format. I'd make -entry mandatory when creating a wasm executable.


That would be a bit awkward. For a "default" C/C++ executable, we should aim to keep the link-line as short as possible, and not require the dev to know magic names for functions like `_start` that are supplied by the platform. (I'm imagining that the Clang+LLD toolchain will be usable and useful as a freestanding thing, which is how my company currently uses Emscripten - we basically provide all the JS syscalls ourselves now, and use only the Emscripten compiler.)



================
Comment at: test/wasm/data-layout.ll:3
 ; RUN: llc -filetype=obj %s -o %t.o
-; RUN: lld -flavor wasm --emit-relocs --allow-undefined -o %t.wasm %t.o %t.hello.o
+; RUN: lld -flavor wasm --emit-relocs --allow-undefined --entry '' -o %t.wasm %t.o %t.hello.o
 ; RUN: obj2yaml %t.wasm | FileCheck %s
----------------
sbc100 wrote:
> Why is this needed?  Shouldn't allow-undefined be enough here?
`--allow-undefined` doesn't allow the entrypoint to be undefined. Without an `--entry` argument you get `--entry _start` by default, and if that function isn't found then it will cause an error. I think that's the right behaviour; it wouldn't make any sense to automatically convert _start into an import, since that's not likely to be what the dev intended.


================
Comment at: wasm/Driver.cpp:301
   Config->SearchPaths = getArgs(Args, OPT_L);
+  Config->Undefined = getArgs(Args, OPT_undefined);
   Config->StripAll = Args.hasArg(OPT_strip_all);
----------------
sbc100 wrote:
> Does this need to be in Config, or can it just be a local ?
Good point, it doesn't _have_ to go in Config. But, if this is ever refactored from being one long do-it-all method, it will have to go there (see ELF driver). It's nicer on the eyes to have all the different options match here, I felt.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D40559