Implement the critical part of wasi_thread_start in asm #376
Conversation
It's fragile to set up the critical part of C environment in C.
| * Note: it's fragile to implement wasi_thread_start in C. | ||
| * On entry, we don't even have C stack (__stack_pointer) | ||
| * set up. Be careful when modifying this function. | ||
| */ | ||
| struct start_args *args = p; | ||
| __asm__(".globaltype __tls_base, i32\n" |
There was a problem hiding this comment.
Would it make sense to move this into the asm file too and have __wasi_thread_start_C take tls_base as a third argument?
There was a problem hiding this comment.
IMO it's better to make the asm minimum.
unlike stack pointer, it's simple to avoid using tls in C.
There was a problem hiding this comment.
But this is asm code being written inline inside of C code, which seems more complex than just doing it in the asm file. e.g. it depends on this rather obscure gcc asm syntax (I personally have to look this up every time).
I probably wouldn't argue for this if it wasn't the case that we are adding a new asm already and it defines a function that is the caller of this function. Given we have a logical place to move this inline asm it makes sense to me that it live there.
Its maybe just comes down to distaste, and perhaps mistrust of the __asm__ syntax. IIRC, the support for inline asm in for wasm in clang is pretty limited and not very well tested. we kind of made it work enough for this kind of simple case, but anything more complex and I wouldn't bet on it.
There was a problem hiding this comment.
But this is asm code being written inline inside of C code, which seems more complex than just doing it in the asm file. e.g. it depends on this rather obscure gcc asm syntax (I personally have to look this up every time).
I probably wouldn't argue for this if it wasn't the case that we are adding a new asm already and it defines a function that is the caller of this function. Given we have a logical place to move this inline asm it makes sense to me that it live there.
Its maybe just comes down to distaste, and perhaps mistrust of the
__asm__syntax. IIRC, the support for inline asm in for wasm in clang is pretty limited and not very well tested. we kind of made it work enough for this kind of simple case, but anything more complex and I wouldn't bet on it.
ok. i didn't know that the support of inline asm is limited. (do you have any references?)
i moved __tls_base stuff to asm.
There was a problem hiding this comment.
@sunfishcode might know more details as he did most of the work for make it work at all. I don't think it ever got much very testing or attention. IIUC until wasi-libc, emscripten was the only place it was used at all. Basic things like this evidently work fine, which is great, but I wouldn't bet on much more.
| .globaltype __stack_pointer, i32 | ||
| .functype __wasi_thread_start_C (i32, i32) -> () | ||
|
|
||
| .hidden wasi_thread_start |
There was a problem hiding this comment.
Logically (since this is an exported function) it maybe shouldn't be hidden?
There was a problem hiding this comment.
as far as i know, symbol visibility here is a completely separate concept from wasm export.
|
|
||
| .export_name wasi_thread_start, wasi_thread_start | ||
|
|
||
| .globaltype __stack_pointer, i32 |
There was a problem hiding this comment.
In the long run we might want to make this .S rather than a .s so we can using macros like __wasm64__ to detect pointer size. e.g.: https://github.com/emscripten-core/emscripten/blob/0d7014e1b20ead9739f1fce54b316152413601c3/system/lib/compiler-rt/stack_ops.S#L6-L12
For now this seems fine though.
There was a problem hiding this comment.
in future, maybe.
for now, i followed the style of nearby asm files.
|
|
||
| local.get 1 # start_arg | ||
| i32.load 4 # tls_base | ||
| global.set __tls_base |
There was a problem hiding this comment.
Would it be simpler just to pass these as two extra args to __wasi_thread_start_C? This would avoid using memory at all, be smaller in terms of code size, and be less brittle in terms of struct offsets.
There was a problem hiding this comment.
Would it be simpler just to pass these as two extra args to __wasi_thread_start_C? This would avoid using memory at all, be smaller in terms of code size, and be less brittle in terms of struct offsets.
i'm not sure what you mean.
do you mean to save a pthread_self() call in the C function?
There was a problem hiding this comment.
Sorry, I was misunderstanding. This code lgtm!
It's fragile to set up the critical part of C environment in C.