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

[WebAssembly] Add no-op MutexWASI.h implementation #29459

Merged
merged 2 commits into from Jan 29, 2020

Conversation

MaxDesiatov
Copy link
Member

@MaxDesiatov MaxDesiatov commented Jan 26, 2020

Since WebAssembly doesn't support threading, no locking is done in runtime. This PR adds a no-op implementation in MutexWASI.h to improve compatibility with WASI.

This is a part of SR-9307 and #24684.

(cc @kateinoigakukun @zhuowei)

@MaxDesiatov
Copy link
Member Author

MaxDesiatov commented Jan 26, 2020

@jrose-apple @stephentyrone @tbkka would any of you be available for a review please? Otherwise should I request it from anyone else? Thanks!

Copy link
Collaborator

@gribozavr gribozavr left a comment

Choose a reason for hiding this comment

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

LGTM with a small change to the comment.

//
//===----------------------------------------------------------------------===//
//
// Stub implementation of Mutex, ConditionVariable, Read/Write lock, and Scoped
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/stub/no-op/

I wouldn't say "no concrete implementation is provided" because it is a concrete implementation, it just does not perform locking.

"No-op implementation of locks for the WebAssembly System Interface. The implementation does not need to perform locking, because as of <insert WebAssembly spec version, or today's date> WebAssembly does not support threads."

Copy link
Collaborator

@gribozavr gribozavr left a comment

Choose a reason for hiding this comment

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

LGTM!

@MaxDesiatov
Copy link
Member Author

@swift-ci Please smoke test

@MaxDesiatov
Copy link
Member Author

@gribozavr could you trigger a CI run here? Thank you!

@gribozavr
Copy link
Collaborator

@swift-ci Please smoke test

@MaxDesiatov
Copy link
Member Author

MaxDesiatov commented Jan 27, 2020

@gribozavr I don't think PackageDescription4LoadingTests.testUnavailableAPIs test failure at this line could be related, is this some kind of a temporary problem or a sporadic issue? Would retriggering CI help?

@compnerd
Copy link
Collaborator

@swift-ci please smoke test Linux platform

@compnerd compnerd merged commit 7392c8d into apple:master Jan 29, 2020
@compnerd
Copy link
Collaborator

@MaxDesiatov in the future, please keep fixups to the change in a single commit in the future.

@MaxDesiatov MaxDesiatov deleted the swiftwasm-mutex branch January 29, 2020 07:14
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.

None yet

3 participants