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

Heapster fix lowned proofs #1415

Merged
merged 34 commits into from
Aug 11, 2021
Merged

Heapster fix lowned proofs #1415

merged 34 commits into from
Aug 11, 2021

Conversation

eddywestbrook
Copy link
Contributor

This PR fixes how lowned permissions are proved, especially with regards to those lowned permissions like

l:lowned (x:p_in -o x:p_out)

which contain variables like x that have equality permissions on them. Previously, the implication prover would cast the lowned permissions using these equality permissions, in order to remove any variables with equality permissions; e.g., if x:eq(y), the above permission would be cast to

l:lowned (y:p_in -o y:p_out)

The issue was when x was equal to a non-variable expression, which yields an lowned containing an implication on non-variable expressions. Instead, the new approach implemented in this PR finds all relevant equality permissions and incorporates them into the local implications used to prove one lowned permission from another.

This uncovered a number of lifetime-related bugs that were fixed, including:

  1. localProveVars was changed so that, when it builds a local proof of ps1 -o ps2, it recombines the input permissions ps1 in reverse order, since these permissions are created (by the implication prover for lowned permissions) with the equality permissions first, and these need to be recombined first.

  2. instantiateLifetimeVars was changed to only instantiate lifetimes that require lowned permissions

  3. implEndLifetimeRecM was tweaked so that it now removes any lifetimes that have been ended from any other lowned permissions

  4. simplify1PermForDetVars was fixed so that if a determined variable has an lowned permission that contains an indetermined lifetime variable, that indetermined lifetime is ended and removed from the lowned so that it no longer has that indetermined lifetime free

Eddy Westbrook added 30 commits July 28, 2021 12:27
…pending on its type; also removed old PPInfo-related code
…ion of contained lifetimes, along with an lfinished permission
…on before eliminating it and would then go right back to trying to eliminate that same permission again...
…lock permission; changed proveVarLLVMBlocks to focus first on eliminating memblock permissions on the left that overlap with but are not contained in memblock permissions on the right
…s a tagged union type where we already know from something on the left what the tag is, in which case we can avoid searching for proofs of all the disjuncts other than the one that matches that known tag
…ence shape sh;emptysh with the empty shape when necessary
…y two separate functions, all of which now take lists of multi-bindings instead of multi-bindings of lists
…fied lowned permission, along with all the new and modified rules; also removed a bunch of whitespace from a previous commit
…casting both sides to incorporate the equality permissions, change solveForPermListImpl to incorporate those equality permissions
…der, since the left-most permissions (including any relevant equality permissions) are actually expected to be recombined first
…ions for the lifetimes it instantiates; also added debug tracing for when lifetimes begin and end
…on on top of the stack, but to instead remove the finished lifetime from any other lowned permissions; also setPerm and setPerms, which have not been used in a long time
…imes contained in lowned permissions for determined lifetimes
@eddywestbrook eddywestbrook added the subsystem: heapster Issues specifically related to memory verification using Heapster label Aug 10, 2021
@eddywestbrook eddywestbrook requested review from glguy and m-yac August 10, 2021 21:08
Copy link
Contributor

@m-yac m-yac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel very out of my depth trying to review changes to the implication prover, so I can't say I really understand solveForPermListImpl or most of the changes to proveVarAtomicImpl, for example. But from just a pure Haskell perspective, it looks OK to me.

solveForPermListImpl ps_inR' mb_ps_inL' >>>= \(Some neededs1) ->
solveForPermListImpl ps_outL' mb_ps_outR' >>>= \(Some neededs2) ->
partialSubstForceM mb_ps_inR "proveVarAtomicImpl" >>>= \ps_inR ->
let mb_ps_inL = fmap (const ps_inL) mb_ps_inR in
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let mb_ps_inL = fmap (const ps_inL) mb_ps_inR in
let mb_ps_inL = ps_inL <$ mb_ps_inR in

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using some of these special-purpose operators sometimes just looks like line noise to me. I know <$> and <*>, but when it starts getting more complex I would rather just be verbose about what the operator is doing. It's just like using the lens library: if you don't know all those operators, it can be really hard to read code that uses them.

@eddywestbrook eddywestbrook added the PR: ready to merge Magic flag for pull requests to ask Mergify to merge given an approval and a successful CI run label Aug 11, 2021
@eddywestbrook eddywestbrook merged commit db63fd6 into master Aug 11, 2021
@mergify mergify bot deleted the heapster-fix-lowned-proofs branch August 11, 2021 23:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: ready to merge Magic flag for pull requests to ask Mergify to merge given an approval and a successful CI run subsystem: heapster Issues specifically related to memory verification using Heapster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants