[PATCH] D135903: [wasm-ld] Add support for calling constructors in reactors.

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added a comment.

In D135903#3856942 <https://reviews.llvm.org/D135903#3856942>, @sunfish wrote:

> In D135903#3856577 <https://reviews.llvm.org/D135903#3856577>, @sbc100 wrote:
>
>> Aren't reactors supposed to define an `_initialize` function that takes care of this?
>
> Yes, but there are two considerations. One, not everyone is calling `_initialize` today, because in some situations today things work without it, however with the change to make `malloc` have a constructor, that will change. The other is that I'm anticipating use cases, such as loading a wasm module in JS, where `_initialize` might not be automatically called, and developers won't know to manually call it.
>
>> Perhaps the idea is to make the reactor model work even for users that don't link against `crt1_reactor.o`?
>
> Yep. It'll be a transition, but that's one of the things this will enable.
>
>> In that case could we instead create a default `_initialize` function here and avoid the wrappers completely?
>
> I don't think we can avoid the wrappers, if there are constructors. The wasm start function is too early for user constructors, because it's limited in what it can do. As I mentioned above, I want to avoid the requirement that `_initialize` be called explicitly, so wrappers seem like the only option.
>
> One way we eventually can avoid the wrappers is to eliminate all the constructors, such as by writing a malloc which doesn't need them, and making other changes, and encouraging users to avoid static constructors. That is a longer-term goal though.

Sounds reasonable.

However, shouldn't the long term goal be to have all wasi loaders run the `_initialize` function correctly, as spec'd?

Also, IIUC if `_initialize` exists and calls `__wasm_call_ctors` then these wrappers won't be generated (because `__wasm_call_ctors` will be referenced)  .. so this solution doesn't help modules that contain a valid `_initialize` function.   Again it seems like fixing the runtime such that it calls initialize would be better solution here.   At least from the perspective of wasi.

What about this solution that I think works even in the fact of `_initialize` function that exists:

1. Is no `_initialize` function exists, create a synthetic one that just calls `__wasm_call_ctors`
2. Create a wrapper around `_initialize` which can detect when it has been run, or not
3. Create wrappers around all the exports that guard against `_initialize` not being called yet

One problem with this solution is that it add non-optional overhead to all reactors and even well-behaved runtimes that call `_initialize` have to then pay that price.. so I guess there should be way to disable it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135903