-
Notifications
You must be signed in to change notification settings - Fork 348
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
Implement rwlocks on Windows #1461
Conversation
@bors r+ |
📌 Commit 3a5bcb9 has been approved by |
Implement rwlocks on Windows Fixes #1059
impl<'mir, 'tcx> EvalContextExt<'mir, 'tcx> for crate::MiriEvalContext<'mir, 'tcx> {} | ||
pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx> { | ||
#[allow(non_snake_case)] | ||
fn AcquireSRWLockExclusive( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function could just invoke its Try*
equivalent and cause the deadlock error in case the result is 0
. That should help deduplicating the logic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you looked at an old intermediate commit -- this changed quite a bit for the final PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I entirely re-implemented rwlocks later and figured I would keep the history. Sorry for the confusion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😆 I didn't get that far yet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so the thread id assertions are gone because the logic would now support threading? (I know we don't support thread starting yet at all, but the lock logic works now correctly for threads?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct. It's the same logic as what we use on POSIX targets.
src/shims/windows/sync.rs
Outdated
// Currently write locked. This is a deadlock. | ||
throw_machine_stop!(TerminationInfo::Deadlock); | ||
} else { | ||
// Bump up read counter (cannot overflow as we just checkd against usize::MAX); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Bump up read counter (cannot overflow as we just checkd against usize::MAX); | |
// Bump up read counter (cannot overflow as we just checked against usize::MAX); |
src/shims/windows/sync.rs
Outdated
} else { | ||
// Bump up read counter (cannot overflow as we just checkd against usize::MAX); | ||
let new_val = lock_val+1; | ||
// Make sure this does not reach the "write locked" flag. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice.
@bors retry r- |
r? @oli-obk |
☀️ Try build successful - checks-travis, status-appveyor |
if let Some(current_writer) = rwlock.writer { | ||
if current_writer != expected_writer { | ||
// Only the owner can unlock the rwlock. | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be an error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We leave error reporting to the caller. Mutexes do the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done with the review. This is pretty neat, especially how lots of the impl is deduplicated.
@bors r=oli-obk |
📌 Commit 3a5bcb9 has been approved by |
☀️ Test successful - checks-travis, status-appveyor |
Fixes #1059