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

Add the wasm32-wasi-pthread output to the sysroot #287

Closed
wants to merge 1 commit into from

Conversation

abrown
Copy link
Collaborator

@abrown abrown commented Jan 11, 2023

Now that the wasm32-wasi-pthread target in wasi-libc has been worked on for some amount of time, this change allows it to be released as a part of wasi-sdk packages. It should be noted somewhere (a compiler warning? README?) that this new target is not entirely stable, but that can be added as a follow-on commit. This change is mainly focused on ensuring that the build artifacts contain the new objects.

Now that the `wasm32-wasi-pthread` target in wasi-libc has been worked
on for some amount of time, this change allows it to be released as a
part of wasi-sdk packages. It should be noted somewhere (a compiler
warning? README?) that this new target is not entirely stable, but that
can be added as a follow-on commit. This change is mainly focused on
ensuring that the build artifacts contain the new objects.
@abrown
Copy link
Collaborator Author

abrown commented Jan 11, 2023

One note on this: I think we may want to change the name of this target to wasm32-wasi-threads in a future commit to wasi-libc and pull that in to wasi-sdk. The reason is that I think we should try to avoid confusion if at all possible and over in rustc (rust-lang/compiler-team#574) it is more likely that wasm32-wasi-threads is selected as the target name, since pthreads are not relevant to the Rust standard library.

@yamt
Copy link
Contributor

yamt commented Jan 12, 2023

see #274

@sbc100
Copy link
Member

sbc100 commented Jan 12, 2023

One note on this: I think we may want to change the name of this target to wasm32-wasi-threads in a future commit to wasi-libc and pull that in to wasi-sdk. The reason is that I think we should try to avoid confusion if at all possible and over in rustc (rust-lang/compiler-team#574) it is more likely that wasm32-wasi-threads is selected as the target name, since pthreads are not relevant to the Rust standard library.

Are you sure pthreads is not relevant to the Rust standard library? I feel like I've asked this question before but doesn't the rust standard library want to use pthreads under the hood? Doesn't it do this already on platforms like linux and mac? Wouldn't it make it hard to link in other C libraries that are built on pthreads if rust doesn't use pthreads but instead rolled its own threads on top of wasi-thread-spawn (or equivalently if rust use close directly instead of pthread_create on linux?)

@abrown
Copy link
Collaborator Author

abrown commented Jan 12, 2023

Are you sure pthreads is not relevant to the Rust standard library? I feel like I've asked this question before but doesn't the rust standard library want to use pthreads under the hood?

Oh, I'm not saying it shouldn't and in fact I think I've discussed with a few people that we might want to do just that--use wasi-libc's pthread support. What I'm trying to say is that "pthread" as a name could confuse things; whether or not the Rust stdlib uses pthreads is kind of an internal detail that we probably don't want users getting hung up on, IMO. It seems preferable that users have 1) a consistent target and 2) a target name that reminds us of the Wasm threads proposal and the wasi-threads proposal, not one use of it. (Hopefully I explained this well over in WebAssembly/wasi-libc#381).

@yamt
Copy link
Contributor

yamt commented Jan 13, 2023

#274 has been containing the identical change and it's more complete IMO.
it has been a draft because i'm still waiting for a critical fix in wasi-libc: WebAssembly/wasi-libc#372

@abrown
Copy link
Collaborator Author

abrown commented Jan 13, 2023

I agree that #274 is more complete. I think I will close this one; @yamt, can you modify #274 to use the new target name that just merged, wasm32-wasi-threads?

@abrown abrown closed this Jan 13, 2023
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.

3 participants