Skip to content

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

Merged
compnerd merged 2 commits into
swiftlang:masterfrom
swiftwasm:swiftwasm-mutex
Jan 29, 2020
Merged

[WebAssembly] Add no-op MutexWASI.h implementation#29459
compnerd merged 2 commits into
swiftlang:masterfrom
swiftwasm:swiftwasm-mutex

Conversation

@MaxDesiatov

@MaxDesiatov MaxDesiatov commented Jan 26, 2020

Copy link
Copy Markdown
Contributor

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

MaxDesiatov commented Jan 26, 2020

Copy link
Copy Markdown
Contributor Author

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

@gribozavr gribozavr left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread include/swift/Runtime/MutexWASI.h Outdated
//
//===----------------------------------------------------------------------===//
//
// Stub implementation of Mutex, ConditionVariable, Read/Write lock, and Scoped

Copy link
Copy Markdown
Contributor

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."

@MaxDesiatov MaxDesiatov requested a review from gribozavr January 26, 2020 21:51

@gribozavr gribozavr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM!

@MaxDesiatov

Copy link
Copy Markdown
Contributor Author

@swift-ci Please smoke test

@MaxDesiatov

Copy link
Copy Markdown
Contributor Author

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

@gribozavr

Copy link
Copy Markdown
Contributor

@swift-ci Please smoke test

@MaxDesiatov

MaxDesiatov commented Jan 27, 2020

Copy link
Copy Markdown
Contributor Author

@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
Copy Markdown
Member

@swift-ci please smoke test Linux platform

@compnerd compnerd merged commit 7392c8d into swiftlang:master Jan 29, 2020
@compnerd

Copy link
Copy Markdown
Member

@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.

3 participants