Skip to content

Conversation

Mandragorian
Copy link
Contributor

Closes #3749

@Mandragorian
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot added the S-waiting-on-review Status: Waiting for a review to complete label Sep 13, 2024
/// Additional data that we attach with each cond instance.
struct AdditionalCondData {
/// The address of the cond.
address: u64,
Copy link
Member

@RalfJung RalfJung Sep 13, 2024

Choose a reason for hiding this comment

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

Please also store the clock id in here instead of in-memory, similar to what you did with the mutex kind.

"PTHREAD_COND_INITIALIZER is incompatible with our pthread_cond layout: clock is not CLOCK_REALTIME"
);
}

offset
}

#[derive(Debug, Clone, Copy)]
enum ClockType {
Copy link
Member

Choose a reason for hiding this comment

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

We call it the clock "ID" in most places, and the C type name for this is clockid_t. So ClockId seems like a better name for this type?

@@ -902,13 +912,13 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
return Ok(());
}
};
let timeout_clock = if is_cond_clock_realtime(this, clock_id) {
let timeout_clock = if matches!(clock_type, ClockType::Realtime) {
Copy link
Member

Choose a reason for hiding this comment

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

Please make this a match clock_id { ... }

@@ -933,7 +943,6 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {

// Destroying an uninit pthread_cond is UB, so check to make sure it's not uninit.
cond_get_id(this, cond_op)?;
Copy link
Member

Choose a reason for hiding this comment

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

I realize that's pre-existing, but -- this function is already called just above, so this call can be removed. Please preserve the comment though.

@RalfJung
Copy link
Member

Looks good, thanks. :)

Can you squash this into a single commit?

@Mandragorian
Copy link
Contributor Author

Thanks for the blazingly fast reviews!

@RalfJung
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Sep 14, 2024

📌 Commit b1aaf1a has been approved by RalfJung

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Sep 14, 2024

⌛ Testing commit b1aaf1a with merge 143ec4d...

@bors
Copy link
Contributor

bors commented Sep 14, 2024

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing 143ec4d to master...

@bors bors merged commit 143ec4d into rust-lang:master Sep 14, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Waiting for a review to complete
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pthreads synchronization primitives: detect mutex/rwlock/... being moved to a different location
4 participants