Add Owicki-Gries annotations by maul-esel · Pull Request #783 · ultimate-pa/ultimate

maul-esel · 2026-05-29T06:37:06Z

This PR adds Owicki-Gries annotations as proofs for concurrent programs, as well as algorithms to compute such annotations after verification.

🚧 This is work in progress 🚧

Specifically, we add:

basic classes for describing and validating Owicki-Gries annotations.
2 algorithms for the construction of Owicki-Gries annotations:
- a naive algorithm (which creates very large annotations, but can be useful as a baseline for comparison)
- the empire automaton-based algorithm described in our POPL'26 paper
We previously implemented some other algorithm variants, but as they have been superseeded by the POPL'26 algorithm, they are no longer part of this PR. With one exception:
a partial implementation of a refined algorithm based on directed empires: while not yet complete, this may be a promising base for future improvements to our Owicki-Gries computation.
changes in backtranslation and correctness witness (v2.1) generation that allow us to output correctness witnesses for concurrent programs from Owicki-Gries proofs.

…/ultimate into wip/dk/empire-owicki

…single assertion condition

…ther classes fields

…nt rooks

…/ultimate into wip/dk/empire-owicki

…rivate field

Also fix base name for ghost mirror variables.

# Conflicts: # trunk/source/Library-ModelCheckerUtils/src/de/uni_freiburg/informatik/ultimate/lib/modelcheckerutils/smt/predicates/PredicateUtils.java # trunk/source/TraceAbstraction/src/de/uni_freiburg/informatik/ultimate/plugins/generator/traceabstraction/HoareAnnotationComposer.java # trunk/source/TraceAbstraction/src/de/uni_freiburg/informatik/ultimate/plugins/generator/traceabstraction/TraceAbstractionStarter.java

Previously, each declaration overwrote the previous one.

- initialize to zero (constant expression) - avoid weird symbols in the name

…utomata

maul-esel · 2026-05-29T12:37:52Z

+	private final IPetriNet<L, IPredicate> mInitialNet;
+	private HashRelation<IPredicate, Transition<L, IPredicate>> mPossibleInterferences;
+
+	private static final boolean USE_ON_DEMAND_RESULT = true;


We need to be careful about this change, it affects verification performance.

Do our algorithms still rely on changing this flag? This was probably only relevant for the unfolding-based approaches (but let's check).

If we do rely on it, we either need to adapt our implementation, or at least only modify this flag if proof production is enabled.

maul-esel · 2026-05-29T12:42:27Z

+				return null;
+			}
+			return scope.getDeclarator().getName().toString();
+		}));


This was a hack to get witness production working. I vaguely recall that I either fixed the NPE that occurred here in another way, or at least realised it should be fixed in another way ;-) I think this was related to invariants / ghost updates at global variable initializations (which we probably want to avoid).

Let's check this, and if it's not strictly needed, re-assess whether this change is a good idea or not.

maul-esel · 2026-05-29T12:43:59Z

+		}
+		return result;
+	}
+


I think this duplicates some logic from annotateFork; see if they can be combined.

maul-esel · 2026-05-29T12:52:29Z

+				new DeclarationInformation(StorageClass.QUANTIFIED, null));
+		mAuxiliaryVariables.put(variable, id);
+		return id;
+	}


Should we here also try to preserve meaningful ghost variable names?

maul-esel · 2026-05-29T13:01:13Z

+import de.uni_freiburg.informatik.ultimate.util.datastructures.ImmutableSet;
+import de.uni_freiburg.informatik.ultimate.util.datastructures.relation.Pair;
+
+// TODO Give this class a more descriptive name


... and document the class

Maybe we can rename it to Empire if we call the interface IEmpire?

That seems like a less descriptive name to me :)

I think it would at least resolve any confusion, whether this class implements the empire as presented in the paper. But maybe something more descriptive would be EmpireTransitionFunctionProvider or EmpireTransitionProvider?

I am ok with getting rid of the Automaton part (though I do not feel strongly about it).

But in regard to making the name more descriptive, I meant more of an indication of which empire is computed. If I see an interface IEmpire and an implementation Empire, I would always ask why two types are needed for the same concept.

I now renamed the interface IEmpire, and this implementation SaturatedEmpire (as in the paper).

maul-esel · 2026-05-29T13:02:44Z

+	 * one transition in enabled(territory(s)), for which there is no successor in the automaton. In this case, the
+	 * successor law must be false.
+	 */
+	public boolean isFinal2(final State<L, P> state) {


Re-examine the usages of final states in this class (and the corresponding interfaces). Delete redundant methods.

After re-examination I did not find any usage of the final method of the EmpireAutomaton class (there are some relevant calls to the isFinal method of the InterpolantAutomaton though).

However as @schuessf pointed out here, there is still implicit usage of the isFinal method for states where we pruned transitions to a state with law=false. This is required to find entry points for the legal focus computation! What would be a nice name for such a method?

maul-esel · 2026-05-29T13:21:10Z

+import de.uni_freiburg.informatik.ultimate.automata.statefactory.IStateFactory;
+import de.uni_freiburg.informatik.ultimate.lib.modelcheckerutils.smt.predicates.IPredicate;
+
+// TODO Possibly rename to IEmpire


... and document

maul-esel · 2026-05-29T13:22:09Z

+ * automaton. For each region, the assigned law must be weaker than the state's full law, and satisfy additional
+ * conditions.
+ *
+ * TODO Document additional conditions


(check in the paper)

maul-esel · 2026-05-29T13:25:41Z

+import de.uni_freiburg.informatik.ultimate.util.datastructures.relation.HashRelation;
+import de.uni_freiburg.informatik.ultimate.util.datastructures.relation.Pair;
+
+public class LegalFocus<S, L, P> implements ILegalFocusFunction<S, P> {


document this class

maul-esel · 2026-05-29T13:33:31Z

+		mSymbolTable = new DefaultIcfgSymbolTable(symbolTable, procedures);
+
+		// TODO let callers pass predicate factory
+		mFactory = new BasicPredicateFactory(services, mManagedScript, mSymbolTable);


Is this TODO still relevant?

schuessf · 2026-06-01T08:29:40Z

+	 */
+	@Override
+	@Deprecated
+	Collection<S> getFinalStates();


This method is marked as deprecated, but it is used in LegalFocus::computeLegalFocus. Is this reason for deprecation ("We should not abuse the final states for empires, they do not represent any meaningful language. Instead introduce a suitably-named new method.") still valid, or should we remove it?

This aligns the implementation terminology with the POPL'26 paper. Furthermore, the class previously called EmpireAutomaton is named SaturatedEmpire to indicate the implemented algorithm.

matthiaszumkeller

Thank you for preparing this merge. I added some additional comments.

matthiaszumkeller · 2026-06-06T20:42:52Z

After inspection of this class and as far as I can recall, this class was a nearly 1-to-1 copy from the original (SaturatedEmpire) class at the time, which was only added so that we a able to quickly check test the approach. However if we would also parametrize the empire and State record with the type of region, with some engeneering effort most of this class (maybe even the whole class) should be obsolete (besides the method extendAll ).

matthiaszumkeller · 2026-06-06T20:47:38Z

+import de.uni_freiburg.informatik.ultimate.util.datastructures.DataStructureUtils;
+import de.uni_freiburg.informatik.ultimate.util.datastructures.ImmutableSet;
+
+public class DirectedEmpireProduct<L, P> {


Maybe this class should also just extend the IEmpire interface? The structure is practically the same and the empire product should again be an empire (I don't think we need to change this right now, but maybe a note for the future).

matthiaszumkeller · 2026-06-06T20:51:03Z

+		return interRegions;
+	}
+
+	private INestedWordAutomaton<Transition<L, P>, ProductState<L, P>> constructProductAutomaton() {


If this approach gets resumed in the future, I think this could also be done on-the-fly.

matthiaszumkeller · 2026-06-06T21:06:01Z

+			final var comparator = getPreference();
+			return (r1, r2) -> comparator.compare(new Pair<>(r1, law), new Pair<>(r2, law));
+		}
+


Should we add some documentation here? As far as I remember, the heuristics for choosing the right region for the focus also left a lot of space for future work.

maul-esel · 2026-06-08T22:42:14Z

In another project building of this work (branch wip/dk/civlized-og) we've just discovered a bug (or multiple bugs) that should be investigated and fixed before this is merged:

With the attached file fork-join-01.bpl, the generated O/G proof seems to be insufficient: the invariant between fork and join does not contain any information about x, and thus the proof should be insufficient to show the assert.
- In the project, we ask a deductive verifier to confirm the proof; and indeed it fails to do so because of this issue.
Yet, no asserts fail. Do we not internally validate the proof when -ea is passed? Or is our validator defect?
Also, inserting some blank lines in the program (fork-join-01.backup-bpl) seems to "fix" this problem: we get a stronger invariant, and the proof is confirmed by the external validator.

Apparently, there is a nondeterminism bug (likely, inserting blank lines changes the line numbers of some actions, which changes their hashcode, and affects some hashset iteration order).

fork-join01.backup.bpl.txt
fork-join01.bpl.txt
settings.epf.txt
toolchain.xml

schuessf · 2026-06-09T09:05:43Z

With the attached file fork-join-01.bpl, the generated O/G proof seems to be insufficient: the invariant between fork and join does not contain any information about x, and thus the proof should be insufficient to show the assert.

Is it really necessary to have an invariant stronger than true between fork and join? In this example, we already have x == 1 as an invariant at the exit location of the thread, so I would expect this to be sufficient to show the assert in the main thread.

In the project, we ask a deductive verifier to confirm the proof; and indeed it fails to do so because of this issue.

Maybe the validator (or your modelling) has a different understanding of the join that allows more behavior?

Also, inserting some blank lines in the program (fork-join-01.backup-bpl) seems to "fix" this problem: we get a stronger invariant, and the proof is confirmed by the external validator.
Apparently, there is a nondeterminism bug (likely, inserting blank lines changes the line numbers of some actions, which changes their hashcode, and affects some hashset iteration order).

Still, we should investigate this nondeterministic behavior!

matthiaszumkeller · 2026-06-10T12:55:43Z

With the attached file fork-join-01.bpl, the generated O/G proof seems to be insufficient: the invariant between fork and join does not contain any information about x, and thus the proof should be insufficient to show the assert.

In the project, we ask a deductive verifier to confirm the proof; and indeed it fails to do so because of this issue.

Yet, no asserts fail. Do we not internally validate the proof when -ea is passed? Or is our validator defect?

We do check validity of the OG-proof internally and at least to me, the resulting proof seems valid wrt. the Petri net representation of the program. It is true, that no information about variable x is contained in the invariant between fork and join . However, for the transition representing the join in the Petri net, there are three predecessor places: {6#L13true,4#threadEXITtrue,threadThread1of1ForFork0InUse}. Two of them do not contain any information about x in their invariants, they ensure that the ghostvariable is set to the value 3 associated with the exit of the thread. However the third place (threadThread1of1ForFork0InUse) does contain xin its invariant, more specifically if ghost==3, then it ensures that the formula x==1 holds. Therefore, the transition corresponding to the join seems to satisfy inductivity and the postcondition contains the invariant x==1. Am I missing something, or are there any other validity concerns that I did not consider?

Also, inserting some blank lines in the program (fork-join-01.backup-bpl) seems to "fix" this problem: we get a stronger invariant, and the proof is confirmed by the external validator.
Apparently, there is a nondeterminism bug (likely, inserting blank lines changes the line numbers of some actions, which changes their hashcode, and affects some hashset iteration order).

I also think, that the non-deterministic behavior should be investigated.

Update:
I again looked at the output of the Ultimate toolchain for the program and it seems to be the case that the invariant of threadThread1of1ForFork0InUse is missing in the result after backtranslation. Maybe this could cause the issue?

schuessf · 2026-06-10T13:08:29Z

However, for the transition representing the join in the Petri net, there are three predecessor places: {6#L13true,4#threadEXITtrue,threadThread1of1ForFork0InUse}. Two of them do not contain any information about x in their invariants, they ensure that the ghostvariable is set to the value 3 associated with the exit of the thread. However the third place (threadThread1of1ForFork0InUse) does contain xin its invariant, more specifically if ghost==3, then it ensures that the invariant x==1 holds. Therefore, the transition corresponding to the join seems to satisfy inductivity and the postcondition contains the invariant x==1. Am I missing something, or are there any other validity concerns?

Oh, I see. So I guess we do not output the invariant for threadThread1of1ForFork0InUse if we consider the Boogie-program, since this location is just an auxiliary-location that does not belong to any location in the Boogie program. As a result, the annotation is valid for the Petri program but not for the Boogie program. I suppose we somehow still need to consider invariants at auxiliary locations during backtranslation.

This also seems to be the reason for non-determinism, it depends whether we put the invariant at threadThread1of1ForFork0InUse or 6#L13true.

matthiaszumkeller · 2026-06-10T13:22:15Z

This also seems to be the reason for non-determinism, it depends whether we put the invariant at threadThread1of1ForFork0InUse or 6#L13true.

Nice catch! I think that I found the location where the non-determinism occurs. In the legal focus, method chooseBestRegion chooses the smallest region (currently our only heuristics) that contains predecessor places of the transition to be focused. In this example, it can choose between those two regions [threadThread1of1ForFork0InUse], [6#L13true] (the third one contains two elements) and the regions are given as a set. If it is very inconvenient to also include the auxilliary places in the resulting backtranslation, we could maybe also filter regions with auxilliary places (that are predecessors of the transition) / adjust the comparator to choose another region if possible.

schuessf · 2026-06-10T13:31:23Z

Nice catch! I think that I found the location where the non-determinism occurs. In the legal focus, method chooseBestRegion chooses the smallest region (currently our only heuristics) that contains predecessor places of the transition to be focused. In this example, it can choose between those two regions [threadThread1of1ForFork0InUse], [6#L13true] (the third one contains two elements) and the regions are given as a set. If it is very inconvenient to also include the auxilliary places in the resulting backtranslation, we could maybe also filter regions with auxilliary places (that are predecessors of the transition) / adjust the comparator to choose another region if possible.

I guess we could check for the auxiliary places in the legal focus as a heuristics (even if it is not quite nice). But I am still wondering whether the loss of precision when omitting the invariants for auxiliary places a) only occurs in combination with the legal focus and b) could be always fixed with such a simple heuristics (which I am quite skeptical).

maul-esel · 2026-06-10T13:44:26Z

Thanks to both of you for investigating this! This explains why we could get a seemingly invalid annotation after backtranslation but not trigger an error before.

Let's maybe continue the discussion how to fix this in another channel to not overload this PR.

maul-esel and others added 30 commits January 19, 2024 14:28

Unpetrifier: finish symbol table

25c271d

Add non-functional shorter places mapping

0828192

Merge branch 'wip/dk/empire-owicki' of https://github.com/ultimate-pa…

ad34e47

…/ultimate into wip/dk/empire-owicki

Add second approach of places mapping construction behind boolean

8a2ac86

Add CrownConstruction approach which constructs KingdomLaw holding a …

0a355c9

…single assertion condition

Some refactoring in colonization to lower the number of accesses to o…

9d110df

…ther classes fields

Small fix and optimization

9a20491

Add Set to store already seen Rooks

60b1c03

further work on unpetrification

7079a37

CrownConstruction: Immediately call crownConstruction on all settleme…

b4d408d

…nt rooks

Merge branch 'wip/dk/empire-owicki' of https://github.com/ultimate-pa…

dd25218

…/ultimate into wip/dk/empire-owicki

updated benchexec files

c499686

CrownConstruction: Store rejectedPairs as local variable instead of p…

ea8ec8b

…rivate field

Some refactoring and removal of unused code

6a096ac

OwickiGriesUnpetrifier: use proper symbol table for predicates

247936d

Also fix base name for ghost mirror variables.

hacky integration of (naive) Owicki-Gries proofs in TraceAbstraction

656fd13

add missing classes

6d09d38

unpetrify variables in Owicki-Gries invariants and ghost mirror updates

3fe4847

hacky support for auxiliary variables in proofs (ghost variables)

d9a5a8a

fix compilation error

575ffb0

add TODO

83b452d

add ghost variable workaround in CACSL2BoogieBacktranslator

d925da4

unpetrify ICFG locations

0b9f583

fix WitnessGhostDeclaration: allow multiple declarations

b505a50

Previously, each declaration overwrote the previous one.

two fixes for ghost mirror variables

cfc1b93

- initialize to zero (constant expression) - avoid weird symbols in the name

extended benchmark file

80210a2

Add some changes to also construct valid Empires for multiple proof a…

2b1251b

…utomata

Add some test cases for different concurrent patterns

3e14511

Only add successor to territory if they are part of the same thread

3b264fb

remove outdated comment

c7bd7d5

maul-esel commented May 29, 2026

View reviewed changes

remove an unused method

9c5b796

maul-esel commented May 29, 2026

View reviewed changes

Comment thread .../src/de/uni_freiburg/informatik/ultimate/lib/proofs/owickigries/empire/EmpireStatistics.java

maul-esel commented May 29, 2026

View reviewed changes

Comment thread ...Proofs/src/de/uni_freiburg/informatik/ultimate/lib/proofs/owickigries/empire/EmpireToOG.java Outdated

maul-esel commented May 29, 2026

View reviewed changes

schuessf reviewed Jun 1, 2026

View reviewed changes

maul-esel added 7 commits June 1, 2026 13:21

cleanup & document "~ghost~" variable name prefix handling

5aba76b

rename OwickiGriesConstruction (add "Naive") and move auxiliary method

9262eca

O/G: move result & ICFG annotation creation to auxiliary method

1d983aa

O/G: rename *EmpireToOG classes, add some documentation

a5da9da

remove unused and dangerous method

fed2a73

fix compile error introduced by 1d983aa

2705c1d

renaming: remove "Automaton" from empires

986eab7

This aligns the implementation terminology with the POPL'26 paper. Furthermore, the class previously called EmpireAutomaton is named SaturatedEmpire to indicate the implemented algorithm.

matthiaszumkeller reviewed Jun 6, 2026

View reviewed changes

Conversation

maul-esel commented May 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matthiaszumkeller left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maul-esel commented Jun 8, 2026

Uh oh!

schuessf commented Jun 9, 2026

Uh oh!

matthiaszumkeller commented Jun 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

schuessf commented Jun 10, 2026

Uh oh!

matthiaszumkeller commented Jun 10, 2026

Uh oh!

schuessf commented Jun 10, 2026

Uh oh!

maul-esel commented Jun 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

maul-esel commented May 29, 2026 •

edited

Loading

matthiaszumkeller commented Jun 10, 2026 •

edited

Loading