Skip to content

Trace loop heads #5381

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 6 commits into from
Jun 25, 2020
Merged

Trace loop heads #5381

merged 6 commits into from
Jun 25, 2020

Conversation

thk123
Copy link
Contributor

@thk123 thk123 commented Jun 12, 2020

Integrating @peterschrammel work for adding loop-heads to the trace. Loop-heads replace location-only trace steps for the specific instruction that is the start of the loop (i.e. has a back edge pointing to it). These are never removed from the trace, even when two loop-heads are adjacent to each other in the trace, allowing seeing how many times the trace goes through the loop. See the documentation changes for more details.

  • Each commit message has a non-empty body, explaining why the change was made.
  • Methods or procedures I have added are documented, following the guidelines provided in CODING_STANDARD.md.
  • The feature or user visible behaviour I have added or modified has been documented in the User Guide in doc/cprover-manual/
  • Regression or unit tests are included, or existing tests cover the modified code (in this case I have detailed which ones those are in the commit message).
  • [na] My commit message includes data points confirming performance improvements (if claimed).
  • My PR is restricted to a single feature or bugfix.
  • White-space or formatting changes outside the feature-related changed lines are in commits of their own.

@thk123 thk123 force-pushed the trace-loop-heads branch from 867bd69 to 3a3dbf4 Compare June 18, 2020 09:43
@thk123
Copy link
Contributor Author

thk123 commented Jun 18, 2020

Rebased to check the XSD validation step fails on CI (fix clang format errors)

@thk123 thk123 force-pushed the trace-loop-heads branch from 3a3dbf4 to 2244c58 Compare June 18, 2020 10:54
@thk123
Copy link
Contributor Author

thk123 commented Jun 18, 2020

Validation step should now pass - source code docs will still fail

@thk123
Copy link
Contributor Author

thk123 commented Jun 18, 2020

A refactor I could put on top of this: thk123#4 (just pulls more "business logic" out of the json/xml specific files

@thk123 thk123 force-pushed the trace-loop-heads branch 3 times, most recently from 18cbd54 to b05045c Compare June 18, 2020 15:47
@thk123 thk123 marked this pull request as ready for review June 18, 2020 15:57
@codecov
Copy link

codecov bot commented Jun 18, 2020

Codecov Report

Merging #5381 into develop will increase coverage by 0.00%.
The diff coverage is 94.11%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #5381   +/-   ##
========================================
  Coverage    68.19%   68.19%           
========================================
  Files         1175     1176    +1     
  Lines        97504    97517   +13     
========================================
+ Hits         66490    66502   +12     
- Misses       31014    31015    +1     
Flag Coverage Δ
#cproversmt2 42.73% <94.11%> (+0.23%) ⬆️
#regression 65.37% <94.11%> (+<0.01%) ⬆️
#unit 32.24% <0.00%> (-0.01%) ⬇️
Impacted Files Coverage Δ
src/goto-programs/structured_trace_util.cpp 87.50% <87.50%> (ø)
src/goto-programs/json_goto_trace.cpp 81.57% <100.00%> (ø)
src/goto-programs/json_goto_trace.h 90.90% <100.00%> (+0.90%) ⬆️
src/goto-programs/xml_goto_trace.cpp 98.29% <100.00%> (+0.02%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 818afcc...657c1a6. Read the comment docs.

Copy link
Contributor

@danpoe danpoe left a comment

Choose a reason for hiding this comment

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

LGTM, just nitpicks

@@ -0,0 +1,32 @@
/*******************************************************************\

Module: Goto Programs
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Not goto programs module 🌴

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What module would you prefer? It is in folder goto-programs?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think normally this just names the feature in the file, here it would be Structured Trace Utilities or something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seems... pointless 😛 I've checked the coding standards and it says it should just mention author, so I'll delete this line

@@ -0,0 +1,37 @@
/*******************************************************************\

Module: Goto Programs
Copy link
Contributor

Choose a reason for hiding this comment

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

🌴


/// There are two kinds of step for location markers - location-only and
/// loop-head (for locations associated with the first step of a loop).
enum class default_step_kindt
Copy link
Contributor

Choose a reason for hiding this comment

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

Why "default"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nomenclature taken from the docs which call these default steps 🤷

@@ -18,10 +18,12 @@ Author: Daniel Kroening
#include <util/symbol.h>
#include <util/xml_irep.h>

#include <algorithm>
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 needed?

Ensure even with sliced formulas, we get a location only step for
each iteration of the loop (called loop-heads) when using partial loops.

This test is checking the following json:(deleting the new lines after each \n to obtain
Copy link
Member

Choose a reason for hiding this comment

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

:)


#include "structured_trace_util.h"
#include <algorithm>
default_step_kindt
Copy link
Member

Choose a reason for hiding this comment

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

⛏️ some new lines would be appreciated.

@thk123 thk123 force-pushed the trace-loop-heads branch from b05045c to f3e231c Compare June 19, 2020 17:48
@thk123
Copy link
Contributor Author

thk123 commented Jun 25, 2020

One travis job is timing out running the tests, hopefully unlucky - will try again.

peterschrammel and others added 6 commits June 25, 2020 10:20
In the trace, we only output the first location of a sequence
of same locations.
However, we don't want to suppress loop head locations because
they might come from different loop iterations. If we suppressed
them it would be impossible to know in which loop iteration
we are in.
Even though the slicing removes all the assignments, the loop heads are
maintained in the test
Extend the documentation to cover the new version of location-only
called loop-head.
Previously the JSON and XML trace printing was duplicating the logic for
determing whether a trace step is a loop head
Consolidate the documentation - deleting the duplicated doxygen that was
attached to the implementation as well as the declaration
@thk123 thk123 force-pushed the trace-loop-heads branch from f3e231c to 657c1a6 Compare June 25, 2020 09:22
@thk123 thk123 merged commit faf7f43 into diffblue:develop Jun 25, 2020
@thk123 thk123 mentioned this pull request Jul 3, 2020
5 tasks
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.

3 participants