diff mbox

[PR70185] Only finalize dot files that have been initialized

Message ID 56EA768D.8040706@mentor.com
State New
Headers show

Commit Message

Tom de Vries March 17, 2016, 9:19 a.m. UTC 
On 16/03/16 12:34, Richard Biener wrote:
> On Wed, Mar 16, 2016 at 11:57 AM, Tom de Vries <Tom_deVries@mentor.com> wrote:
>> Hi,
>>
>> Atm, using fdump-tree-all-graph produces invalid dot files:
>> ...
>> $ rm *.c.* ; gcc test.c -O2 -S -fdump-tree-all-graph
>> $ for f in *.dot; do dot -Tpdf $f -o dot.pdf; done
>> Warning: test.c.006t.omplower.dot: syntax error in line 1 near '}'
>> Warning: test.c.007t.lower.dot: syntax error in line 1 near '}'
>> Warning: test.c.010t.eh.dot: syntax error in line 1 near '}'
>> Warning: test.c.292t.statistics.dot: syntax error in line 1 near '}'
>> $ cat test.c.006t.omplower.dot
>> }
>> $
>> ...
>> These dot files are finalized, but never initialized or used.
>>
>> The 006/007/010 files are not used because '(fn->curr_properties & PROP_cfg)
>> == 0' at the corresponding passes.
>>
>> And the file test.c.292t.statistics.dot is not used, because it doesn't
>> belong to a single pass.
>>
>> The current finalization code doesn't handle these cases:
>> ...
>>    /* Do whatever is necessary to finish printing the graphs.  */
>>    for (i = TDI_end; (dfi = dumps->get_dump_file_info (i)) != NULL; ++i)
>>      if (dumps->dump_initialized_p (i)
>>          && (dfi->pflags & TDF_GRAPH) != 0
>>          && (name = dumps->get_dump_file_name (i)) != NULL)
>>        {
>>          finish_graph_dump_file (name);
>>          free (name);
>>        }
>> ...
>>
>> The patch fixes this by simply testing for pass->graph_dump_initialized
>> instead.
>>
>> [ That fix exposes the lack of initialization of graph_dump_initialized. It
>> seems to be initialized for static passes, but for dynamically added passes,
>> such as f.i. vzeroupper the value is uninitialized. The patch also fixes
>> this. ]
>>
>> Bootstrapped and reg-tested on x86_64.
>>
>> OK for stage1?
>
> Seeing this I wonder if it makes more sense to move ->graph_dump_initialized
> from pass to dump_file_info?

Done.

> Also in the above shouldn't it use
> dfi->pfilename rather than dumps->get_dump_file_name (i)?

That one isn't defined anymore once we get to finish_optimization_passes.

OK for stage1 if bootstrap and reg-test succeeds?

Thanks,
- Tom

Comments

Richard Biener March 17, 2016, 10:12 a.m. UTC | #1 
On Thu, Mar 17, 2016 at 10:19 AM, Tom de Vries <Tom_deVries@mentor.com> wrote:
> On 16/03/16 12:34, Richard Biener wrote:
>>
>> On Wed, Mar 16, 2016 at 11:57 AM, Tom de Vries <Tom_deVries@mentor.com>
>> wrote:
>>>
>>> Hi,
>>>
>>> Atm, using fdump-tree-all-graph produces invalid dot files:
>>> ...
>>> $ rm *.c.* ; gcc test.c -O2 -S -fdump-tree-all-graph
>>> $ for f in *.dot; do dot -Tpdf $f -o dot.pdf; done
>>> Warning: test.c.006t.omplower.dot: syntax error in line 1 near '}'
>>> Warning: test.c.007t.lower.dot: syntax error in line 1 near '}'
>>> Warning: test.c.010t.eh.dot: syntax error in line 1 near '}'
>>> Warning: test.c.292t.statistics.dot: syntax error in line 1 near '}'
>>> $ cat test.c.006t.omplower.dot
>>> }
>>> $
>>> ...
>>> These dot files are finalized, but never initialized or used.
>>>
>>> The 006/007/010 files are not used because '(fn->curr_properties &
>>> PROP_cfg)
>>> == 0' at the corresponding passes.
>>>
>>> And the file test.c.292t.statistics.dot is not used, because it doesn't
>>> belong to a single pass.
>>>
>>> The current finalization code doesn't handle these cases:
>>> ...
>>>    /* Do whatever is necessary to finish printing the graphs.  */
>>>    for (i = TDI_end; (dfi = dumps->get_dump_file_info (i)) != NULL; ++i)
>>>      if (dumps->dump_initialized_p (i)
>>>          && (dfi->pflags & TDF_GRAPH) != 0
>>>          && (name = dumps->get_dump_file_name (i)) != NULL)
>>>        {
>>>          finish_graph_dump_file (name);
>>>          free (name);
>>>        }
>>> ...
>>>
>>> The patch fixes this by simply testing for pass->graph_dump_initialized
>>> instead.
>>>
>>> [ That fix exposes the lack of initialization of graph_dump_initialized.
>>> It
>>> seems to be initialized for static passes, but for dynamically added
>>> passes,
>>> such as f.i. vzeroupper the value is uninitialized. The patch also fixes
>>> this. ]
>>>
>>> Bootstrapped and reg-tested on x86_64.
>>>
>>> OK for stage1?
>>
>>
>> Seeing this I wonder if it makes more sense to move
>> ->graph_dump_initialized
>> from pass to dump_file_info?
>
>
> Done.
>
>> Also in the above shouldn't it use
>> dfi->pfilename rather than dumps->get_dump_file_name (i)?
>
>
> That one isn't defined anymore once we get to finish_optimization_passes.
>
> OK for stage1 if bootstrap and reg-test succeeds?

Ok.

Richard.

> Thanks,
> - Tom
>
diff mbox

Patch

Only finalize dot files that have been initialized

2016-03-16  Tom de Vries  <tom@codesourcery.com>

	PR other/70185
	* tree-pass.h (class opt_pass): Remove graph_dump_initialized member.
	* dumpfile.h (struct dump_file_info): Add graph_dump_initialized field.
	* dumpfile.c (dump_files): Initialize graph_dump_initialized field.
	* passes.c (finish_optimization_passes): Only call
	finish_graph_dump_file if dfi->graph_dump_initialized.
	(execute_function_dump, pass_init_dump_file): Use
	dfi->graph_dump_initialized instead of pass->graph_dump_initialized.

---
 gcc/dumpfile.c  | 22 +++++++++++-----------
 gcc/dumpfile.h  |  4 ++++
 gcc/passes.c    | 16 ++++++++++------
 gcc/tree-pass.h |  5 -----
 4 files changed, 25 insertions(+), 22 deletions(-)

diff --git a/gcc/dumpfile.c b/gcc/dumpfile.c
index 144e371..74522a6 100644
--- a/gcc/dumpfile.c
+++ b/gcc/dumpfile.c
@@ -50,29 +50,29 @@  int dump_flags;
    TREE_DUMP_INDEX enumeration in dumpfile.h.  */
 static struct dump_file_info dump_files[TDI_end] =
 {
-  {NULL, NULL, NULL, NULL, NULL, NULL, NULL, 0, 0, 0, 0, 0, 0, false},
+  {NULL, NULL, NULL, NULL, NULL, NULL, NULL, 0, 0, 0, 0, 0, 0, false, false},
   {".cgraph", "ipa-cgraph", NULL, NULL, NULL, NULL, NULL, TDF_IPA,
-   0, 0, 0, 0, 0, false},
+   0, 0, 0, 0, 0, false, false},
   {".type-inheritance", "ipa-type-inheritance", NULL, NULL, NULL, NULL, NULL, TDF_IPA,
-   0, 0, 0, 0, 0, false},
+   0, 0, 0, 0, 0, false, false},
   {".tu", "translation-unit", NULL, NULL, NULL, NULL, NULL, TDF_TREE,
-   0, 0, 0, 0, 1, false},
+   0, 0, 0, 0, 1, false, false},
   {".class", "class-hierarchy", NULL, NULL, NULL, NULL, NULL, TDF_TREE,
-   0, 0, 0, 0, 2, false},
+   0, 0, 0, 0, 2, false, false},
   {".original", "tree-original", NULL, NULL, NULL, NULL, NULL, TDF_TREE,
-   0, 0, 0, 0, 3, false},
+   0, 0, 0, 0, 3, false, false},
   {".gimple", "tree-gimple", NULL, NULL, NULL, NULL, NULL, TDF_TREE,
-   0, 0, 0, 0, 4, false},
+   0, 0, 0, 0, 4, false, false},
   {".nested", "tree-nested", NULL, NULL, NULL, NULL, NULL, TDF_TREE,
-   0, 0, 0, 0, 5, false},
+   0, 0, 0, 0, 5, false, false},
 #define FIRST_AUTO_NUMBERED_DUMP 6
 
   {NULL, "tree-all", NULL, NULL, NULL, NULL, NULL, TDF_TREE,
-   0, 0, 0, 0, 0, false},
+   0, 0, 0, 0, 0, false, false},
   {NULL, "rtl-all", NULL, NULL, NULL, NULL, NULL, TDF_RTL,
-   0, 0, 0, 0, 0, false},
+   0, 0, 0, 0, 0, false, false},
   {NULL, "ipa-all", NULL, NULL, NULL, NULL, NULL, TDF_IPA,
-   0, 0, 0, 0, 0, false},
+   0, 0, 0, 0, 0, false, false},
 };
 
 /* Define a name->number mapping for a dump flag value.  */
diff --git a/gcc/dumpfile.h b/gcc/dumpfile.h
index c168cbf..3f08b16 100644
--- a/gcc/dumpfile.h
+++ b/gcc/dumpfile.h
@@ -120,6 +120,10 @@  struct dump_file_info
   bool owns_strings;            /* fields "suffix", "swtch", "glob" can be
 				   const strings, or can be dynamically
 				   allocated, needing free.  */
+  bool graph_dump_initialized;  /* When a given dump file is being initialized,
+				   this flag is set to true if the corresponding
+				   TDF_graph dump file has also been
+				   initialized.  */
 };
 
 /* In dumpfile.c */
diff --git a/gcc/passes.c b/gcc/passes.c
index 9d90251..5a98e50 100644
--- a/gcc/passes.c
+++ b/gcc/passes.c
@@ -364,10 +364,9 @@  finish_optimization_passes (void)
 
   /* Do whatever is necessary to finish printing the graphs.  */
   for (i = TDI_end; (dfi = dumps->get_dump_file_info (i)) != NULL; ++i)
-    if (dumps->dump_initialized_p (i)
-	&& (dfi->pflags & TDF_GRAPH) != 0
-	&& (name = dumps->get_dump_file_name (i)) != NULL)
+    if (dfi->graph_dump_initialized)
       {
+	name = dumps->get_dump_file_name (dfi);
 	finish_graph_dump_file (name);
 	free (name);
       }
@@ -1756,10 +1755,13 @@  execute_function_dump (function *fn, void *data)
       if ((fn->curr_properties & PROP_cfg)
 	  && (dump_flags & TDF_GRAPH))
 	{
-	  if (!pass->graph_dump_initialized)
+	  gcc::dump_manager *dumps = g->get_dumps ();
+	  struct dump_file_info *dfi
+	    = dumps->get_dump_file_info (pass->static_pass_number);
+	  if (!dfi->graph_dump_initialized)
 	    {
 	      clean_graph_dump_file (dump_file_name);
-	      pass->graph_dump_initialized = true;
+	      dfi->graph_dump_initialized = true;
 	    }
 	  print_graph_cfg (dump_file_name, fn);
 	}
@@ -2103,7 +2105,9 @@  pass_init_dump_file (opt_pass *pass)
 	  && cfun && (cfun->curr_properties & PROP_cfg))
 	{
 	  clean_graph_dump_file (dump_file_name);
-	  pass->graph_dump_initialized = true;
+	  struct dump_file_info *dfi
+	    = dumps->get_dump_file_info (pass->static_pass_number);
+	  dfi->graph_dump_initialized = true;
 	}
       timevar_pop (TV_DUMP);
       return initializing_dump;
diff --git a/gcc/tree-pass.h b/gcc/tree-pass.h
index 5f5055d..cd8c339 100644
--- a/gcc/tree-pass.h
+++ b/gcc/tree-pass.h
@@ -108,11 +108,6 @@  public:
   /* Static pass number, used as a fragment of the dump file name.  */
   int static_pass_number;
 
-  /* When a given dump file is being initialized, this flag is set to
-     true if the corresponding TDF_graph dump file has also been
-     initialized.  */
-  bool graph_dump_initialized;
-
 protected:
   gcc::context *m_ctxt;
 };