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

Make lifetimesThatCouldProve more precise #1667

Merged
merged 10 commits into from
May 18, 2022
Merged

Conversation

eddywestbrook
Copy link
Contributor

The main focus of this PR is to make the lifetimesThatCouldProve function more precise. The goal of lifetimesThatCouldProve is to determine all the lifetimes that might need to be ended in order to prove a given goal in the implication prover. The implication prover then tries ending each of these in turn to see which ones need to be ended in order to succeed.

The old approach to lifetimesThatCouldProve was to simply test if ending a lifetime l would give back permissions on any variable x that we are trying to prove permissions for. The old version of this function turned out to have three problems:

  1. It would return a lifetime l in cases where the specific permissions for x given back by ending l wasn't actually needed, giving too many false positives; and
  2. It would not consider other variables y for which we currently have permissions like x:ptr((W,0) |-> eq(y)) whose permissions we do need to prove permissions on x.
  3. It didn't take into account that the implication prover should try to end "smaller" lifetimes first, before trying "bigger" lifetimes that contain those smaller ones.
    This PR addresses these problems by redefining lifetimesThatCouldProve to take these issues into account.

As part of this changes, this PR also makes the following changes:

  • extImplState, withExtVarsM, and related functions no longer need a KnownRepr TypeRepr a for the type of the new evars added to the state, but now instead only need a `TypeRepr a;
  • The SomeMbPerm and MbRangeForLLVMType types now only need a CruCtx instead of a KnownCruCtx;
  • A problematic case for recombinePerm was commented out; and
  • A non-exhaustive pattern-matching error in SAWTranslation.hs was fixed.

Eddy Westbrook added 10 commits May 5, 2022 06:59
… overlaps with another permission that is already held; fixed a bug in the logic for proving lcurrent permissions to avoid a lifetime containment cycle
…the type being used for a function when it is type-checked at debug level
…uld actually return permissions that are useful in proving the RHS are considered as ones that could be ended in the current implication; also added a topological sort to lifetimesThatCouldProve, so child lifetimes are returned before their parents
…eturned by a lifetime is in a contained eq permission of a permission we currently hold on a variable on the right
@eddywestbrook eddywestbrook added the subsystem: heapster Issues specifically related to memory verification using Heapster label May 18, 2022
@eddywestbrook eddywestbrook requested a review from m-yac May 18, 2022 01:15
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.

LGTM. I like the comments you added to lifetimesThatCouldProve and proveVarAtomicImpl!

@m-yac m-yac 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 May 18, 2022
@mergify mergify bot merged commit 305d728 into master May 18, 2022
@mergify mergify bot deleted the heapster/debug-recombine-perm branch May 18, 2022 20:05
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.

2 participants