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

Refactor must-locksets to use definite mvals instead of addresses #1430

Merged
merged 47 commits into from
May 27, 2024

Conversation

sim642
Copy link
Member

@sim642 sim642 commented Apr 23, 2024

Currently must-locksets are (reversed) sets of addresses, which can be the usual addresses (as mvals), but the type also allows UnknownPtr, NullPtr and StrPtr. None of these should be in must-locksets, but currently this is enforced by the combination of three different checks in three different files.

The goal of this PR is to refine the type of must-lockset elements from addresses to mvals with definite indices, whereby this correctness property is enforced by the type system.
This immediately paid off: it revealed two cases of unsoundness in recursive mutex handling because definite indices were only checked for the must-lockset, but not for the recursive locking counter map (which must duplicate all the set logic to avoid becoming unsound). See #1432.

Hopefully this also allows some Addr wrapping and unwrapping to be avoided, which should be less verbose.

TODO

  • Enforce definite indices in must-locks by type.
  • Rename Simple.
  • Add all and is_all operations to Simple.
  • Fix NULL warning in cram test.

@sim642 sim642 marked this pull request as ready for review April 24, 2024 15:32
@sim642
Copy link
Member Author

sim642 commented Apr 24, 2024

This goes quite a long way to clean things up, but not as far as I would've liked. The oddity is that privatizations get the mutex as an AddressDomain address, which can still be any of the odd things (UnknownPtr, NullPtr or ambiguous index).

@michael-schwarz What are privatizations supposed to do with those? It seems to me that we completely forgot about the non-definite possibilities, which might make things even unsound. Privatizations may have to be adapted to behave more like the must-lockset analysis: only lock definite addresses and unlock should unlock all semantic_equal addresses in case of non-definite.

@sim642 sim642 changed the title Refactor must-locksets to use mvals instead of addresses Refactor must-locksets to use definite mvals instead of addresses Apr 24, 2024
@sim642 sim642 removed their assignment Apr 29, 2024
@michael-schwarz michael-schwarz self-requested a review May 8, 2024 07:45
@michael-schwarz
Copy link
Member

What are privatizations supposed to do with those? It seems to me that we completely forgot about the non-definite possibilities, which might make things even unsound.

I think I considered this issue when designing the privatizations. They should still be sound for the original setting according to the following arguments:

  • For lock & mine-W: Incorporating too many values early makes the analysis imprecise, not unsound; All other operations are based on must-locksets, and for background locksets and minimal locksets only definite elements are kept
  • For write: lock is a no-op, other actions are based on must-lockset
  • For per-mutex flavors incl. relational & protection-based: Protection by one of these indefinite addresses means they are always held when the global is accessed, so that is not harmful either

However, these are indeed a bit shaky in some places and may easily be subject to breakage. So in the long run, it does make sense to refactor this as well.

Also, with the new additions such as atomic and invariants about unprotected globals, I'm not sure if this still all holds.

@sim642 sim642 added this to the v2.4.0 milestone May 27, 2024
@sim642 sim642 merged commit bf089e7 into master May 27, 2024
21 checks passed
@sim642 sim642 deleted the lock-mval branch May 27, 2024 12:04
sim642 added a commit that referenced this pull request May 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants