Conversation
|
@ArquintL you may now take a look at this PR when you have a chance. |
|
Some thoughts about the description (not having looked at the code yet). I think fixing the description would be good for documentation purposes and feel free to also add those clarifications to the codebase where you think it would make sense
As you say, this depends on ghost methods not being able to open invariants and additionally atomic methods not having an implementation. Is there an appropriate remark in the type-checker that, should we ever relax the latter, this might introduce an unsoundness if we are not careful?
Is this solution part of this PR or will you create a separate PR to fix this limitation? |
ArquintL
left a comment
There was a problem hiding this comment.
I've made a pass over the implementation without looking at the testcases yet
| specification returns[boolean trusted = false, boolean pure = false, boolean mayInit = false, boolean opensInv = false, boolean atomic = false, boolean opaque = false;]: | ||
| // Non-greedily match PURE to avoid missing eos errors. | ||
| ((specStatement | OPAQUE {$opaque = true;} | PURE {$pure = true;} | MAYINIT {$mayInit = true;} | TRUSTED {$trusted = true;}) eos)*? (PURE {$pure = true;})? backendAnnotation? | ||
| ((specStatement | OPAQUE {$opaque = true;} | PURE {$pure = true;} | OPENSINV {$opensInv = true;} | MAYINIT {$mayInit = true;} | ATOMIC {$atomic = true;} | TRUSTED {$trusted = true;}) eos)*? (PURE {$pure = true;})? (ATOMIC {$atomic = true;})? backendAnnotation? |
There was a problem hiding this comment.
Is there a reason against using the same order as on L. 183?
There was a problem hiding this comment.
well, the two orders never matched anyway, but I don't mind changing that
There was a problem hiding this comment.
That's what I noticed too. Double checking whether we assigned all of them just became increasingly difficult ^^
...ain/scala/viper/gobra/frontend/info/implementation/typing/ghost/separation/GhostTyping.scala
Show resolved
Hide resolved
| )(exprSrc) | ||
| _ <- write(inhaleInv) | ||
|
|
||
| // stmts |
There was a problem hiding this comment.
I hope the checks disallow gotos in here
There was a problem hiding this comment.
We do, the only actual statements we allow are calls to atomic functions whose parameters have been evaluated and assignments to local exclusive variables whose rhs are calls to atomic functions
There was a problem hiding this comment.
Could you please document all these side-conditions at the place where we type-check critical?
| )(exprSrc) | ||
| _ <- write(markClosed) | ||
|
|
||
| } yield exhaleInv |
There was a problem hiding this comment.
Wouldn't it be cleaner do define an encoding for the critical stmt instead of making the desugarer even larger?
There was a problem hiding this comment.
What do you mean exactly? Introducing a critical statement in the intermediate representation?
|
@ArquintL thank you for taking the time to review the PR! I will revise the PR description to clarify your questions. For now, I will address the comments that I can address quickly, and I will come back to the bigger changes later. |
Co-authored-by: Linard Arquint <ArquintL@users.noreply.github.com>
|
Answering your first comment:
Yes,
The fix applies to all identifiers that resolve to the
Hmm, I guess I can add sth to the type-checker of atomic methods, but I feel like these comments often go unnoticed or unmaintained when assumptions change. I think a much better way is to have tests that catch violations of expectations and design documents/PR descriptions for these features that we can later revisit.
Yes, it is already implemented in this PR. |
The goal of this PR is to bring down the TCB of our resource algebra formalization to what I believe is the smallest possible TCB that we can have given the absence of existentially quantified permissions in Gobra. Our assumptions are all listed in file `docs.gobra`, and only have to do with the introduction and elimination of existential quantifiers. There are a couple of todos here: - [ ] (maybe) clean up the code and use the implementations for MonoSet and MonoMap instead of the bespoke cooliomapio and cooliosetio :) - [x] Make `GlobalMem()` an invariant established during initialization and replace the `inhale`/`exhale` pairs with code to open the invariant. Done in #35, but it depends on viperproject/gobra#983. - [ ] Prove the last outstanding assumption: the product of all currently allocated elements for a reference is valid (might do this in a separate PR) (#36) PS: I introduced a new packet rather than changing package `resalgebra`, as the API for RAs changed slightly. I might deprecate `resalgebra` soon though
This PR improves Gobra's situation in dealing with atomicity, so that we no longer need informal arguments to justify that opening invariants around certain parts of the code is safe. In particular, it brings the following changes:
atomicmodifier for abstract methods and functions (non-abstract atomic members are disallowed, as we cannot prove atomicity - at least for now). Atomic methods should be those whose effects occur, logically, at a single linearization point. Interface methods may be marked asatomictoo, in which case they may only be implemented byatomicmethods.Pof typepred()is an invariant, written asInvariant(p)if it has been shown to hold using theEstablishInvariantbuiltin ghost function. Once established, invariants must be preserved by all atomic operations, and thus, by all operations.P(), wherePis a built-in FPredicate likePredTrue. We were forced to writePredTrue!<!>()at all times before.This statement opens invariant
P!<!>(), which is assumed at the start of the critical region, and must be shown at its end. Critical regions check that there is no re-entrance, i.e., no invariant is opened twice. Statements inSmay contain, at most, a single call to an atomic method (from an interface or otherwise, more on this later) and arbitrary ghost code (which must be shown to terminate). Like theoultinestatement, thecriticalstatement does not introduce a block (i.e., it does not introduce its own namespace).There are two critical decisions I took to simplify the logics here:
PopensPagain. One way of doing this would be to have some way of tracking the currently open invariants, and specify in the method specifications which invariants are required to not be open. This requires a more complex encoding. Instead, I opted for the following split of concerns, which I think is not very limiting:ghostmethods cannot open invariants. The invariants must be opened in the actual code before calling ghost methods that depend on them. Thus, ghost methods may all be called safely from critical regions. This makes checking for reentrance very easy.Scontains a calli.M(), whereiis of interface type, I believe that reasoning about this program is similar to reasoning about the following:Step (1) is "transparent", i.e., its effects cannot be observed, if the value stored in
icannot change between (1) and (2), which guarantees that the method that is called still matches the dynamic type of the value stored ini. I believe that is always the case:iis either in an exclusive or shared memory location.i.M, there must be at least read permissions toiin the current thread. The permissions may either come from from the surrounding environment or fromP!<!>().a. If they come the former, then no other thread may ever obtain full permission to
iand modify it while the call is being performed.b. If they come from the latter, another thread could in principle try to open
P!<!>()in parallel and modifyiin an atomic step. However, there is no way to do so as far as I can tell. Regular assignments toiare not atomic (and thus, disallowed in critical regions) and the packageatomicdoes not offer a way (as far as I can see) to atomically mutate a variable of interface type.EDIT: disallowing opening invariants in ghost methods is too restrictive after all. An example where this is very limiting is in the implementation of Iris's ghost locations in gobra-libs, where the only way to implement a model for this requires an invariant. At first sight, a solution to this may be to require an annotation on methods that may open invariants and disallow calling those methods from methods without that annotation or inside critical regions. I have implemented this solution, and I have shown that it is not super restrictive by trying it in two different proofs: