Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Unassume benchmarking fixes #1124
Unassume benchmarking fixes #1124
Changes from all commits
027b9cd
152f34e
6d04983
f0a0566
5372a76
10881f0
90f155c
5e19b23
e018cb7
0715a04
ac8baaa
33eb674
9127dad
d512cb5
66204e4
22f6061
870a9b8
4fcfc71
409cbd1
ddecc85
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Why are we treating
pthread_cond_t
as a mutex type here? That is confusing.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.
It's just in the base analysis to ignore the condition variable struct contents.
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.
Still, for consistency, I think we should introduce a
is_cond_type
here, there are several similar functions already around.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.
It wouldn't be more consistent though because it still uses
Mutex
forValueDomain
. I don't think it's worth it to introduceCond
intoValueDomain
just for this. Mutex analyses treat condition variables via unlock-lock anyway.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.
What do you mean by this? If there is an analysis that does this, it is wrong. (Or are you talking about the second argument of
pthread_cond_wait
? That seems to be unrelated, no?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.
And we do something with condition variables, see #520.
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.
Well yes, but we don't care about the implementation detail struct of a condition variable in base, just like we don't care about it for mutexes. All this does is make the contents opaque instead of tracking a struct whose contents our
special
functions don't update and thus those contents are outright wrong.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 get that. I still think it is cleaner to have a dedicated function for this case, rather than confusing things. Maybe one should replace
Mutex
in base withOpaquePThreadType
or something like this?However, my happiness isn't tied up in this, so if you prefer we can merge as-is.