WASI icon indicating copy to clipboard operation
WASI copied to clipboard

command and extra exports

Open yamt opened this issue 2 years ago • 14 comments

recent versions of llvm wasm-ld inserts ctor/dtor for every exports for a command. [1] it broke a few use cases including wamr's malloc/free exports. [2]

while preview2 slide [3] p7 says "No other exports", my understanding is that component-model will use some exports like canonical_abi_realloc/free, which are essentially the same as what the above mentioned wamr malloc/free exports do.

anyway, whatever we will do for preview2, it's better to unbreak the existing use cases. IMO, the wasm-ld patch in question should be reverted, or at least conditionalized to allow plain exports. how do you think?

[1] https://reviews.llvm.org/D81689 [2] https://reviews.llvm.org/D81689#3611504 [3] https://github.com/WebAssembly/meetings/blob/main/wasi/2022/presentations/2022-06-30-gohman-wasi-preview2.pdf

yamt avatar Sep 06 '22 09:09 yamt

From https://reviews.llvm.org/D81689: "This new behavior is disabled when the input has an explicit call to __wasm_call_ctors, indicating code not expecting new-style command support."

This means that if you export a traditional _start function (which should call __wasm_call_ctors) then the new behaviour should not kick in. In your examples that broke, how was __wasm_call_ctors being called?

sbc100 avatar Sep 06 '22 09:09 sbc100

From https://reviews.llvm.org/D81689: "This new behavior is disabled when the input has an explicit call to __wasm_call_ctors, indicating code not expecting new-style command support."

This means that if you export a traditional _start function (which should call __wasm_call_ctors) then the new behaviour should not kick in. In your examples that broke, how was __wasm_call_ctors being called?

i'm sure the new behavior kicks in for simple programs like https://github.com/yamt/garbage/tree/8dda71f39714720c921533324290b8523e263e5a/wasm/hello

yamt avatar Sep 06 '22 09:09 yamt

Putting it another way: Assuming the new behaviour can easily be avoided for those that what the more transitional single-entry command (with extra exports that are not entry point) would that be a satisfactory solution for you?

Perhaps there is some kind of bug in the automatic fallback to the old behaviour.

sbc100 avatar Sep 06 '22 09:09 sbc100

IIRC wasi-libc's _start function should trigger the old behavior because it calls __wasm_call_ctors itself: https://github.com/WebAssembly/wasi-libc/blob/2057ce9262f76f7ef5a2002fa16da219e2176896/libc-bottom-half/crt/crt1.c#L9

Perhaps I'm missing something here? @sunfishcode ?

sbc100 avatar Sep 06 '22 09:09 sbc100

Putting it another way: Assuming the new behaviour can easily be avoided for those that what the more transitional single-entry command (with extra exports that are not entry point) would that be a satisfactory solution for you?

yes. i couldn't find a nice workaround though. (i don't want to modify application C sources for this.)

yamt avatar Sep 06 '22 10:09 yamt

IIRC wasi-libc's _start function should trigger the old behavior because it calls __wasm_call_ctors itself: https://github.com/WebAssembly/wasi-libc/blob/2057ce9262f76f7ef5a2002fa16da219e2176896/libc-bottom-half/crt/crt1.c#L9

iirc llvm picks crt1-command.o if it exists. (this one: https://github.com/WebAssembly/wasi-libc/blob/2057ce9262f76f7ef5a2002fa16da219e2176896/libc-bottom-half/crt/crt1-command.c)

yamt avatar Sep 06 '22 10:09 yamt

Ah I see, I that case perhaps its wasi-sdk, or clang it self that should have some kind of option for force the old style crt1 to be chosen.

sbc100 avatar Sep 06 '22 10:09 sbc100

Ah I see, I that case perhaps its wasi-sdk, or clang it self that should have some kind of option for force the old style crt1 to be chosen.

probably.

btw, how do you think about my concern about the interaction with canonical_abi_realloc/free?

yamt avatar Sep 06 '22 10:09 yamt

Yes, clearly canonical_abi_mallc/realloc would never want to be wrapped in calls to ctors/dtors. Linker support for canonical ABI stuff is yet to be decided IIUC.. its still being thought about/worked on. For example, I've heard talk of making commands run main at instantiation time.. I'm not sure how allocation works in that case since exports are not yet available to the host (perhaps I'm confusing component instantiation with module instantiation though).

sbc100 avatar Sep 06 '22 10:09 sbc100

Yes, I now think we should make a change along these lines. I'm attempting to move deliberately here, to make sure that we don't cause more churn as a result of this change than we need to.

The currently-implemented concept of "new-style commands" where every entrypoint runs the constructors was based on preliminary specs for wasm commands that predate the component model and the canonical ABI. Now, the underlying standards have evolved to the point where those ideas no longer make sense. So I think it makes sense to drop the "run ctors on all exports" mode.

I can also add that the current thinking for components is that for now, wasm-ld will continue to emit core wasm modules, and we'll have a separate tool convert them into components (this may change over time, but we can consider how that works separately). So we can focus on core modules here.

sunfishcode avatar Sep 14 '22 21:09 sunfishcode

I've now submitted https://github.com/WebAssembly/wasi-libc/pull/328, which changes wasi-libc to always call __wasm_call_ctors for commands, which tells wasm-ld not to emit those calls itself.

sunfishcode avatar Sep 28 '22 21:09 sunfishcode

Either calling __wasm_call_ctors or linking with -Wl,--export=__wasm_call_ctors (from https://github.com/bytecodealliance/wasm-micro-runtime/pull/1255) should work as a workaround. See https://github.com/WebAssembly/WASI/issues/471 for previous discussion.

PiotrSikora avatar Sep 28 '22 21:09 PiotrSikora

Either calling __wasm_call_ctors or linking with -Wl,--export=__wasm_call_ctors (from bytecodealliance/wasm-micro-runtime#1255) should work as a workaround. See #471 for previous discussion.

the former needs modification to app code, which i don't want to have. the latter still needs someone to call ctors.

yamt avatar Sep 29 '22 03:09 yamt

Yes, the workaround is useful for some people, and works today. The fix will be useful to everyone, once it's published, as from that point on the toolchain will just do the right thing (now that we know what the right thing is).

sunfishcode avatar Sep 29 '22 20:09 sunfishcode