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

Multiset domain for recursive must-locksets #1432

Open
sim642 opened this issue Apr 24, 2024 · 0 comments
Open

Multiset domain for recursive must-locksets #1432

sim642 opened this issue Apr 24, 2024 · 0 comments
Labels
cleanup Refactoring, clean-up unsound

Comments

@sim642
Copy link
Member

sim642 commented Apr 24, 2024

#1073 introduced multiplicity counters for recursive mutexes in must-locksets. As hinted in #1073 (comment), this is simply a multiset domain. However, the implementation keeps a map of multiplicities in addition to the existing must-lockset..

During #1430, which enforces additional correctness via type-safety, numerous unsoundness issues have already been revealed:

  1. pthread_mutex_lock(&m[i]); // may lock m[1]
    pthread_mutex_lock(&m[j]); // may lock m[1] recursively
    pthread_mutex_lock(&m[0]); // must lock m[0]
    pthread_mutex_unlock(&m[k]); // may unlock m[0]
    // m[0] should not be in must-lockset here!
  2. pthread_mutex_lock(&m[0]); // must lock m[0]
    pthread_mutex_unlock(&m[i]); // may unlock m[0]
    pthread_mutex_lock(&m[0]); // must lock m[0]
    pthread_mutex_unlock(&m[0]); // must unlock m[0]
    // m[0] should not be in must-lockset here!

(Full regression tests on the mutex-recursive-unsound branch.)

I will fix these in #1430, but a clean multiset-based solution is needed to avoid futher/future instances of unsoundness. The key reason for unsoundness in these cases is that must-locksets have special add and remove behavior:

let add ((addr, _) as lock) set =
match addr with
| Addr.Addr mv when Addr.Mval.is_definite mv -> (* avoids NULL *)
add lock set
| _ ->
set
let remove ((addr, _) as lock) set =
match addr with
| Addr.Addr mv when Addr.Mval.is_definite mv -> (* avoids NULL *)
remove lock set
| _ ->
filter (fun (addr', _) ->
Addr.semantic_equal addr addr' = Some false
) set

The Multiplicity map does not, but should. However, duplicating the same logic and keeping it in sync in two data structures is error-prone. A multiset domain would avoid that.

The overhead of storing multiplicity 1 for each non-recursive must-lock is negligible, because it's just one int for each must-lock and our must-locksets are very small.

@sim642 sim642 added cleanup Refactoring, clean-up unsound pr-dependency Depends or builds on another PR, which should be merged before labels Apr 24, 2024
@michael-schwarz michael-schwarz removed the pr-dependency Depends or builds on another PR, which should be merged before label Dec 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Refactoring, clean-up unsound
Projects
None yet
Development

No branches or pull requests

2 participants