Skip to content

Comments

Removes Injectivity Assumptions on Inhale#409

Draft
ArquintL wants to merge 3 commits intomasterfrom
viper-release
Draft

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

Conversation

@ArquintL
Copy link
Member

@ArquintL ArquintL commented Feb 8, 2022

Fixes #427

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

jcp19 commented Feb 21, 2022

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

@ArquintL
Copy link
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
Copy link
Contributor

jcp19 commented Mar 29, 2022

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
Copy link
Contributor

jcp19 commented Oct 6, 2022

@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
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
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

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