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

Reserve some TID values #16

Merged
merged 2 commits into from
Jan 4, 2023
Merged

Reserve some TID values #16

merged 2 commits into from
Jan 4, 2023

Conversation

yamt
Copy link
Contributor

@yamt yamt commented Dec 14, 2022

discussion:
#15


* TID 0 is reserved. `wasi_thread_spawn` should not return this value.

* The upper-most 3-bits of TID are always 0.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

note:

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you explain in the document why is that a requirement?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, I'm not sure if the proposal should be driven by the current implementation of it in other libraries; I think it should be other way around - unless it really makes sense to keep it like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think it really makes sense to avoid rewriting tid-based locking in musl.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you explain in the document why is that a requirement?

done

@sbc100
Copy link
Member

sbc100 commented Dec 14, 2022

@abrown, I was going to add you as a reviewer but I don't seem to have permission. How do you feel about adding me as a contributor?

@loganek
Copy link
Collaborator

loganek commented Dec 14, 2022

I don't think @abrown has necessary permission to do that (even I as a commiter to this repo cannot do it); @linclark can should be able to help with that though.

README.md Outdated
locking implementation in musl/wasi-libc.

* The third bit need to be 0 in order to make an extra room for other
reversed values in wasi-libc.

Choose a reason for hiding this comment

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

Suggested change
reversed values in wasi-libc.
reserved values in wasi-libc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops, fixed. thank you

Copy link
Collaborator

@abrown abrown left a comment

Choose a reason for hiding this comment

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

I'm fine with adding something like this to the specification, though I may be tempted to rewrite this a bit to make it a bit more clear. I think those of us doing implementation of wasi-threads should be thinking whether more bits than 3 will need to be reserved.

yamt added a commit to yamt/toywasm that referenced this pull request Dec 17, 2022
@yamt
Copy link
Contributor Author

yamt commented Jan 4, 2023

is there anything blocking this?
this is a blocker for WebAssembly/wasi-libc#360, which (or an alternative) is necessary for basic operations like printf to work.

@loganek
Copy link
Collaborator

loganek commented Jan 4, 2023

Do apologize, I was on holiday and still catching up on various threads. I'm ok with that change.

@loganek loganek merged commit 97aa487 into WebAssembly:main Jan 4, 2023
yamt added a commit to yamt/wasi-libc that referenced this pull request Jan 4, 2023
This fixes TID-based locking used within libc.

Also, initialize detach_state.

cf. WebAssembly/wasi-threads#16
wenyongh pushed a commit to bytecodealliance/wasm-micro-runtime that referenced this pull request Jan 6, 2023
According to the [WASI thread specification](WebAssembly/wasi-threads#16),
some thread identifiers are reserved and should not be used. In fact, only IDs between `1` and
`0x1FFFFFFF` are valid.

The thread ID allocator has been moved to a separate class to avoid polluting the
`lib_wasi_threads_wrapper` logic.
abrown pushed a commit to WebAssembly/wasi-libc that referenced this pull request Jan 6, 2023
This fixes TID-based locking used within libc.

Also, initialize detach_state.

cf. WebAssembly/wasi-threads#16
john-sharratt pushed a commit to john-sharratt/wasix-libc that referenced this pull request Mar 6, 2023
This fixes TID-based locking used within libc.

Also, initialize detach_state.

cf. WebAssembly/wasi-threads#16
victoryang00 pushed a commit to victoryang00/wamr-aot-gc-checkpoint-restore that referenced this pull request May 27, 2024
According to the [WASI thread specification](WebAssembly/wasi-threads#16),
some thread identifiers are reserved and should not be used. In fact, only IDs between `1` and
`0x1FFFFFFF` are valid.

The thread ID allocator has been moved to a separate class to avoid polluting the
`lib_wasi_threads_wrapper` logic.
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