Page MenuHomePhabricator

Thread safety analysis: Warn when demoting locks on back edges
ClosedPublic

Authored by aaronpuchert on Jul 23 2021, 2:06 PM.

Details

Summary

Previously in D104261 we warned about dropping locks from back edges,
this is the corresponding change for exclusive/shared joins. If we're
entering the loop with an exclusive change, which is then relaxed to a
shared lock before we loop back, we have already analyzed the loop body
with the stronger exclusive lock and thus might have false positives.

There is a minor non-observable change: we modify the exit lock set of a
function, but since that isn't used further it doesn't change anything.

Diff Detail

Event Timeline

aaronpuchert requested review of this revision.Jul 23 2021, 2:06 PM
aaronpuchert created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJul 23 2021, 2:06 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
aaronpuchert retitled this revision from Thread safety: Warn when demoting locks on back edges to Thread safety analysis: Warn when demoting locks on back edges.Jul 24 2021, 5:45 AM

This seems reasonable to me, but I leave it to @delesley for the final sign-off.

aaron.ballman accepted this revision.Sep 13 2021, 6:50 AM

Accepting the review -- if you don't hear back from @delesley in the next few days, I think it's fine to land.

This revision is now accepted and ready to land.Sep 13 2021, 6:50 AM
This revision was landed with ongoing or failed builds.Sep 18 2021, 5:02 AM
This revision was automatically updated to reflect the committed changes.

@aaron.ballman, since this is reintroducing some warnings after the relaxation in D102026, should we bring this to Clang 13?

@aaron.ballman, since this is reintroducing some warnings after the relaxation in D102026, should we bring this to Clang 13?

I think that's reasonable; the alternative is that we warn in Clang 12, don't warn in Clang 13, then start warning again in Clang 14 which does not seem very user friendly.

@aaron.ballman, since this is reintroducing some warnings after the relaxation in D102026, should we bring this to Clang 13?

I think that's reasonable; the alternative is that we warn in Clang 12, don't warn in Clang 13, then start warning again in Clang 14 which does not seem very user friendly.

@tstellar, could we still bring this into release/13.x? Doesn't fix a crash, but it prevents inconsistencies between our releases and I also tested it on a pretty large code base. So I'm pretty confident it doesn't regress anything.