-
Notifications
You must be signed in to change notification settings - Fork 112
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
Conversation
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.
There was a problem hiding this 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: |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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))); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep.
PR #776 had a fundamental bug which this fixes.