HB-relationship involving thread creations while mutexes are held#1913
HB-relationship involving thread creations while mutexes are held#1913dabund24 wants to merge 39 commits intogoblint:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements the second part of happens-before (HB) relationship analysis for thread creations while mutexes are held. It introduces two new analyses (MustlockHistory and DescendantLockset) that work together to detect race conditions by establishing happens-before relationships between thread operations based on mutex locking patterns.
Changes:
- Added
MustlockHistoryanalysis to track which threads have locked specific mutexes - Added
DescendantLocksetanalysis to compute descendant locksets and determine happens-before relationships - Extended
CreationLocksetanalysis with query support for integration with the new analyses - Added 20 comprehensive test cases covering both race-free and racing scenarios
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
src/analyses/mustlockHistory.ml |
New analysis tracking mutex lock history per thread |
src/analyses/descendantLockset.ml |
New analysis computing descendant locksets and HB relationships |
src/analyses/creationLockset.ml |
Added CreationLockset query support |
src/domains/queries.ml |
Added CreationLockset and MustlockHistory query types with supporting domains |
src/goblint_lib.ml |
Exported the two new analysis modules |
tests/regression/53-races-mhp/40-45-*.c |
Race-free test cases validating correct HB detection |
tests/regression/53-races-mhp/50-59-*.c |
Racing test cases validating race detection |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… of descendant lockset analysis
|
Random thought: What do your analyses do for recursive mutexes? Are they sound in these cases? |
|
Thanks for bringing this up, those would never have crossed my mind. I think the analyses remain sound, but get less precise. The only relevant thing changing here coming to my mind is the fact that after an unlock, we can't assume anymore that the mutex is now unlocked. As unlock statements have been places in our analyses, where we assume things to just break, but not start/keep working, this wouldn't be an issue. |
👍 Could you add tests here and maybe also include some tests for your first analysis (potentially in a separate PR)? |
|
@DrMichaelPetter and I decided to remove the flow-insensitive analysis detecting cases like the third example, since we did not see a simple way of both having only monotonic contributions and being certain that the analysis is sound |
|
Just so I can understand: why is monotonicity important here? @dabund24 @DrMichaelPetter (By default (without update rule) globals are accumulated, so their values grow monotonically anyway) |
|
I'm not deeply familiar with what exactly you're trying to do, but in general our fixpoint solvers can deal with non-monotonic right-hand sides without any issues. ==== Nothing wrong with copying values from a MapBot to a MapTop, though it usually doesn't do much for globals. If the binding is not present, it is assumed to be top, so contributing more to it will not really have much of an effect... |
|
Thanks for remarking this. In that case, I'm going to undo the changes |
|
No, please discuss with @DrMichaelPetter what he suggests before doing so. |
|
I re-added the |
michael-schwarz
left a comment
There was a problem hiding this comment.
Sorry for the stall here. I think we should now try to get this merged so it evolves with the rest of the system. Can you merge master into this (or rebase, whatever you prefer) and address the comments?
Then it should be good to merge!
| @@ -0,0 +1,31 @@ | |||
| // PARAM: --set ana.activated[+] threadJoins --set ana.activated[+] threadDescendants --set ana.activated[+] mustlockHistory --set ana.activated[+] descendantLockset --set "ana.activated[+]" creationLockset | |||
There was a problem hiding this comment.
| // PARAM: --set ana.activated[+] threadJoins --set ana.activated[+] threadDescendants --set ana.activated[+] mustlockHistory --set ana.activated[+] descendantLockset --set "ana.activated[+]" creationLockset | |
| // PARAM: --set ana.activated[+] threadJoins --set ana.activated[+] threadDescendants --set ana.activated[+] mustlockHistory --set ana.activated[+] descendantLockset --set ana.activated[+] creationLockset |
| @@ -0,0 +1,36 @@ | |||
| // PARAM: --set ana.activated[+] threadJoins --set ana.activated[+] threadDescendants --set ana.activated[+] mustlockHistory --set ana.activated[+] descendantLockset --set "ana.activated[+]" creationLockset | |||
There was a problem hiding this comment.
"ana.activated[+]" creationLockset should be ana.activated[+] creationLockset here as well. Probably easiest to do with a search & replace.
|
|
||
| int main(void) { | ||
|
|
||
| int maybe; |
There was a problem hiding this comment.
With the goal of turning these into SV-COMP tests later, it may make sense to replace all occurences where we rely on uninitialized locals for non-determinism with the proper nondet functions.
|
|
||
| (** [{ t_0 |-> { t_d |-> L } }] | ||
|
|
||
| [{ t_d |-> L }] is the descendant lockset valid for the [V] value, |
There was a problem hiding this comment.
What is [V] supposed to mean here?
| module V = struct | ||
| include TID | ||
| include StdV | ||
| end |
There was a problem hiding this comment.
I think we have four instances of this now, maybe add a TIDV to analyses.ml?
| if Lockset.is_bot locks_held_creating_t2 | ||
| then false | ||
| else ( | ||
| let relevant_lh2_threads = | ||
| Lockset.fold | ||
| (fun lock -> TIDs.union (Queries.LH.find lock lh2)) | ||
| locks_held_creating_t2 | ||
| (TIDs.empty ()) | ||
| in | ||
| TIDs.exists | ||
| (fun t_lh -> | ||
| TID.must_be_ancestor t1 t_lh | ||
| && (TID.equal t_lh t2 || TID.must_be_ancestor t_lh t2)) | ||
| relevant_lh2_threads) |
There was a problem hiding this comment.
The indentation is a little strange here. Also, I don't think you need the ( ) around the else case.
| (** descendant lockset analysis [descendantLockset] | ||
| analyzes a happened-before relationship related to thread creations with mutexes held. | ||
|
|
||
| Enabling [creationLockset] may improve the precision of this analysis. | ||
|
|
||
| @see https://github.com/goblint/analyzer/pull/1923 | ||
| *) |
There was a problem hiding this comment.
Maybe you should push your thesis to https://github.com/goblint/theses and then reference it here (and add something like "available upon request") until there's a formal publication.
There was a problem hiding this comment.
Same for the other analyses from your thesis.
| let tid_lifted = man.ask Queries.CurrentThreadId in | ||
| let child_tid_lifted = fman.ask Queries.CurrentThreadId in | ||
| match tid_lifted, child_tid_lifted with | ||
| | `Lifted tid, `Lifted child_tid when TID.must_be_ancestor tid child_tid -> |
There was a problem hiding this comment.
This implicitly checks uniqueness of tid. Could you elaborate why such a check is not needed for the child?
I was wondering about the case where two threads get the same non-unique id, but for one of the creations no mutex is held while one is held for the other?
main
for(int i = 0; i < 2; i++) {
if(i == 1) { lock(m) }
create(thread1) // Both copies receive same TID
}
g = 42; //RACE!
unlock(m)
thread1:
lock(m)
g = 8; // RACE!
unlock(m)
Is this handled by the lockset being a must lockset? Do you have a test for this?

second part of #1805. The first half was implemented in #1865.
closes #1805.
Summary
Simplest case: After creating$t_1$ in $t_0$ with mutex $l$ held, succeeding statements until maybe unlocking in $t_0$ must happen before everything after definitely locking $l$ in $t_1$ .
generalizations:
Examples
In the following examples,
Amust happen beforeB.Simple example
graph TB; subgraph t1; E["lock(l);"]-->F; F["unlock(l);"]-->G; G((B)) end; subgraph t0; A["lock(l);"]-->B; B["create(t1);"]-->C; C((A))-->D; D["unlock(l);"]; end; B-.->EBin a descendant ofgraph TB; subgraph t2; H((B)) end; subgraph t1; E["lock(l);"]-->F; F["create(t2);"] end; subgraph t0; A["lock(l);"]-->B; B["create(t1);"]-->C; C((A))-->D; D["unlock(l);"]; end; B-.->E F-.->HAin a descendant ofNote
This case is not covered by the analysis implemented in this Pull Request, but may be added some time later
graph TB; subgraph t1; E["lock(l);"]-->I; I["unlock(l);"]-->F; F((B)); end; subgraph t2; H((A)) end; subgraph t0; A["lock(l);"]-->B; B["create(t1);"]-->C; C["create(t2);"]-->D; D["join(t2);"]-->G; G["unlock(l);"] end; B-.->E C-.->H H-.->DHere, it is important that no unlock happens in$t_0$ before $t_2$ is joined into $t_0$ , which was computed in #1865.
Dependency Analyses
From these analyses, we compute:
create(t)all threads transitively created, for whichAnalyses
Descendant Locksets$\mathcal{DL}$
flow-sensitive
Domain:$T\to 2^L$
MapBotThere must have existed at least one$t_c$ $t_A$ with $t_B\in c^*\ t_c$ .
create()statement inFor all of those$t_c$ $\mathit{l}\in\mathcal{L}$ .
create()statements,We must not have encountered an$l$
unlock()statement after having detected the thread creation.Transfer functions
Mustlock History$\mathcal{LH}$
MapTopTransfer functions
Global descendant lockset$\mathcal{DL}_g\ t$
VMapBotContributions
We only contribute at$t_c$ $t_d\in t^*\ t_c$ :
$$DL_{t_d}:=\set{t\mapsto (\mathcal{DL}\ t)\cap (\mathcal{CL}\ t_d\ t_{\mathrm{ego}})\mid t\in T}$$
$$\mathcal{DL}_ g\ t_ d\sqsupseteq \set{t_ {\mathrm{ego}} \mapsto DL_ {t_d}}$$
create()statements for allHappened-Before rules
Statement$\mathcal{LH}_ 2, t_2$ must happen after $\mathcal{DL}_ 1, \mathcal{LH}_ 1, t_1$ , if:
s2withs1with