Skip to content

remove lhs_object from goto_trace_stept #2204

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

Merged
merged 3 commits into from
Sep 5, 2018
Merged

Conversation

kroening
Copy link
Member

Not done yet; in particular, it's unclear to me how these GraphML witnesses look like.

@martin-cs
Copy link
Collaborator

FWIW I remember @tautschnig expressing ... disappointment ... with the GraphML witnesses.

@kroening kroening requested a review from pkesseli as a code owner August 23, 2018 08:05
@kroening kroening force-pushed the remove-lhs-object branch 2 times, most recently from 636bbe1 to ea0ea36 Compare August 23, 2018 09:52
@kroening
Copy link
Member Author

The way the assignment is generated in error traces is now much simpler -- but it's non-obvious whether that simpler way is right or wrong.

@kroening
Copy link
Member Author

Depends on #2824, which should get tweaked/reviewed/merged first.

@tautschnig
Copy link
Collaborator

Reading the code changes I'm finding it near impossible to assess whether what's happening is right or wrong (except that the graphml bit is now missing one sanity check). Can we have tests that actually inspect contents of the trace output, please?

@kroening
Copy link
Member Author

kroening commented Sep 4, 2018

Now added a test that checks the cases I could think of for console traces.
The output for graphml on that test is identical, and so is the output for the JSON UI.

The XML output changes as the 'value' elements are removed. I'll make that a separate commit.

Daniel Kroening added 2 commits September 4, 2018 17:07
Rationale:
 * lhs_object_value may be extremely large (think of large struct or array), but the information is contained in (more compact form) full_lhs_value.
 * lhs_object is contained in full_lhs. A helper function is provided to extract it.
Copy link
Collaborator

@tautschnig tautschnig left a comment

Choose a reason for hiding this comment

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

I'll just trust that this test serves as specification of what we expect to find in a trace, and will therefore detect if something breaks. It may need to be extended over time, but it's a good start to have one.

@tautschnig tautschnig assigned kroening and unassigned tautschnig Sep 4, 2018
Copy link
Contributor

@allredj allredj left a comment

Choose a reason for hiding this comment

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

This PR failed Diffblue compatibility checks (cbmc commit: 844a34c).
Status will be re-evaluated on next push.
Please contact @peterschrammel, @thk123, or @allredj for support.

Common spurious failures:

  • the cbmc commit has disappeared in the mean time (e.g. in a force-push)
  • the author is not in the list of contributors (e.g. first-time contributors).

Copy link
Contributor

@allredj allredj left a comment

Choose a reason for hiding this comment

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

This PR failed Diffblue compatibility checks (cbmc commit: df868b8).
Status will be re-evaluated on next push.
Please contact @peterschrammel, @thk123, or @allredj for support.

Common spurious failures:

  • the cbmc commit has disappeared in the mean time (e.g. in a force-push)
  • the author is not in the list of contributors (e.g. first-time contributors).

@kroening kroening merged commit 78d811f into develop Sep 5, 2018
@kroening kroening deleted the remove-lhs-object branch September 5, 2018 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants