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

Fix crash on low rlimit #859

Merged
merged 4 commits into from
Mar 13, 2024
Merged

Fix crash on low rlimit #859

merged 4 commits into from
Mar 13, 2024

Conversation

atomb
Copy link
Collaborator

@atomb atomb commented Mar 12, 2024

Z3 has multiple ways of responding that the resource limit has been exceeded. In some cases it responds with an error message including "push canceled". This updates Boogie to handle more of the strange side cases.

@atomb atomb enabled auto-merge (squash) March 12, 2024 20:18
Comment on lines +168 to +171
// Sometimes Z3 responds with "(error ...)" followed by "unknown"
if (outcomeSExp.Name is "error" && responseStack.TryPeek(out var sExpr) && sExpr.Name is "unknown") {
responseStack.Pop();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be better to do this before ParseOutcome(...) instead? Or perhaps we should also set wasUnknown to true ourselves so we hit ParseReasonUnknown below?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I considered that, but ultimately the error message is what really tells us what the outcome is, somewhat confusingly.


if (options.LibOptions.ProduceUnsatCores) {
if (options.LibOptions.ProduceUnsatCores && responseStack.Count > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we communicate some kind of error explicitly if ProduceUnsatCores is on but responseStack is empty?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, we should. I'll add that.

@@ -1209,6 +1209,7 @@ protected SolverOutcome ParseOutcome(SExpr resp, out bool wasUnknown)
case "error":
if (resp.Arguments.Length == 1 && resp.Arguments[0].IsId &&
(resp.Arguments[0].Name.Contains("max. resource limit exceeded")
|| resp.Arguments[0].Name.Contains("push canceled")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this because the only reason a push is ever cancelled is when Boogie itself determines resource limits are reached?

Copy link
Collaborator Author

@atomb atomb Mar 12, 2024

Choose a reason for hiding this comment

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

Yes, exactly. I've confirmed this in the Z3 source code.

@atomb atomb merged commit 5c786ff into boogie-org:master Mar 13, 2024
4 checks passed
atomb added a commit to dafny-lang/dafny that referenced this pull request Mar 13, 2024
### Description

Bump Boogie dependency to v3.1.3

This most importantly includes
boogie-org/boogie#859

### How has this been tested?

Existing tests should be sufficient.

By submitting this pull request, I confirm that my contribution is made
under the terms of the MIT license.
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.

2 participants