[PATCH] Add dotfn

[PATCH] Add dotfn

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 384 bytes --]

Hi,

this patch adds a debug function dotfn and a convenience macro DOTFN 
similar to dot-fn in gdbhooks.py.

It can be used to have the compiler:
- dump a control flow graph, or
- pop up a control flow graph window
at specific moments in the compilation flow, for debugging purposes.

Bootstrapped and reg-tested on x86_64.

Used for debugging PR81192.

OK for trunk?

Thanks,
- Tom

[-- Attachment #2: 0001-Add-dotfn.patch --]
[-- Type: text/x-patch, Size: 3337 bytes --]

Add dotfn

2017-06-30  Tom de Vries  <tom@codesourcery.com>

	* graph.c (get_graph_file_name): New function, factored out of ...
	(open_graph_file): ... here.
	(dotfn): New function.
	* graph.h (dotfn): Declare.
	(DOTFN): New macro.

---
 gcc/graph.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++++++++++------
 gcc/graph.h |  7 +++++++
 2 files changed, 69 insertions(+), 6 deletions(-)

diff --git a/gcc/graph.c b/gcc/graph.c
index 9261732..e6d3f2e 100644
--- a/gcc/graph.c
+++ b/gcc/graph.c
@@ -37,23 +37,45 @@ along with GCC; see the file COPYING3.  If not see
    ignore that recommendation...  */
 static const char *const graph_ext = ".dot";
 
+/* Return filename for dumping our graph to.  If NUM, use *NUM in the file name
+   and increase *NUM.  */
+
+static char *
+get_graph_file_name (const char *base, unsigned int *num = NULL)
+{
+  size_t namelen = strlen (base);
+  size_t extlen = strlen (graph_ext);
+  size_t numlen = num ? 11 : 0;
+  size_t len = namelen + extlen + numlen + 1;
+  char *buf = XNEWVEC (char, len);
+
+  char *pos = buf;
+  memcpy (pos, base, namelen);
+  pos += namelen;
+  if (num)
+    {
+      pos += snprintf (pos, numlen, ".%u", *num);
+      ++*num;
+    }
+  memcpy (pos, graph_ext, extlen + 1);
+
+  return buf;
+}
+
 /* Open a file with MODE for dumping our graph to.
    Return the file pointer.  */
 static FILE *
 open_graph_file (const char *base, const char *mode)
 {
-  size_t namelen = strlen (base);
-  size_t extlen = strlen (graph_ext) + 1;
-  char *buf = XALLOCAVEC (char, namelen + extlen);
+  char *buf = get_graph_file_name (base);
   FILE *fp;
 
-  memcpy (buf, base, namelen);
-  memcpy (buf + namelen, graph_ext, extlen);
-
   fp = fopen (buf, mode);
   if (fp == NULL)
     fatal_error (input_location, "can%'t open %s: %m", buf);
 
+  XDELETEVEC (buf);
+
   return fp;
 }
 
@@ -354,3 +376,37 @@ finish_graph_dump_file (const char *base)
   end_graph_dump (fp);
   fclose (fp);
 }
+
+/* Dump graph into file '<base>.dot', or if COUNTER is defined
+   '<base>.<*COUNTER>.dot'.  If POPUP, show the graph in a popup window.  */
+
+void DEBUG_FUNCTION
+dotfn (const char *base, unsigned int *counter, bool popup)
+{
+  char *name = get_graph_file_name (base, counter);
+
+  FILE *fp = fopen (name, "w");
+  if (fp != NULL)
+    {
+      start_graph_dump (fp, "<debug>");
+      print_graph_cfg (fp, cfun, dump_flags);
+      end_graph_dump (fp);
+      fclose (fp);
+
+      if (popup)
+	{
+	  char prog[] = "dot";
+	  char arg1[] = "-Tx11";
+	  char *const cmd[] = { prog, arg1, name , NULL };
+	  struct pex_obj *pex = pex_init (0, prog, NULL);
+	  int err;
+	  pex_run (pex, PEX_LAST | PEX_SEARCH, prog, cmd, NULL, NULL, &err);
+
+	  int status;
+	  pex_get_status (pex, 1, &status);
+	  pex_free (pex);
+	}
+    }
+
+  XDELETEVEC (name);
+}
diff --git a/gcc/graph.h b/gcc/graph.h
index 4db253f..168b60c 100644
--- a/gcc/graph.h
+++ b/gcc/graph.h
@@ -24,4 +24,11 @@ extern void print_graph_cfg (const char *, struct function *);
 extern void clean_graph_dump_file (const char *);
 extern void finish_graph_dump_file (const char *);
 
+extern void dotfn (const char *base, unsigned int *counter, bool popup);
+#define DOTFN(base, popup)			\
+  {						\
+    static unsigned int counter = 0;		\
+    dotfn (base, &counter, popup);		\
+  }
+
 #endif /* ! GCC_GRAPH_H */

Re: [PATCH] Add dotfn

[Richard Biener]

On Mon, 3 Jul 2017, Tom de Vries wrote:

> Hi,
> 
> this patch adds a debug function dotfn and a convenience macro DOTFN similar
> to dot-fn in gdbhooks.py.
> 
> It can be used to have the compiler:
> - dump a control flow graph, or
> - pop up a control flow graph window
> at specific moments in the compilation flow, for debugging purposes.
> 
> Bootstrapped and reg-tested on x86_64.
> 
> Used for debugging PR81192.
> 
> OK for trunk?

Why's dot-fn not enough?  I'd rather extend stuff in gdbhooks.py than
adding this kind of stuff to gcc itself.

Thanks,
Richard.

Re: [PATCH] Add dotfn

[Tom de Vries]

On 07/03/2017 09:05 AM, Richard Biener wrote:
> On Mon, 3 Jul 2017, Tom de Vries wrote:
> 
>> Hi,
>>
>> this patch adds a debug function dotfn and a convenience macro DOTFN similar
>> to dot-fn in gdbhooks.py.
>>
>> It can be used to have the compiler:
>> - dump a control flow graph, or
>> - pop up a control flow graph window
>> at specific moments in the compilation flow, for debugging purposes.
>>
>> Bootstrapped and reg-tested on x86_64.
>>
>> Used for debugging PR81192.
>>
>> OK for trunk?
> 
> Why's dot-fn not enough? > I'd rather extend stuff in gdbhooks.py than
> adding this kind of stuff to gcc itself.

When expressing where and when to dump or pop-up a control flow graph, 
sometimes it's easier for me to do that in C than in gdb scripting.

Thanks,
- Tom

Re: [PATCH] Add dotfn

[Richard Biener]

On Mon, 3 Jul 2017, Tom de Vries wrote:

> On 07/03/2017 09:05 AM, Richard Biener wrote:
> > On Mon, 3 Jul 2017, Tom de Vries wrote:
> > 
> > > Hi,
> > > 
> > > this patch adds a debug function dotfn and a convenience macro DOTFN
> > > similar
> > > to dot-fn in gdbhooks.py.
> > > 
> > > It can be used to have the compiler:
> > > - dump a control flow graph, or
> > > - pop up a control flow graph window
> > > at specific moments in the compilation flow, for debugging purposes.
> > > 
> > > Bootstrapped and reg-tested on x86_64.
> > > 
> > > Used for debugging PR81192.
> > > 
> > > OK for trunk?
> > 
> > Why's dot-fn not enough? > I'd rather extend stuff in gdbhooks.py than
> > adding this kind of stuff to gcc itself.
> 
> When expressing where and when to dump or pop-up a control flow graph,
> sometimes it's easier for me to do that in C than in gdb scripting.

Ah, you mean by patching GCC.  Yeah, I can see that this is useful
in some cases.  OTOH I had dot-fn this way in my local dev tree for
a few years ...

I'm retracting my objection but leave approval to somebody else
just to see if we can arrive at any consensus for "advanced"
debug stuff in GCC itself.

For my usecase the gdb python stuff is now nearly perfect -- apart
from the cases where graph generation ICEs (like corrupt loop info).

Richard.

Re: [PATCH] Add dotfn

[Tom de Vries]

On 07/03/2017 11:53 AM, Richard Biener wrote:
> On Mon, 3 Jul 2017, Tom de Vries wrote:
> 
>> On 07/03/2017 09:05 AM, Richard Biener wrote:
>>> On Mon, 3 Jul 2017, Tom de Vries wrote:
>>>
>>>> Hi,
>>>>
>>>> this patch adds a debug function dotfn and a convenience macro DOTFN
>>>> similar
>>>> to dot-fn in gdbhooks.py.
>>>>
>>>> It can be used to have the compiler:
>>>> - dump a control flow graph, or
>>>> - pop up a control flow graph window
>>>> at specific moments in the compilation flow, for debugging purposes.
>>>>
>>>> Bootstrapped and reg-tested on x86_64.
>>>>
>>>> Used for debugging PR81192.
>>>>
>>>> OK for trunk?
>>>
>>> Why's dot-fn not enough? > I'd rather extend stuff in gdbhooks.py than
>>> adding this kind of stuff to gcc itself.
>>
>> When expressing where and when to dump or pop-up a control flow graph,
>> sometimes it's easier for me to do that in C than in gdb scripting.
> 
> Ah, you mean by patching GCC.  Yeah, I can see that this is useful
> in some cases.  OTOH I had dot-fn this way in my local dev tree for
> a few years ...
> 
> I'm retracting my objection but leave approval to somebody else
> just to see if we can arrive at any consensus for "advanced"
> debug stuff in GCC itself.
> 

Ack.

> For my usecase the gdb python stuff is now nearly perfect -- apart
> from the cases where graph generation ICEs (like corrupt loop info).

I suppose we can make a dotfn variant that calls draw_cfg_nodes_no_loops 
even if loop info is present.


Btw, I think this needs fixing:
...
/* Draw all edges in the CFG.  Retreating edges are drawin as not 

    constraining, this makes the layout of the graph better. 

    (??? Calling mark_dfs_back may change the compiler's behavior when 

    dumping, but computing back edges here for ourselves is also not 

    desirable.)  */

static void
draw_cfg_edges (pretty_printer *pp, struct function *fun)
{
   basic_block bb;
   mark_dfs_back_edges ();
   FOR_ALL_BB_FN (bb, cfun)
     draw_cfg_node_succ_edges (pp, fun->funcdef_no, bb);
...

We don't want that calling a debug function changes compiler behavior 
(something I ran into while debugging PR81192).

Any suggestion on how to address this? We could allocate a bitmap before 
and save the edge flag for all edges, and restore afterwards.

Thanks,
- Tom

Re: [PATCH] Add dotfn

[Richard Biener]

On Mon, 3 Jul 2017, Tom de Vries wrote:

> On 07/03/2017 11:53 AM, Richard Biener wrote:
> > On Mon, 3 Jul 2017, Tom de Vries wrote:
> > 
> > > On 07/03/2017 09:05 AM, Richard Biener wrote:
> > > > On Mon, 3 Jul 2017, Tom de Vries wrote:
> > > > 
> > > > > Hi,
> > > > > 
> > > > > this patch adds a debug function dotfn and a convenience macro DOTFN
> > > > > similar
> > > > > to dot-fn in gdbhooks.py.
> > > > > 
> > > > > It can be used to have the compiler:
> > > > > - dump a control flow graph, or
> > > > > - pop up a control flow graph window
> > > > > at specific moments in the compilation flow, for debugging purposes.
> > > > > 
> > > > > Bootstrapped and reg-tested on x86_64.
> > > > > 
> > > > > Used for debugging PR81192.
> > > > > 
> > > > > OK for trunk?
> > > > 
> > > > Why's dot-fn not enough? > I'd rather extend stuff in gdbhooks.py than
> > > > adding this kind of stuff to gcc itself.
> > > 
> > > When expressing where and when to dump or pop-up a control flow graph,
> > > sometimes it's easier for me to do that in C than in gdb scripting.
> > 
> > Ah, you mean by patching GCC.  Yeah, I can see that this is useful
> > in some cases.  OTOH I had dot-fn this way in my local dev tree for
> > a few years ...
> > 
> > I'm retracting my objection but leave approval to somebody else
> > just to see if we can arrive at any consensus for "advanced"
> > debug stuff in GCC itself.
> > 
> 
> Ack.
> 
> > For my usecase the gdb python stuff is now nearly perfect -- apart
> > from the cases where graph generation ICEs (like corrupt loop info).
> 
> I suppose we can make a dotfn variant that calls draw_cfg_nodes_no_loops even
> if loop info is present.

Locally I have

@@ -236,7 +242,8 @@ draw_cfg_nodes_for_loop (pretty_printer
 static void
 draw_cfg_nodes (pretty_printer *pp, struct function *fun)
 {
-  if (loops_for_fn (fun))
+  if (loops_for_fn (fun)
+      && !(loops_for_fn (fun)->state & LOOPS_NEED_FIXUP))
     draw_cfg_nodes_for_loop (pp, fun->funcdef_no, get_loop (fun, 0));
   else
     draw_cfg_nodes_no_loops (pp, fun);

that avoids most of the cases but of course not always.  I suppose
a special dump_flag might work here.  The problem is really
get_loop_body* trusting loop->num_nodes and ICEing when that doesn't 
match.  Using get_loop_body_with_size with n_basic_blocks_for_fn
would avoid that but it isn't a replacement for
get_loop_body_in_bfs_order -- at least with get_loop_body_with_size
we could avoid repeatedly allocating the array in draw_cfg_nodes_for_loop.

Not sure if bfs_order dots so much nicer than dfs order.

> 
> Btw, I think this needs fixing:
> ...
> /* Draw all edges in the CFG.  Retreating edges are drawin as not 
>    constraining, this makes the layout of the graph better. 
>    (??? Calling mark_dfs_back may change the compiler's behavior when 
>    dumping, but computing back edges here for ourselves is also not 
>    desirable.)  */
> 
> static void
> draw_cfg_edges (pretty_printer *pp, struct function *fun)
> {
>   basic_block bb;
>   mark_dfs_back_edges ();
>   FOR_ALL_BB_FN (bb, cfun)
>     draw_cfg_node_succ_edges (pp, fun->funcdef_no, bb);
> ...
> 
> We don't want that calling a debug function changes compiler behavior
> (something I ran into while debugging PR81192).
> 
> Any suggestion on how to address this? We could allocate a bitmap before and
> save the edge flag for all edges, and restore afterwards.

Something like that, yes.

> Thanks,
> - Tom
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)

[PATCH] Save and restore EDGE_DFS_BACK in draw_cfg_edges

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 1366 bytes --]

[was: Re: [PATCH] Add dotfn ]

On 07/03/2017 12:23 PM, Richard Biener wrote:
>> Btw, I think this needs fixing:
>> ...
>> /* Draw all edges in the CFG.  Retreating edges are drawin as not
>>     constraining, this makes the layout of the graph better.
>>     (??? Calling mark_dfs_back may change the compiler's behavior when
>>     dumping, but computing back edges here for ourselves is also not
>>     desirable.)  */
>>
>> static void
>> draw_cfg_edges (pretty_printer *pp, struct function *fun)
>> {
>>    basic_block bb;
>>    mark_dfs_back_edges ();
>>    FOR_ALL_BB_FN (bb, cfun)
>>      draw_cfg_node_succ_edges (pp, fun->funcdef_no, bb);
>> ...
>>
>> We don't want that calling a debug function changes compiler behavior
>> (something I ran into while debugging PR81192).
>>
>> Any suggestion on how to address this? We could allocate a bitmap before and
>> save the edge flag for all edges, and restore afterwards.

> Something like that, yes.
> 

This patch implements that approach.

I've tried it with the PR81192 example and calling DOTFN in tail-merge, 
like this:
1. Just compiling the example without any patches gives a tail-merge
    sigsegv.
2. Compiling with the DOTFN call in tail-merge makes the sigsegv go
    away.
3. Adding this patch makes the sigsegv come back.

OK for trunk if bootstrap and reg-test on x86_64 succeeds?

Thanks,
- Tom

[-- Attachment #2: 0001-Save-and-restore-EDGE_DFS_BACK-in-draw_cfg_edges.patch --]
[-- Type: text/x-patch, Size: 2091 bytes --]

Save and restore EDGE_DFS_BACK in draw_cfg_edges

2017-07-03  Tom de Vries  <tom@codesourcery.com>

	* graph.c (draw_cfg_edges): Save and restore EDGE_DFS_BACK.

---
 gcc/graph.c | 49 +++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 45 insertions(+), 4 deletions(-)

diff --git a/gcc/graph.c b/gcc/graph.c
index 9261732..628769b 100644
--- a/gcc/graph.c
+++ b/gcc/graph.c
@@ -243,19 +243,60 @@ draw_cfg_nodes (pretty_printer *pp, struct function *fun)
 }
 
 /* Draw all edges in the CFG.  Retreating edges are drawin as not
-   constraining, this makes the layout of the graph better.
-   (??? Calling mark_dfs_back may change the compiler's behavior when
-   dumping, but computing back edges here for ourselves is also not
-   desirable.)  */
+   constraining, this makes the layout of the graph better.  */
 
 static void
 draw_cfg_edges (pretty_printer *pp, struct function *fun)
 {
   basic_block bb;
+
+  /* Save EDGE_DFS_BACK flag to dfs_back.  */
+  auto_bitmap dfs_back;
+  edge e;
+  edge_iterator ei;
+  unsigned int idx = 0;
+  FOR_EACH_BB_FN (bb, cfun)
+    {
+      FOR_EACH_EDGE (e, ei, bb->preds)
+	{
+	  if (e->flags & EDGE_DFS_BACK)
+	    bitmap_set_bit (dfs_back, idx);
+	  idx++;
+	}
+      FOR_EACH_EDGE (e, ei, bb->succs)
+	{
+	  if (e->flags & EDGE_DFS_BACK)
+	    bitmap_set_bit (dfs_back, idx);
+	  idx++;
+	}
+    }
+
   mark_dfs_back_edges ();
   FOR_ALL_BB_FN (bb, cfun)
     draw_cfg_node_succ_edges (pp, fun->funcdef_no, bb);
 
+  /* Restore EDGE_DFS_BACK flag from dfs_back.  */
+  idx = 0;
+  FOR_EACH_BB_FN (bb, cfun)
+    {
+      FOR_EACH_EDGE (e, ei, bb->preds)
+	{
+	  if (bitmap_bit_p (dfs_back, idx))
+	    e->flags |= EDGE_DFS_BACK;
+	  else
+	    e->flags &= ~EDGE_DFS_BACK;
+	  idx++;
+	}
+      FOR_EACH_EDGE (e, ei, bb->succs)
+	{
+	  if (bitmap_bit_p (dfs_back, idx))
+	    e->flags |= EDGE_DFS_BACK;
+	  else
+	    e->flags &= ~EDGE_DFS_BACK;
+	  idx++;
+	}
+    }
+
   /* Add an invisible edge from ENTRY to EXIT, to improve the graph layout.  */
   pp_printf (pp,
 	     "\tfn_%d_basic_block_%d:s -> fn_%d_basic_block_%d:n "

Re: [PATCH] Save and restore EDGE_DFS_BACK in draw_cfg_edges

[Richard Biener]

On Tue, 4 Jul 2017, Tom de Vries wrote:

> [was: Re: [PATCH] Add dotfn ]
> 
> On 07/03/2017 12:23 PM, Richard Biener wrote:
> > > Btw, I think this needs fixing:
> > > ...
> > > /* Draw all edges in the CFG.  Retreating edges are drawin as not
> > >     constraining, this makes the layout of the graph better.
> > >     (??? Calling mark_dfs_back may change the compiler's behavior when
> > >     dumping, but computing back edges here for ourselves is also not
> > >     desirable.)  */
> > > 
> > > static void
> > > draw_cfg_edges (pretty_printer *pp, struct function *fun)
> > > {
> > >    basic_block bb;
> > >    mark_dfs_back_edges ();
> > >    FOR_ALL_BB_FN (bb, cfun)
> > >      draw_cfg_node_succ_edges (pp, fun->funcdef_no, bb);
> > > ...
> > > 
> > > We don't want that calling a debug function changes compiler behavior
> > > (something I ran into while debugging PR81192).
> > > 
> > > Any suggestion on how to address this? We could allocate a bitmap before
> > > and
> > > save the edge flag for all edges, and restore afterwards.
> 
> > Something like that, yes.
> > 
> 
> This patch implements that approach.
> 
> I've tried it with the PR81192 example and calling DOTFN in tail-merge, like
> this:
> 1. Just compiling the example without any patches gives a tail-merge
>    sigsegv.
> 2. Compiling with the DOTFN call in tail-merge makes the sigsegv go
>    away.
> 3. Adding this patch makes the sigsegv come back.
> 
> OK for trunk if bootstrap and reg-test on x86_64 succeeds?

You don't need to iterate over both preds and succs, succs
is enough.

Ok with that change.

Richard.

> Thanks,
> - Tom
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)