Rewriting call_grapht to inherit from grapht<T>, and introduce reachable_call_graph class #1509

polgreen · 2017-10-23T16:19:40Z

I have rewritten the call_grapht class to inherit from grapht. There are many graph functions implemented for grapht that it would be useful to have for call_grapht. It also makes traversing the call graph backwards easier.

call_grapht now inherits from grapht
I have moved the code from slice_global_inits that produces a list of reachable functions to a class function
I have introduced a reachable_call_grapht which inherits from the new call_grapht (I am using this for work that I will create a pull request for in the next couple of weeks). I've implemented this as a separate class rather than using a function to trim the call_graph because, in cases where the call graph is very large, I want to avoid constructing the entire thing.
I have kept the get_inverted function, but it's now much easier to traverse the call graph backwards, so I think it might be possible to get rid of this eventually.
I have updated the unit test for call_grapht.

Rewriting call_grapht also highlighted some minor changes in grapht that should be made.

This was needed after an update to goto-cc, which caused previously-compiled binaries used in the test suite to trigger an assertion failure. Also updated instructions on how to generate these binaries. This commit fixes diffblue#1376.

Without the "digraph call_graph{" this is not parsable dot. This change makes the output dot consistent with the previous output_dot function in call_grapht

…it from grapht<T> I have rewritten call_grapht to inherit from grapht, because I think there should be fewer graph classes in CBMC. Instead of constructing the entire call graph, the reachable call graph class constructs only the reachable call graph. There is some duplication between the call_graph class and the show_call_sequences function, however the show_call_sequences function outputs the graph on the fly, instead of constructing it, which doesn't allow for further use of the graph

Michael's comments on CR-766007 highlighted things that should be changed in grapht.

Returns the list of functions on the shortest path from a src function to a destination function on the function call graph.

smowton

So given I was recently burned by merging something that changed a public interface, I think we should try to keep it stable here.

call_grapht exposed all of its implementation, inviting users to directly inspect the std::multimap<irep_idt, irep_idt> it used to store the graph. Therefore if we don't want to surprise external users, we should keep that.

That's why #1498 keeps the original call_grapht interface but adds the ability to export it in grapht form. I propose we merge #1498 and then build this functionality (the single-root call graph and backward-reachability functions for example) on top of it. I'm happy to do the porting work.

smowton · 2017-10-24T09:29:43Z

regression/ansi-c/arch_flags_mcpu_good/test.desc


-On Ubuntu, you can get a suitable compiler using:
+To regenerate object.arm on an x86_64 machine, you will need to install


I don't think these changes belong in this PR

I'm not entirely sure why this commit has appeared in the PR, since it is the head commit of the develop branch (or was when I created this branch). I created the branch by checking out develop and then cherry-picking my commits on top of it.

Nevertheless, I need to rebase against develop again anyway so I will make it disappear then

smowton · 2017-10-24T09:31:44Z

src/analyses/call_graph.cpp

-  out << "}\n";
+void call_grapht::output_xml(std::ostream &out) const
+{
+  for(node_indext n = 0; n < nodes.size(); n++)


Use C++11 for-loop (i.e. for(const auto &node : nodes) output_xml_node(out, node);)

The reason for this for loop is that the output_xml_node function requires a node indext to be passed to it.

The reason the output_xml_node function requires a node indext to be passed to it, rather than a reference to the node is to preserve the compatability with the output functions of grapht (see grapht::output_dot_node).

And the reason graphtoutput_dot_node needs to have the node index passed to it is because it outputs that node index (instead of outputting data stored in the node, because the template doesn't know what might be contained in the node).

In order to use a C++11 for loop I would have to reimplement the output functions of grapht, would you still prefer that?

OK never mind then

smowton · 2017-10-24T09:32:39Z

src/analyses/call_graph.cpp

 }

-void call_grapht::output_xml(std::ostream &out) const
+std::list<irep_idt>call_grapht::shortest_function_path


Space after >

smowton · 2017-10-24T09:33:08Z

src/analyses/call_graph.cpp

 }

-void call_grapht::output_xml(std::ostream &out) const
+std::list<irep_idt>call_grapht::shortest_function_path
+(irep_idt src, irep_idt dest)


Open-paren on previous line, indent this by two

smowton · 2017-10-24T09:34:44Z

src/analyses/call_graph.cpp

+  std::list<irep_idt> result;
+  node_indext src_idx, dest_idx;
+  if(!get_node_index(src, src_idx))
+    throw "unable to find src function in call graph";


If we're throwing, throw a structured error (probably just call_graph_errort : public std::invalid_argument {} that a caller can selectively catch

smowton · 2017-10-24T09:47:05Z

src/analyses/call_graph.h

+  /// \brief get a list of functions called by a function
+  /// \param function_name   the irep_idt of the function
+  /// \return an unordered set of all functions called by function_name
+  std::unordered_set<irep_idt, irep_id_hash> get_successors(


Members of out are guaranteed to be unique, so we could return vector here and let the caller decide whether they need the more-costly hashtable

Sure. Actually the only place this is called is the unit test, so it could just be a function within the unit test

smowton · 2017-10-24T09:50:41Z

src/analyses/reachable_call_graph.cpp

+    irep_idt caller=*working_queue.begin();
+    working_queue.erase(working_queue.begin());
+
+    if(!done.insert(caller).second)


Given this is happening, I don't think working_queue needs to itself be a set type

smowton · 2017-10-24T09:52:14Z

src/analyses/reachable_call_graph.cpp

+        {
+          irep_idt callee = to_symbol_expr(function_expr).get_identifier();
+          add(caller, callee);
+          working_queue.insert(callee);


Same as above, depth-first search would mean cheaper types (stacks made out of vectors) for the working set

smowton · 2017-10-24T09:57:50Z

src/analyses/reachable_call_graph.cpp

+{
+  std::unordered_set<irep_idt, irep_id_hash> result;
+  std::list<node_indext> worklist;
+  for(node_indext idx=0; idx<nodes.size(); idx++)


Use get_node_index

smowton · 2017-10-24T10:02:20Z

src/analyses/reachable_call_graph.cpp

+  }
+  INVARIANT(!worklist.empty(), "destination function not found");
+
+  while(!worklist.empty())


Factor this and the forward-reachable function above

polgreen · 2017-10-24T11:18:07Z

Can we keep the original call graph interface and the one built on top of grapht for some period of time, but then eventually remove the original one? That would allow external users time to adapt their code to the new one, but also wouldn't be adding to the already very large collection of graph classes in CBMC

smowton · 2017-10-24T12:51:07Z

Sure sounds good. I'll put together a PR cherry-picked from these two that combines their features...

polgreen · 2017-10-24T12:56:00Z

Ok, thanks. It might take me a couple of days to get round to the changes in here, but I hope I will have them done by the end of the week

smowton · 2017-10-25T15:18:05Z

To avoid duplicating effort: making a merged version of these two PRs now...

smowton · 2017-10-25T17:14:04Z

Made a draft here for review: https://github.com/smowton/cbmc/tree/call_graph_polgreen_draft

(The latest commit adds most of the features from this PR on top of #1498 -- it still needs the DOT/XML output stuff porting however)

polgreen · 2017-10-27T15:58:04Z

Thanks Chris! Sorry I haven't had time to look at any of this, I should be able to get to it next week.

I'll do the re-implementation of the BFS as DFS etc on top of a copy of your new combined pull request, is that ok? I can also do the dot/xml stuff

smowton · 2017-10-27T16:01:55Z

The DFS is already in there actually (it dropped out naturally from tweaking grapht a little). DOT/XML would be much appreciated though.

I was also considering whether perhaps to avoid cluttering call_graph.h too much, perhaps the get-successors/predecessors/reachable-nodes functions should move into some call_graph_helpers.h as out-of-line functions, since they can do all their work using grapht's public interface?

polgreen · 2017-10-27T16:11:47Z

That works for me. An alternative, if there is no likely use for them elsewhere, would be to integrate them into the call graph based slicer i built them for. I'll look and see if that would actually improve the efficiency of the slicer at all, and maybe we can have that discussion once i've created a pull request for it

smowton · 2017-10-27T16:19:11Z

Sounds good to me. Note the graph now hosts a name->index map, so the lookup is cheaper than it was.

tautschnig · 2017-11-01T15:15:47Z

@smowton @polgreen As there seem to be two possible variants of this PR/branch now, which one is to be used?

polgreen · 2017-11-01T15:22:06Z

The pull request we are commenting on is identical to the internal code review you looked at.

Chris has created a branch, which merges this pull request and #1498 : https://github.com/smowton/cbmc/tree/call_graph_polgreen_draft

I need to add in the dot output changes, which I haven't got round to doing yet.

smowton · 2017-12-10T13:57:03Z

@tautschnig @polgreen finally got around to completing the merge! You can find the results at my original PR, #1498

thk123 · 2018-01-05T12:28:10Z

Is this now redundant/incorporated into #1498? @smowton ?

smowton · 2018-01-05T13:06:34Z

Yes, closing this as subsumed by #1498

polgreen requested review from kroening, martin-cs, peterschrammel, smowton, tautschnig and thk123 as code owners October 23, 2017 16:19

karkhaz and others added 5 commits October 23, 2017 18:21

Regenerated cross-compiled arch flag test binaries

bcccb98

This was needed after an update to goto-cc, which caused previously-compiled binaries used in the test suite to trigger an assertion failure. Also updated instructions on how to generate these binaries. This commit fixes diffblue#1376.

grapht::output_dot needs digraph declaration

b47bb42

Without the "digraph call_graph{" this is not parsable dot. This change makes the output dot consistent with the previous output_dot function in call_grapht

CR fixes for grapht

04e56c3

Michael's comments on CR-766007 highlighted things that should be changed in grapht.

return shortest path from function call graph

799d034

Returns the list of functions on the shortest path from a src function to a destination function on the function call graph.

polgreen force-pushed the call_graph_changes branch from bb0327f to 799d034 Compare October 23, 2017 16:21

polgreen mentioned this pull request Oct 23, 2017

Call graph enhancements #1498

Merged

Documentation of call graph and reachable call graph

635071e

polgreen force-pushed the call_graph_changes branch from 74d0638 to 635071e Compare October 23, 2017 18:46

smowton reviewed Oct 24, 2017

View reviewed changes

white space call graph

6f7d4b4

tautschnig assigned smowton and polgreen Nov 1, 2017

polgreen mentioned this pull request Nov 14, 2017

Aggressive slicer #1587

Closed

smowton closed this Jan 5, 2018

polgreen deleted the call_graph_changes branch June 27, 2018 08:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewriting call_grapht to inherit from grapht<T>, and introduce reachable_call_graph class #1509

Rewriting call_grapht to inherit from grapht<T>, and introduce reachable_call_graph class #1509

polgreen commented Oct 23, 2017

smowton left a comment •

edited

Loading

smowton Oct 24, 2017

polgreen Oct 24, 2017

smowton Oct 24, 2017

polgreen Oct 24, 2017

smowton Oct 24, 2017

smowton Oct 24, 2017

smowton Oct 24, 2017

smowton Oct 24, 2017

smowton Oct 24, 2017

polgreen Oct 24, 2017

smowton Oct 24, 2017

smowton Oct 24, 2017

smowton Oct 24, 2017

smowton Oct 24, 2017

polgreen commented Oct 24, 2017

smowton commented Oct 24, 2017

polgreen commented Oct 24, 2017

smowton commented Oct 25, 2017

smowton commented Oct 25, 2017

polgreen commented Oct 27, 2017

smowton commented Oct 27, 2017

polgreen commented Oct 27, 2017

smowton commented Oct 27, 2017

tautschnig commented Nov 1, 2017

polgreen commented Nov 1, 2017

smowton commented Dec 10, 2017

thk123 commented Jan 5, 2018 •

edited

Loading

smowton commented Jan 5, 2018


		On Ubuntu, you can get a suitable compiler using:
		To regenerate object.arm on an x86_64 machine, you will need to install

Rewriting call_grapht to inherit from grapht<T>, and introduce reachable_call_graph class #1509

Rewriting call_grapht to inherit from grapht<T>, and introduce reachable_call_graph class #1509

Conversation

polgreen commented Oct 23, 2017

smowton left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

polgreen commented Oct 24, 2017

smowton commented Oct 24, 2017

polgreen commented Oct 24, 2017

smowton commented Oct 25, 2017

smowton commented Oct 25, 2017

polgreen commented Oct 27, 2017

smowton commented Oct 27, 2017

polgreen commented Oct 27, 2017

smowton commented Oct 27, 2017

tautschnig commented Nov 1, 2017

polgreen commented Nov 1, 2017

smowton commented Dec 10, 2017

thk123 commented Jan 5, 2018 • edited Loading

smowton commented Jan 5, 2018

smowton left a comment •

edited

Loading

thk123 commented Jan 5, 2018 •

edited

Loading