Skip to content

Removes Injectivity Assumptions on Inhale - #409

Draft
ArquintL wants to merge 3 commits into
masterfrom
viper-release
Draft

Removes Injectivity Assumptions on Inhale#409
ArquintL wants to merge 3 commits into
masterfrom
viper-release

Conversation

@ArquintL

@ArquintL ArquintL commented Feb 8, 2022

Copy link
Copy Markdown
Member

Fixes #427

@ArquintL
ArquintL marked this pull request as draft February 8, 2022 15:33
@jcp19

jcp19 commented Feb 21, 2022

Copy link
Copy Markdown
Contributor

Before merging this, I would also like to run it with verifiedSCION

@ArquintL

Copy link
Copy Markdown
Member Author

Before merging this, I would also like to run it with verifiedSCION

Makes sense. I've just opened this PR to detect possible issues before the Viper release. Might make sense to run your experiments sooner than later in case we need to defer the Viper release

@jcp19

jcp19 commented Mar 29, 2022

Copy link
Copy Markdown
Contributor

I believe it makes sense to merge these changes before I start looking at enabling the consistency checks. I am now running tests locally on the verified scion repository.

@ArquintL should we disable the dense_sparse_matrix tests? Or did you find a way to make them pass the injectivity tests?

@ArquintL ArquintL changed the title Update Dependencies to Latest Viper Release Removes Injectivity Assumptions on Inhale Apr 1, 2022
@jcp19

jcp19 commented Oct 6, 2022

Copy link
Copy Markdown
Contributor

@ArquintL should we close this PR? Right now, it seems unlikely that it will be merged. Regarding the changes to the encoding, if you think it is worth keeping, maybe we can open a PR for that already

@ArquintL

Copy link
Copy Markdown
Member Author

The only changes to me seems to be that I use len(arr) instead of the statically known array length. This is only a problem in the context of null arrays because their length is 1 to we cannot simply use the statically known array length (which can be different from 1). This issue did not show up before because we assumed injectivity but as soon as you no longer assume it Viper will complain about the quantified permissions no longer being injective because injectivity of array locations is defined only within bounds.
I thus think it does not hurt merging the changes to the encoding in even though they are not strictly necessary as long as we assume injectivity. However, there are 2 points we should check:

  1. what is the impact on performance due to introducing a len expression instead of using an int literal (while still assuming injectivity)
  2. do we observe incompletenesses (also while still assuming injectivity)

@ArquintL
ArquintL marked this pull request as ready for review October 17, 2022 08:22
@ArquintL
ArquintL requested a review from jcp19 October 17, 2022 08:22
@ArquintL
ArquintL marked this pull request as draft October 17, 2022 08:22
@ArquintL
ArquintL removed the request for review from jcp19 October 17, 2022 08:23
@ArquintL

Copy link
Copy Markdown
Member Author

Should only be merged after merging PR #531 and fixing the resulting merge conflicts. Since #531 changes the representation of null values, these changes might actually no longer be needed

jcp19 pushed a commit that referenced this pull request Aug 27, 2026
Quantifying the footprint over len(loc) alone is not enough. Permission is
then inhaled for `idx < len(loc)`, and to use it at the statically known
indices the prover has to re-derive `len(loc) == n` from the embedding
invariant `len(loc) == n || loc == nil`. It cannot, because nothing rules
out the nil case, so `acc(&a)` for `var a@ [12]int` stopped reporting a
failed injectivity check and started reporting "Permission to a might not
suffice" instead.

`Ref[loc] != nil` is implied by the quantifier -- all locations of the nil
array are null and Viper does not permit holding permission to null -- but
that derivation is never triggered where it is needed, so state it
explicitly, as PR #409 does.

Measured over the regression suite with the injectivity check enabled
(DefaultAssumeInjectivityOnInhale = false), out of 1195 tests:

    no encoding change                62 failures
    len(loc) bound only               44 failures
    len(loc) bound + non-nil          10 failures

with no test failing that did not already fail without the encoding change.
Two of the remaining ten need Boogie, which is absent from the machine these
numbers were taken on. The other eight are quantified permissions over
stored pointers -- (m.values)[i][j], b.valuesMap[i], m[s], s.nodes[i] --
where injectivity means pairwise distinctness of the pointers and has to be
specified by hand; they are not encoding gaps.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ESqeR8oFbRH3HB7D92eiEb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Disable assumeInjectivityOnInhale

2 participants