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.

Sign up for GitHub

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

Jump to bottom

revise Hermit's mutex interface to support the behaviour of StaticMutex #77610

Merged
merged 13 commits into from
Oct 25, 2020
Merged

revise Hermit's mutex interface to support the behaviour of StaticMutex #77610

merged 13 commits into from
Oct 25, 2020

Conversation

stlankes
Copy link
Contributor

@stlankes stlankes commented Oct 6, 2020 •
edited
Loading

#77147 simplifies things by splitting this Mutex type into two types matching the two use cases: StaticMutex and MovableMutex. To support the new behavior of StaticMutex, we move part of the mutex implementation into libstd.

The interface to the OS changed. Consequently, I removed a few functions, which aren't longer needed.

@rust-highfive
Copy link
Collaborator

r? @LukasKalbertodt

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 6, 2020
@stlankes stlankes mentioned this pull request Oct 6, 2020
Closed
@m-ou-se
Copy link
Member

m-ou-se commented Oct 7, 2020 •
edited
Loading

new behavior of StaticMutex

The behaviour shouldn't have changed. I only gave it a name. Using sys::(hermit::)Mutex like this (without .init() and .destroy()), was already done by all of std's static Mutexes before #77147.

Did #77147 specifically break something for you? Or was this already a problem before?

@stlankes
Copy link
Contributor Author

stlankes commented Oct 7, 2020 •
edited
Loading

I recognize the problem just after your commit. Maybe it was already there before. In my old approach, StaticMutex' are sometimes not correctly initialized.

@stlankes
Copy link
Contributor Author

stlankes commented Oct 9, 2020

My fix to pass the format check was wrong. I fixed it and it works on my systems.

@m-ou-se
Copy link
Member

m-ou-se commented Oct 12, 2020

Does std compile for hermit now? Because it looks like type MovableMutex = Box<Mutex>; (or type MovableMutex = Mutex;) is missing for this platform in sys/hermit/mutex.rs.

@stlankes
Copy link
Contributor Author

My local version seems to work. After an update, std was broken. I fixed it with my latest commits

@stlankes
Copy link
Contributor Author

The latest commit isn't related to the Mutex interface. The alloc crate provides now a new symbol __rg_oom. Our kernel and also Hermit's "user space" based on this crate. To avoid duplicated symbols, I moved a version to our libos.

@stlankes
minor changes to pass the format check
77d9831
@stlankes
box mutex to get a movable mutex
bf268fe 
the commit avoid an alignement issue in Mutex implementation
@stlankes
Copy link
Contributor Author

stlankes commented Oct 14, 2020 •
edited
Loading

@m-ou-se When should I use type MovableMutex = Box<Mutex>;? If I don't used a boxed type, the kernel triggers some exceptions because a few data types aren't aligned.

By the way, the current version works and I created a docker container, which provide the nightly compiler for HermitCore.

@m-ou-se
Copy link
Member

m-ou-se commented Oct 14, 2020

@m-ou-se When should I use type MovableMutex = Box<Mutex>;?

A Box should only be used there if a Mutex object may not be moved (while not borrowed/used), e.g. because it might have internal pointers. Posix pthread_mutex_t needs it. Most Mutex implementations don't need it and should use type MovableMutex = Mutex;.

If I don't used a boxed type, the kernel triggers some exceptions because a few data types aren't aligned.

That sounds like a type (Mutex, SpinLock, or PriorityQueue?) is missing a #[repr(align(..))].

@stlankes
Copy link
Contributor Author

That sounds like a type (Mutex, SpinLock, or PriorityQueue?) is missing a #[repr(align(..))].

Good point, I will check it.

@stlankes
Copy link
Contributor Author

stlankes commented Oct 15, 2020 •
edited
Loading

That sounds like a type (Mutex, SpinLock, or PriorityQueue?) is missing a #[repr(align(..))].

Good point, I will check it.

No, that isn't the problem. I would say the current version is fine. @LukasKalbertodt If you like, you are able to review the PR.

@stlankes
Copy link
Contributor Author

@dtolnay Do you have time to review the code?

@m-ou-se
Copy link
Member

m-ou-se commented Oct 23, 2020

@bors r+

I did not review this in detail, as Hermit is a Tier 3 platform.

@bors
Copy link
Contributor

bors commented Oct 23, 2020

📌 Commit bf268fe has been approved by m-ou-se

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 23, 2020
@jonas-schievink jonas-schievink mentioned this pull request Oct 23, 2020
Closed
This was referenced Oct 23, 2020
Closed
Closed
@m-ou-se m-ou-se mentioned this pull request Oct 23, 2020
Closed
@jonas-schievink jonas-schievink mentioned this pull request Oct 24, 2020
Closed
@jonas-schievink jonas-schievink mentioned this pull request Oct 24, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 24, 2020
@bors
Auto merge of rust-lang#78334 - jonas-schievink:rollup-z0gzbmm, r=jon…
f58ffc9 
…as-schievink

Rollup of 12 pull requests

Successful merges:

 - rust-lang#75115 (`#[deny(unsafe_op_in_unsafe_fn)]` in sys/cloudabi)
 - rust-lang#76614 (change the order of type arguments on ControlFlow)
 - rust-lang#77610 (revise Hermit's mutex interface to support the behaviour of StaticMutex)
 - rust-lang#77830 (Simplify query proc-macros)
 - rust-lang#77930 (Do not ICE with TraitPredicates containing [type error])
 - rust-lang#78069 (Fix const core::panic!(non_literal_str).)
 - rust-lang#78072 (Cleanup constant matching in exhaustiveness checking)
 - rust-lang#78119 (Throw core::panic!("message") as &str instead of String.)
 - rust-lang#78191 (Introduce a temporary for discriminant value in MatchBranchSimplification)
 - rust-lang#78272 (const_evaluatable_checked: deal with unused nodes + div)
 - rust-lang#78318 (TyCtxt: generate single impl block with `slice_interners` macro)
 - rust-lang#78327 (resolve: Relax macro resolution consistency check to account for any errors)

Failed merges:

r? `@ghost`
@bors bors merged commit e34263d into rust-lang:master Oct 25, 2020
@rustbot rustbot added this to the 1.49.0 milestone Oct 25, 2020
This was referenced Oct 27, 2020
Merged
Closed
@mkroening mkroening deleted the dtors branch December 11, 2022 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@LukasKalbertodt LukasKalbertodt

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.49.0
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@stlankes @rust-highfive @m-ou-se @bors @LukasKalbertodt @rustbot