Re: [PATCH] Fix PR tree-optimization/17724

Re: [PATCH] Fix PR tree-optimization/17724

[Zdenek Dvorak]

Hello,

> ----- Forwarded message from Diego Novillo <dnovillo@redhat.com> -----
> 
> Date: Fri, 08 Oct 2004 19:28:08 -0400
> From: Diego Novillo <dnovillo@redhat.com>
> To: Jakub Jelinek <jakub@redhat.com>
> Cc: Richard Henderson <rth@redhat.com>,
>         "gcc-patches@gcc.gnu.org" <gcc-patches@gcc.gnu.org>
> Subject: Re: [PATCH] Fix PR tree-optimization/17724
> 
> On Thu, 2004-10-07 at 16:04, Jakub Jelinek wrote:
> 
> > The third one recomputes dominators (and post-dominators) of neighbours
> > of unreachable basic blocks after they are deleted by
> > delete_unreachable_blocks.
> > 
> I like this one.  It looks OK and safe, so let's go with it.
> 
> 
> Thanks.  Diego.
>
> ----- End forwarded message -----

this does not seem to be the right fix to me:

1) Change to delete_unreachable_blocks is not necessary.  As long as
   dominators are set correctly before delete_unreachable_blocks,
   they are also set correctly after it (since removal of unreachable
   blocks cannot affect any path from entry to basic block bb, and
   immediate dominator is determined from exactly these paths).

2) Change in tree_purge_dead_eh_edges also is not OK.
   tree_purge_dead_eh_edges may remove basically arbitrary edge.
   After removal of the edge, just locally updating the dominators
   of the destination of the edge is not sufficient, since also
   immediate dominators of other blocks may get changed.

3) The attempts to update postdominators are just wasted time, since
   we do not try to update those anywhere else (making updating of
   postdominators work would require changes on many more places,
   and so far it is not needed anywhere).

So the real problem for the bug is that tree_purge_dead_eh_edges
breaks the information about dominators irrepairably (at least in the
current state; it would be of course possible to write code to update
dominators after edge removal incrementally, but it would be relatively
complicated and I am not entirely sure it would pay up in the compile
time).  The right fix is simply to call free_dominance_info in
tree_purge_dead_eh_edges when the edge is removed.

Zdenek

[PATCH] Fix PR tree-optimization/17724 (take 2)

[Jakub Jelinek]

On Sat, Oct 09, 2004 at 11:00:00AM +0200, Zdenek Dvorak wrote:
> this does not seem to be the right fix to me:
> 
> 1) Change to delete_unreachable_blocks is not necessary.  As long as
>    dominators are set correctly before delete_unreachable_blocks,
>    they are also set correctly after it (since removal of unreachable
>    blocks cannot affect any path from entry to basic block bb, and
>    immediate dominator is determined from exactly these paths).

Are you sure that in all other cases where some basic blocks become
unreachable code that removes the edge ensures that dominators are freed
or recomputed for all basic blocks where idom(bb) could change?

I have tested following patch and it certainly cures the problem
(bootstrapped/regtested on 7 arches).

2004-10-09  Jakub Jelinek  <jakub@redhat.com>
	    Zdenek Dvorak  <dvorakz@suse.cz>

	PR tree-optimization/17724
	* tree-cfg.c (tree_purge_dead_eh_edges): Free dominance info.

	* g++.dg/opt/pr17724-1.C: New test.
	* g++.dg/opt/pr17724-2.C: New test.
	* g++.dg/opt/pr17724-3.C: New test.
	* g++.dg/opt/pr17724-4.C: New test.
	* g++.dg/opt/pr17724-5.C: New test.
	* g++.dg/opt/pr17724-6.C: New test.

--- gcc/tree-cfg.c.jj	2004-10-02 04:46:07.000000000 -0400
+++ gcc/tree-cfg.c	2004-10-09 13:24:42.000000000 -0400
@@ -5016,6 +5016,26 @@ tree_purge_dead_eh_edges (basic_block bb
 	ei_next (&ei);
     }
 
+  /* Removal of dead EH edges might change dominators of not
+     just immediate successors.  E.g. when bb1 is changed so that
+     it no longer can throw and bb1->bb3 and bb1->bb4 are dead
+     eh edges purged by this function in:
+           0
+	  / \
+	 v   v
+	 1-->2
+        / \  |
+       v   v |
+       3-->4 |
+        \    v
+	 --->5
+	     |
+	     -
+     idom(bb5) must be recomputed.  For now just free the dominance
+     info.  */
+  if (changed)
+    free_dominance_info (CDI_DOMINATORS);
+
   return changed;
 }
 
--- gcc/testsuite/g++.dg/opt/pr17724-4.C.jj	2004-10-02 11:27:29.000000000 +0200
+++ gcc/testsuite/g++.dg/opt/pr17724-4.C	2004-10-02 11:29:21.803990546 +0200
@@ -0,0 +1,24 @@
+// PR tree-optimization/17724
+// { dg-do compile }
+// { dg-options "-O2" }
+
+extern "C" char *strcpy (char* d, const char* s);
+
+class A { public: A (); ~A (); };
+
+inline char * B (char *s, const char *t)
+{ return ::strcpy (s, t); }
+
+class C { int D (void); int E; };
+
+int C::D (void)
+{
+  A a;
+  try
+    {
+      char z[22];
+      if (this->E) B (z, "");
+      return 0;
+    }
+  catch (int &f) { return -1; }
+}
--- gcc/testsuite/g++.dg/opt/pr17724-6.C.jj	2004-10-02 11:27:29.000000000 +0200
+++ gcc/testsuite/g++.dg/opt/pr17724-6.C	2004-10-02 11:28:39.000000000 +0200
@@ -0,0 +1,24 @@
+// PR tree-optimization/17724
+// { dg-do compile }
+// { dg-options "-O2" }
+
+extern char *strcpy (char* d, const char* s);
+
+class A { public: A (); ~A (); };
+
+inline char * B (char *s, const char *t)
+{ return ::strcpy (s, t); }
+
+class C { int D (void); int E; };
+
+int C::D (void)
+{
+  A a;
+  try
+    {
+      char z[22];
+      if (this->E) B (z, "");
+      return 0;
+    }
+  catch (int &f) { return -1; }
+}
--- gcc/testsuite/g++.dg/opt/pr17724-1.C.jj	2004-10-02 11:13:45.797278812 +0200
+++ gcc/testsuite/g++.dg/opt/pr17724-1.C	2004-10-02 11:13:45.797278812 +0200
@@ -0,0 +1,23 @@
+// PR tree-optimization/17724
+// { dg-do compile }
+// { dg-options "-O2" }
+
+namespace N { char *strcpy (char *, const char *); }
+extern "C" char *strcpy (char *, const char *) throw ();
+inline char *N::strcpy (char *s, const char *t) { return ::strcpy (s, t); }
+
+struct S { ~S (); };
+int foo ();
+
+int
+main ()
+{
+  S s;
+  int a;
+  char b[64];
+  N::strcpy (b, "ABCDEFGHIJKLM");
+  while ((a = foo ()) != -1)
+    if (a)
+      return -1;
+  return 0;
+}
--- gcc/testsuite/g++.dg/opt/pr17724-2.C.jj	2004-10-02 11:13:45.797278812 +0200
+++ gcc/testsuite/g++.dg/opt/pr17724-2.C	2004-10-02 11:13:45.797278812 +0200
@@ -0,0 +1,23 @@
+// PR tree-optimization/17724
+// { dg-do compile }
+// { dg-options "-O2" }
+
+namespace N { char *strcpy (char *, const char *); }
+extern "C" char *strcpy (char *, const char *);
+inline char *N::strcpy (char *s, const char *t) { return ::strcpy (s, t); }
+
+struct S { ~S (); };
+int foo ();
+
+int
+main ()
+{
+  S s;
+  int a;
+  char b[64];
+  N::strcpy (b, "ABCDEFGHIJKLM");
+  while ((a = foo ()) != -1)
+    if (a)
+      return -1;
+  return 0;
+}
--- gcc/testsuite/g++.dg/opt/pr17724-3.C.jj	2004-10-02 11:27:29.650919752 +0200
+++ gcc/testsuite/g++.dg/opt/pr17724-3.C	2004-10-02 11:29:13.933389195 +0200
@@ -0,0 +1,24 @@
+// PR tree-optimization/17724
+// { dg-do compile }
+// { dg-options "-O2" }
+
+extern "C" char *strcpy (char* d, const char* s) throw ();
+
+class A { public: A (); ~A (); };
+
+inline char * B (char *s, const char *t)
+{ return ::strcpy (s, t); }
+
+class C { int D (void); int E; };
+
+int C::D (void)
+{
+  A a;
+  try
+    {
+      char z[22];
+      if (this->E) B (z, "");
+      return 0;
+    }
+  catch (int &f) { return -1; }
+}
--- gcc/testsuite/g++.dg/opt/pr17724-5.C.jj	2004-10-02 11:27:29.000000000 +0200
+++ gcc/testsuite/g++.dg/opt/pr17724-5.C	2004-10-02 11:29:29.275662780 +0200
@@ -0,0 +1,24 @@
+// PR tree-optimization/17724
+// { dg-do compile }
+// { dg-options "-O2" }
+
+extern char *strcpy (char* d, const char* s) throw ();
+
+class A { public: A (); ~A (); };
+
+inline char * B (char *s, const char *t)
+{ return ::strcpy (s, t); }
+
+class C { int D (void); int E; };
+
+int C::D (void)
+{
+  A a;
+  try
+    {
+      char z[22];
+      if (this->E) B (z, "");
+      return 0;
+    }
+  catch (int &f) { return -1; }
+}


	Jakub

Re: [PATCH] Fix PR tree-optimization/17724 (take 2)

[Diego Novillo]

On Sun, 2004-10-10 at 10:56, Jakub Jelinek wrote:

> 	PR tree-optimization/17724
> 	* tree-cfg.c (tree_purge_dead_eh_edges): Free dominance info.
> 
Zdenek raised a good point re EH edges and dominance info.  Lets take
the safe approach for now.  This is OK.


Diego.

Re: [PATCH] Fix PR tree-optimization/17724 (take 2)

[Zdenek Dvorak]

Hello,

> On Sat, Oct 09, 2004 at 11:00:00AM +0200, Zdenek Dvorak wrote:
> > this does not seem to be the right fix to me:
> > 
> > 1) Change to delete_unreachable_blocks is not necessary.  As long as
> >    dominators are set correctly before delete_unreachable_blocks,
> >    they are also set correctly after it (since removal of unreachable
> >    blocks cannot affect any path from entry to basic block bb, and
> >    immediate dominator is determined from exactly these paths).
> 
> Are you sure that in all other cases where some basic blocks become
> unreachable code that removes the edge ensures that dominators are freed
> or recomputed for all basic blocks where idom(bb) could change?

not completely, of course.  Verify_dominators that is able to catch
such problems is run on several places, so I am fairly sure we update
dominators correctly most of the time, but of course there may be
some more buggy places.

Zdenek