WebAssembly / WASI

WebAssembly System Interface
Other
4.85k stars 251 forks source link

command and extra exports #493

Open yamt opened 2 years ago

yamt commented 2 years ago

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

sbc100 commented 2 years ago

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?

yamt commented 2 years ago

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

sbc100 commented 2 years ago

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 commented 2 years ago

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 ?

yamt commented 2 years ago

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 commented 2 years ago

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)

sbc100 commented 2 years ago

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.

yamt commented 2 years ago

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?

sbc100 commented 2 years ago

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).

sunfishcode commented 2 years ago

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 commented 2 years ago

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.

PiotrSikora commented 2 years ago

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.

yamt commented 2 years ago

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.

sunfishcode commented 2 years ago

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).