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

Smt fix #55

Merged
merged 5 commits into from
Oct 23, 2023
Merged

Smt fix #55

merged 5 commits into from
Oct 23, 2023

Conversation

rozlynd
Copy link
Contributor

@rozlynd rozlynd commented Nov 23, 2021

Contains two soundness fixes for the SMT backend

@@ -187,10 +186,6 @@ let prepreproc sq =
let hs,_ = List.split res in
Smt.record_signatures := hs :: !Smt.record_signatures;
Record (List.map (fun (s, e) -> (s, self#expr scx e)) res) @@ e
| Apply ({core = Internal B.Eq}, [x ; {core = Internal (B.TRUE | B.FALSE)} as y])
Copy link
Contributor

@muenchnerkindl muenchnerkindl Nov 23, 2021

Choose a reason for hiding this comment

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

I believe you disabled this rewrite because = and <=> have different strength and so there could be an unsoundness if the rewrite was applied to formulas with unsuitable polarity? Would you mind leaving a comment in the code just as you explained the removal of the rewrite about CHOOSE?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That change was not me, but your explanation is correct. To be even more precise, the core issue is the fact that "x <=> FALSE" is weaker than "x = FALSE". For "TRUE", the two statements are actually equivalent, so I re-included that part of the rewriting rule.

Copy link
Contributor

@damiendoligez damiendoligez left a comment

Choose a reason for hiding this comment

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

LGTM

hvanz and others added 5 commits June 21, 2022 13:51
Would be better to keep this rule enabled for negative contexts
(for ex. on equality hypotheses), but that would imply making changes
everywhere in the SMT code.
This reverts commit a2e46fd.

One case of the rule is sound, and I added a comment
The error was in using the term unspec(f, x) in place of
unspec([ f EXCEPT ![y] = ex ], x).  That rule would allow one to prove
obligations without checking the condition x \in DOMAIN f.
This bug was corrected already in two places in the code, one in
smt/rewrite, the other in smt/typ_t.  There was a third place to
correct.
@damiendoligez
Copy link
Contributor

Rebased onto main.

@damiendoligez damiendoligez merged commit 98556bb into main Oct 23, 2023
@damiendoligez damiendoligez deleted the smt-fix branch October 23, 2023 14:38
@kape1395 kape1395 mentioned this pull request Oct 24, 2023
ahelwer added a commit to ahelwer/tlapm that referenced this pull request Oct 29, 2023
This reverts commit 98556bb, reversing
changes made to dc344b6.

Signed-off-by: Andrew Helwer <[email protected]>
@ahelwer
Copy link
Collaborator

ahelwer commented Nov 13, 2023

@damiendoligez since this is causing a failing test can it be reverted so that the main branch has all tests pass in the CI? It can be merged again when it doesn't cause a test to fail.

@kape1395
Copy link
Collaborator

It would be better to merge #98 instead of reverting this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

6 participants