Skip to content

Move pthread TLS state native code using wasm globals #12839

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

Merged
merged 1 commit into from
Nov 22, 2020

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Nov 20, 2020

This moves pthead_self and other thread state into
native code.

@sbc100 sbc100 requested review from kripken and juj November 20, 2020 05:30
@sbc100 sbc100 force-pushed the pthread_self_native branch from a4234ec to 3b16cc3 Compare November 20, 2020 06:29
is_runtime_thread:

.globl pthread_self
pthread_self:
Copy link
Collaborator

@juj juj Nov 20, 2020

Choose a reason for hiding this comment

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

Wait, so any function that is defined in .s file gets a _ prefixed to it? So pthread_self here becomes _pthread_self?

And any functions defined in C/C++ files do not get a _ prefix?

That is actually pretty backwards: in native compilers all assembly code is unprefixed, and all C/C++ code is prefixed with _. Is there a specific reason why this was flipped for Wasm backend? That seems a bit arbitrary.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The _ prefix is added to all native symbols by the emscripten JS code. We do this purely for legacy/asm.js compatability.

The actual compiler, llvm, does not emit the _ mangling for any symbols. I think maybe the _ mangling is some kind of mac or windows convention. It certainly doesn't exist on windows.

From the linker's POV there is no way to way to the difference between a C symbol and an assembler symbols. They are all just "native" sybmols and we mangle all of them when we import them into JS.

One day we should completely remove the mangling...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I created an issue to remove this mangling completely one day: #12844

@sbc100 sbc100 force-pushed the pthread_self_native branch 3 times, most recently from 946f80d to e7f8bc6 Compare November 20, 2020 17:44
Base automatically changed from split_library_pthread_wasm to master November 20, 2020 18:47
@sbc100 sbc100 force-pushed the pthread_self_native branch from e7f8bc6 to 9ef57be Compare November 20, 2020 18:51
@sbc100 sbc100 force-pushed the pthread_self_native branch 3 times, most recently from bcc4c4f to 98c0f00 Compare November 22, 2020 05:06
This moves pthead_self and other thread state into
native code.
@sbc100 sbc100 force-pushed the pthread_self_native branch from 98c0f00 to 34142fd Compare November 22, 2020 05:45
@sbc100 sbc100 merged commit 5d3268f into master Nov 22, 2020
@sbc100 sbc100 deleted the pthread_self_native branch November 22, 2020 06:27
@kripken
Copy link
Member

kripken commented Dec 1, 2020

@sbc100

This appears to have regressed something:

./emcc tests/hello_world.c -pthread

That fails from this commit, due to error: undefined symbol: _emscripten_thread_init (referenced by $PThread__deps: ['_emscripten_thread_init','$ERRNO_CODES','emscripten_futex_wake','$killThread','$cancelThread','$cleanupThread','$PThread'], referenced by $establishStackSpace__deps: ['$PThread'], referenced by top-level compiled C/C++ code) .

-s USE_PTHREADS instead of -pthread works, however.

@kripken
Copy link
Member

kripken commented Dec 1, 2020

Oh, nm, I had to clear the cache... I forgot we don't bump the version number for things like that any more...sorry for the noise.

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