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

Actually parse structured program elements #777

Merged
merged 11 commits into from
Sep 5, 2023

Conversation

atomb
Copy link
Collaborator

@atomb atomb commented Sep 5, 2023

PR #776 had a fundamental bug which this fixes.

This prints an informative error when the solver segfaults, but it
doesn't continue on to process the remaining goals as it ideally would.
Instead of concatenating and parsing strings everywhere, this introduces
a structured data type for representing the transformations on statement
labels that can happen during Boogie elaboration and VC generation. It
relieves clients from needing to parse the full unsat core element
labels.
robin-aws
robin-aws previously approved these changes Sep 5, 2023
Copy link
Contributor

@robin-aws robin-aws left a comment

Choose a reason for hiding this comment

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

Fix itself LGTM, just a couple of things to think about.

// CHECK: Covered elements: cont_ens_abs$call_cont$ensures, xpos_abs$call_cont$requires, xpos_caller
// CHECK: Elements covered by verification: a_gt_10, a0, assert_a0, assert_r0, assumeFalse, call2_tee1, constrained, cont_assume_1, cont_assume_2, cont_ens_abs$call_cont$ensures, cont_req_1, cont_req_2, false_req, r0, sinv_not_1$established, sinv_not_1$maintained, sinv1$assume_in_body, sinv1$established, sinv1$maintained, sinv2$assume_in_body, sinv2$established, sinv2$maintained, spost, spre1, tee_not_1, tee0, tee0$call1$ensures, tee0$call2$ensures, tee1, tee1$call2$ensures, ter0, ter0$call1$requires, ter0$call2$requires, ter1, x_gt_10, xpos_abs$call_cont$requires, xpos_caller, xy_sum
// RUN: %OutputCheck "%s" --file-to-check="%t.coverage"
// CHECK: Proof dependencies:
Copy link
Contributor

Choose a reason for hiding this comment

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

Side note: if you're checking for the literal output line-by-line without anything expected in between, it's more strict and probably more efficient to use CHECK-NEXT: - https://github.com/stp/OutputCheck#check-next-regex

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, yeah, it would probably make sense for some (but not all) of these to be CHECK-NEXT. However, I don't imagine the lack of that would cause us to miss bugs in this case.

@@ -21,8 +97,8 @@
// RUN: %diff "%s.expect" "%t.coverage-p"
// RUN: %boogie -trackVerificationCoverage -normalizeDeclarationOrder:1 -prune "%s" > "%t.coverage-d"
// RUN: %diff "%s.expect" "%t.coverage-d"
// RUN %boogie -trackVerificationCoverage -normalizeNames:1 -prune "%s" > "%t.coverage-n"
// RUN %diff "%s.expect" "%t.coverage-n"
// RUN: %boogie -trackVerificationCoverage -normalizeNames:1 -prune "%s" > "%t.coverage-n"
Copy link
Contributor

Choose a reason for hiding this comment

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

Oof, I wish I had a concrete suggestion for how to catch this type of testing error. :P

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Me too!

@@ -224,7 +224,7 @@ public override void FullReset(VCExpressionGenerator generator)
{
usedNamedAssumes.Add(resp.Name);
if (libOptions.TrackVerificationCoverage) {
reporter.AddCoveredElement(new LabeledNodeComponent(resp.Name.Substring("aux$$assume$$".Length)));
reporter.AddCoveredElement(TrackedNodeComponent.ParseSolverString(resp.Name.Substring("aux$$assume$$".Length)));
Copy link
Contributor

Choose a reason for hiding this comment

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

I take it these two lines were the fundamental bug?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep.

@atomb atomb merged commit 85191e1 into boogie-org:master Sep 5, 2023
4 checks passed
@atomb atomb deleted the structured-program-elements branch January 4, 2024 16:35
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