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

detect when pthread_cond_t is moved #3884

Merged
merged 1 commit into from
Sep 14, 2024

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