Re: [PATCH] Add dotfn

[Richard Biener <rguenther at suse dot de>]

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)