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 a separate install target for threaded libc #331

Merged
merged 2 commits into from
Dec 14, 2022

Conversation

penzn
Copy link
Collaborator

@penzn penzn commented Oct 5, 2022

Use MULTIARCH_TRIPLE to append 'pthread' and TARGET_TRIPLE as more generic wasm32-wasi.

To use this sysroot, one would have to compile with --triple=wasm32-wasi-pthread.

Makefile Outdated
# Threaded version necessitates a different traget, as objects from different
# targets can't be mixed together while linking
ifeq ($(THREAD_MODEL), posix)
MULTIARCH_TRIPLE = wasm32-wasi-pthread
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if its work being explicit here and calling this wasm32-unknown-wasi-pthread ?

Copy link
Member

Choose a reason for hiding this comment

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

Also, is there a reason with have TARGET_TRIPLE and MULTIARCH_TRIPLE separated here? I would that thought that we want to just use a single triple everywhere?

Copy link
Member

Choose a reason for hiding this comment

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

I propose we handle the vendor string in the same way as the non-pthread builds, so that the only difference is the suffix.

Copy link
Collaborator Author

@penzn penzn Oct 6, 2022

Choose a reason for hiding this comment

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

Also, is there a reason with have TARGET_TRIPLE and MULTIARCH_TRIPLE separated here? I would that thought that we want to just use a single triple everywhere?

Should we update both to include pthread or should we only keep one variable? Both are viable solutions I think.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe I misread your question initially. I don't know why there are two variables, but can try to make it into just one.

Copy link
Member

Choose a reason for hiding this comment

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

I don't know the precise state of things here, but my suggestion was just to avoid making the unknown vendor field explicit here, since it's implicit everywhere else and making it explicit here would make more things different.

Copy link
Collaborator Author

@penzn penzn Oct 14, 2022

Choose a reason for hiding this comment

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

Oh, I meant @sbc100's comment right before yours - it looks like he was asking why there are two variables. If there is a desire to do that I can make it into just one variable.

Makefile Outdated Show resolved Hide resolved
@penzn penzn marked this pull request as ready for review October 14, 2022 22:19
@penzn
Copy link
Collaborator Author

penzn commented Oct 14, 2022

I've added a commit to that would leave only one triple variable in the Makefile. I am not squashing it yet, so that it is easier to drop it if that is not what we need.

@penzn penzn requested a review from sbc100 October 17, 2022 16:01
Copy link
Member

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

LGTM!

Have you tested this? Does clang/llvm already support wasm32-wasi-pthread?

@penzn
Copy link
Collaborator Author

penzn commented Oct 18, 2022

Have you tested this? Does clang/llvm already support wasm32-wasi-pthread?

I just realized I didn't build compiler-rt with it, working through that, but Clang would happily accept wasm32-wasi-pthread today.

Produce a different sysroot directory for threaded target. Restructure
`expected` directory to correspond to the target.
@penzn
Copy link
Collaborator Author

penzn commented Dec 13, 2022

@sbc100 how do you normally test something like that? I have built wasi-sdk pointing to this branch with THREAD_MODEL=posix on, I can get through full build of a simple program and it gets atomics in the output.

@sbc100
Copy link
Member

sbc100 commented Dec 13, 2022

@sbc100 how do you normally test something like that? I have built wasi-sdk pointing to this branch with THREAD_MODEL=posix on, I can get through full build of a simple program and it gets atomics in the output.

Until very recently there was no testing at all in this repo and testing via integration with wasi-sdk was the only way to go. There was recently some minimal testing added to the test/ directory. I think those get run during CI so if its green you didn't break them.

If you have have manually verified via integration with wasi-sdk and the CI here is green I think thats good enough.

@penzn
Copy link
Collaborator Author

penzn commented Dec 14, 2022

I don't have commit privileges here, can you merge this for me?

@sbc100 sbc100 merged commit fb9c922 into WebAssembly:main Dec 14, 2022
john-sharratt pushed a commit to john-sharratt/wasix-libc that referenced this pull request Mar 6, 2023
Produce a different sysroot directory for threaded target. Restructure
`expected` directory to correspond to the target.
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