Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

threads: implement init of TLS and stack pointer #342

Merged
merged 6 commits into from
Nov 10, 2022

Conversation

12101111
Copy link
Contributor

No description provided.

@abrown
Copy link
Collaborator

abrown commented Oct 25, 2022

Looks like this and #343 do some of the same things; do either of you (@12101111 and @haraldh) have any comments on which PR is preferable?

@haraldh
Copy link
Contributor

haraldh commented Oct 25, 2022

lol.. I'll have a look tomorrow

Copy link
Contributor

@haraldh haraldh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

#else
new_tls_base = __copy_tls(tsd - tls_size);
tls_offset = new_tls_base - tls_base;
new = (void*)((uintptr_t)self + tls_offset);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice trick to get the new __wasilibc_pthread_self pointer.

@haraldh
Copy link
Contributor

haraldh commented Oct 26, 2022

I like this implementation more, because of how it uses a trick to fill in the new struct pthread.

The only thing missing is __init_tp() for the main struct pthread.

With some pthread_exit() fixes and function naming and signature corrections this PR works for me.

I also added more pthread funcs in this patch.

@haraldh
Copy link
Contributor

haraldh commented Oct 26, 2022

updated the above comment with new patch links

@haraldh
Copy link
Contributor

haraldh commented Oct 27, 2022

@12101111 please remove the last commit. That should go into a separate PR.

@haraldh
Copy link
Contributor

haraldh commented Oct 27, 2022

@12101111 btw, I have a working wasmtime branch to try out threading.

You have to compile your C source with -pthread -Xlinker --import-memory -Xlinker --max-memory=4294967296, then convert to wat, add (export "memory" (memory 0)) at the end and convert back to wasm. (Fix for the memory stuff would be https://reviews.llvm.org/D135898)

Then you can run it with:

$ cargo +nightly run  --features wasi-threads -- run --disable-cache --wasm-features bulk-memory,threads --wasi-modules experimental-wasi-threads <WASM_file>

@haraldh
Copy link
Contributor

haraldh commented Nov 8, 2022

Ok, in my testing I needed haraldh@be2151b for the stack to be properly aligned.

@npmccallum
Copy link

@sunfishcode Could we get a review on this?

Copy link
Member

@sunfishcode sunfishcode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a merge conflict (with #339). Would you mind rebasing this?

static void dummy_0()
{
}
weak_alias(dummy_0, __wasi_init_tp);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of using a weak symbol, could you put this under #if defined(_REENTRANT) so that it isn't included in non-threaded builds?

libc-bottom-half/crt/crt1-command.c Show resolved Hide resolved
12101111 and others added 6 commits November 10, 2022 15:39
Signed-off-by: Harald Hoyer <harald@profian.com>
Signed-off-by: Harald Hoyer <harald@profian.com>
Can't use `exit()` because it is too high level.
Have to unlock the thread list.

Signed-off-by: Harald Hoyer <harald@profian.com>
Signed-off-by: Harald Hoyer <harald@profian.com>
Signed-off-by: Harald Hoyer <harald@profian.com>
@sunfishcode sunfishcode merged commit a00bf32 into WebAssembly:main Nov 10, 2022
@sunfishcode
Copy link
Member

Looks good, thanks!

john-sharratt pushed a commit to john-sharratt/wasix-libc that referenced this pull request Mar 6, 2023
* threads: implement init of TLS and stack pointer

* fix: rename wasi_snapshot_preview2_thread_spawn to wasi_thread_spawn

Signed-off-by: Harald Hoyer <harald@profian.com>

* fix: change signature of wasi_thread_start

Signed-off-by: Harald Hoyer <harald@profian.com>

* fix: pthread_exit for WASI

Can't use `exit()` because it is too high level.
Have to unlock the thread list.

Signed-off-by: Harald Hoyer <harald@profian.com>

* fix: initialize struct pthread for the main thread

Signed-off-by: Harald Hoyer <harald@profian.com>

* fix: store the aligned stack minus `struct start_args`

Signed-off-by: Harald Hoyer <harald@profian.com>

Signed-off-by: Harald Hoyer <harald@profian.com>
Co-authored-by: Harald Hoyer <harald@profian.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants