Fix two more memory leaks in threader

Fix two more memory leaks in threader

[Jeff Law]

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


These fix the remaining leaks in the threader that I'm aware of.  We 
failed to properly clean-up when we had to cancel certain jump threading 
opportunities.  So thankfully this wasn't a big leak.

Bootstrapped and regression tested on x86_64-unknown-linux-gnu. 
Installed on the trunk.

Jeff

[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 1639 bytes --]

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index fe4dfc4..27435c6 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,9 @@
+2015-05-20  Jeff Law  <law@redhat.com>
+
+	* tree-ssa-threadupdate.c (mark_threaded_blocks): Properly
+	dispose of the jump thread path when the jump threading
+	opportunity is cancelled.
+
 2015-05-20  Manuel López-Ibáñez  <manu@gcc.gnu.org>
 
 	* diagnostic.c (diagnostic_print_caret_line): Fix off-by-one error
diff --git a/gcc/tree-ssa-threadupdate.c b/gcc/tree-ssa-threadupdate.c
index c5b78a4..4bccad0 100644
--- a/gcc/tree-ssa-threadupdate.c
+++ b/gcc/tree-ssa-threadupdate.c
@@ -2159,9 +2159,16 @@ mark_threaded_blocks (bitmap threaded_blocks)
         {
 	  /* Attach the path to the starting edge if none is yet recorded.  */
           if ((*path)[0]->e->aux == NULL)
-            (*path)[0]->e->aux = path;
-	  else if (dump_file && (dump_flags & TDF_DETAILS))
-	    dump_jump_thread_path (dump_file, *path, false);
+	    {
+              (*path)[0]->e->aux = path;
+	    }
+	  else
+	    {
+	      paths.unordered_remove (i);
+	      if (dump_file && (dump_flags & TDF_DETAILS))
+	        dump_jump_thread_path (dump_file, *path, false);
+	      delete_jump_thread_path (path);
+	    }
         }
     }
   /* Second, look for paths that have any other jump thread attached to
@@ -2185,8 +2192,10 @@ mark_threaded_blocks (bitmap threaded_blocks)
 	  else
 	    {
 	      e->aux = NULL;
+	      paths.unordered_remove (i);
 	      if (dump_file && (dump_flags & TDF_DETAILS))
 	        dump_jump_thread_path (dump_file, *path, false);
+	      delete_jump_thread_path (path);
 	    }
 	}
     }

Re: Fix two more memory leaks in threader

[Jakub Jelinek]

On Wed, May 20, 2015 at 10:36:25AM -0600, Jeff Law wrote:
> 
> These fix the remaining leaks in the threader that I'm aware of.  We failed
> to properly clean-up when we had to cancel certain jump threading
> opportunities.  So thankfully this wasn't a big leak.
> 
> Bootstrapped and regression tested on x86_64-unknown-linux-gnu. Installed on
> the trunk.
> 
> Jeff

> diff --git a/gcc/ChangeLog b/gcc/ChangeLog
> index fe4dfc4..27435c6 100644
> --- a/gcc/ChangeLog
> +++ b/gcc/ChangeLog
> @@ -1,3 +1,9 @@
> +2015-05-20  Jeff Law  <law@redhat.com>
> +
> +	* tree-ssa-threadupdate.c (mark_threaded_blocks): Properly
> +	dispose of the jump thread path when the jump threading
> +	opportunity is cancelled.
> +
>  2015-05-20  Manuel López-Ibáñez  <manu@gcc.gnu.org>
>  
>  	* diagnostic.c (diagnostic_print_caret_line): Fix off-by-one error
> diff --git a/gcc/tree-ssa-threadupdate.c b/gcc/tree-ssa-threadupdate.c
> index c5b78a4..4bccad0 100644
> --- a/gcc/tree-ssa-threadupdate.c
> +++ b/gcc/tree-ssa-threadupdate.c
> @@ -2159,9 +2159,16 @@ mark_threaded_blocks (bitmap threaded_blocks)
>          {
>  	  /* Attach the path to the starting edge if none is yet recorded.  */
>            if ((*path)[0]->e->aux == NULL)
> -            (*path)[0]->e->aux = path;
> -	  else if (dump_file && (dump_flags & TDF_DETAILS))
> -	    dump_jump_thread_path (dump_file, *path, false);
> +	    {
> +              (*path)[0]->e->aux = path;
> +	    }

Why the braces around single stmt if body?
Also, the indentation seems to be weird.

	Jakub

Re: Fix two more memory leaks in threader

[Jeff Law]

On 05/20/2015 10:41 AM, Jakub Jelinek wrote:
> On Wed, May 20, 2015 at 10:36:25AM -0600, Jeff Law wrote:
>>
>> These fix the remaining leaks in the threader that I'm aware of.  We failed
>> to properly clean-up when we had to cancel certain jump threading
>> opportunities.  So thankfully this wasn't a big leak.
>>
>> Bootstrapped and regression tested on x86_64-unknown-linux-gnu. Installed on
>> the trunk.
>>
>> Jeff
>
>> diff --git a/gcc/ChangeLog b/gcc/ChangeLog
>> index fe4dfc4..27435c6 100644
>> --- a/gcc/ChangeLog
>> +++ b/gcc/ChangeLog
>> @@ -1,3 +1,9 @@
>> +2015-05-20  Jeff Law  <law@redhat.com>
>> +
>> +	* tree-ssa-threadupdate.c (mark_threaded_blocks): Properly
>> +	dispose of the jump thread path when the jump threading
>> +	opportunity is cancelled.
>> +
>>   2015-05-20  Manuel López-Ibáñez  <manu@gcc.gnu.org>
>>
>>   	* diagnostic.c (diagnostic_print_caret_line): Fix off-by-one error
>> diff --git a/gcc/tree-ssa-threadupdate.c b/gcc/tree-ssa-threadupdate.c
>> index c5b78a4..4bccad0 100644
>> --- a/gcc/tree-ssa-threadupdate.c
>> +++ b/gcc/tree-ssa-threadupdate.c
>> @@ -2159,9 +2159,16 @@ mark_threaded_blocks (bitmap threaded_blocks)
>>           {
>>   	  /* Attach the path to the starting edge if none is yet recorded.  */
>>             if ((*path)[0]->e->aux == NULL)
>> -            (*path)[0]->e->aux = path;
>> -	  else if (dump_file && (dump_flags & TDF_DETAILS))
>> -	    dump_jump_thread_path (dump_file, *path, false);
>> +	    {
>> +              (*path)[0]->e->aux = path;
>> +	    }
>
> Why the braces around single stmt if body?
To match what was happening in the else clause.  I always find

if (fubar)
   something
else
   {
     more stuff
     even more stuff
   }

more painful to parse because of the mis-matched indentation than

if (fubar)
   {
     something
   }
else
   {
     more stuff
     even more stuff
   }

It's purely a style thing and if you'd prefer not to have the extra 
braces, I wouldn't lose any sleep over removing them.

> Also, the indentation seems to be weird.
Looks like spaces vs tabs issue.  I'll do a pass over 
tree-ssa-threadedge.c and fix them all up at once.

jeff

Re: Fix two more memory leaks in threader

[James Greenhalgh]

On Wed, May 20, 2015 at 05:36:25PM +0100, Jeff Law wrote:
> 
> These fix the remaining leaks in the threader that I'm aware of.  We 
> failed to properly clean-up when we had to cancel certain jump threading 
> opportunities.  So thankfully this wasn't a big leak.

Hi Jeff,

I don't have a reduced testcase to go on, but by inspection this patch
looks wrong to me... 

> diff --git a/gcc/tree-ssa-threadupdate.c b/gcc/tree-ssa-threadupdate.c
> index c5b78a4..4bccad0 100644
> --- a/gcc/tree-ssa-threadupdate.c
> +++ b/gcc/tree-ssa-threadupdate.c
> @@ -2159,9 +2159,16 @@ mark_threaded_blocks (bitmap threaded_blocks)
>          {
>  	  /* Attach the path to the starting edge if none is yet recorded.  */
>            if ((*path)[0]->e->aux == NULL)
> -            (*path)[0]->e->aux = path;
> -	  else if (dump_file && (dump_flags & TDF_DETAILS))
> -	    dump_jump_thread_path (dump_file, *path, false);
> +	    {
> +              (*path)[0]->e->aux = path;
> +	    }
> +	  else
> +	    {
> +	      paths.unordered_remove (i);

Here we are part-way through iterating through PATHS. With unordered_remove
we are going to move the end element of the vector to position 'i'. We'll
then move on and look at 'i + 1' and so never look at the element we just
moved.

This manifests as a lower number of cancelled jump-threads, and in
turn some extra jumps threaded - some of which may no longer be safe.

For a particular workload we've talked about before in relation to
jump-threading, dom1 ends up cancelling and threading these edges with
your patch applied:

  Cancelling jump thread: (28, 32) incoming edge;  (32, 36) joiner;  (36, 61) normal;
  Cancelling jump thread: (31, 7) incoming edge;  (7, 92) joiner;  (92, 8) normal;
  Cancelling jump thread: (63, 39) incoming edge;  (39, 89) joiner;  (89, 40) normal;
  Cancelling jump thread: (67, 68) incoming edge;  (68, 69) joiner;  (69, 93) normal;
  Cancelling jump thread: (4, 32) incoming edge;  (32, 36) joiner;  (36, 64) normal;
  Threaded jump 30 --> 28 to 299
  Threaded jump 91 --> 28 to 300
  Threaded jump 35 --> 36 to 302
  Threaded jump 88 --> 60 to 305
  Threaded jump 62 --> 60 to 306
  Threaded jump 32 --> 36 to 304

Reverting the patch we get these edges cancelled and threaded:

  Cancelling jump thread: (28, 32) incoming edge;  (32, 36) joiner;  (36, 61) normal;
  Cancelling jump thread: (31, 7) incoming edge;  (7, 92) joiner;  (92, 8) normal;
  Cancelling jump thread: (63, 39) incoming edge;  (39, 89) joiner;  (89, 40) normal;
  Cancelling jump thread: (67, 68) incoming edge;  (68, 69) joiner;  (69, 93) normal;
  Cancelling jump thread: (4, 32) incoming edge;  (32, 36) joiner;  (36, 64) normal;
  Cancelling jump thread: (4, 29) incoming edge;  (29, 91) joiner;  (91, 30) normal; (30, 31) nocopy;
  Cancelling jump thread: (32, 36) incoming edge;  (36, 64) joiner;  (64, 68) normal;
  Cancelling jump thread: (36, 61) incoming edge;  (61, 88) joiner;  (88, 62) normal; (62, 63) nocopy;
  Cancelling jump thread: (64, 68) incoming edge;  (68, 69) joiner;  (69, 93) normal;
  Threaded jump 30 --> 28 to 299
  Threaded jump 91 --> 28 to 300
  Threaded jump 35 --> 36 to 302
  Threaded jump 88 --> 60 to 303
  Threaded jump 62 --> 60 to 304

Note the extra thread of 32 --> 36 to 304 with this patch applied.

I think we either want to defer the unordered_remove until we're done
processing all the vector elements, or make sure to look at element 'i'
again after we've moved something new in to it.

A testcase would need to expose at least two threads which we later want
to cancel, one of which ends up at the end of the vector of threading
opportunities. We should then see only the first of those threads
actually get cancelled, and the second skipped over. Reproducing these
conditions is quite tough, which has stopped me finding a useful example
for the list, I'll be sure to follow-up this thread if I do get to one.

> +	      if (dump_file && (dump_flags & TDF_DETAILS))
> +	        dump_jump_thread_path (dump_file, *path, false);
> +	      delete_jump_thread_path (path);
> +	    }
>          }
>      }
>    /* Second, look for paths that have any other jump thread attached to
> @@ -2185,8 +2192,10 @@ mark_threaded_blocks (bitmap threaded_blocks)
>  	  else
>  	    {
>  	      e->aux = NULL;
> +	      paths.unordered_remove (i);

Likewise here.

Thanks,
James

>  	      if (dump_file && (dump_flags & TDF_DETAILS))
>  	        dump_jump_thread_path (dump_file, *path, false);
> +	      delete_jump_thread_path (path);
>  	    }
>  	}
>      }

Re: Fix two more memory leaks in threader

[Marek Polacek]

On Mon, Jul 20, 2015 at 03:19:06PM +0100, James Greenhalgh wrote:
> On Wed, May 20, 2015 at 05:36:25PM +0100, Jeff Law wrote:
> > 
> > These fix the remaining leaks in the threader that I'm aware of.  We 
> > failed to properly clean-up when we had to cancel certain jump threading 
> > opportunities.  So thankfully this wasn't a big leak.
> 
> Hi Jeff,
> 
> I don't have a reduced testcase to go on, but by inspection this patch
> looks wrong to me... 
> 
> > diff --git a/gcc/tree-ssa-threadupdate.c b/gcc/tree-ssa-threadupdate.c
> > index c5b78a4..4bccad0 100644
> > --- a/gcc/tree-ssa-threadupdate.c
> > +++ b/gcc/tree-ssa-threadupdate.c
> > @@ -2159,9 +2159,16 @@ mark_threaded_blocks (bitmap threaded_blocks)
> >          {
> >  	  /* Attach the path to the starting edge if none is yet recorded.  */
> >            if ((*path)[0]->e->aux == NULL)
> > -            (*path)[0]->e->aux = path;
> > -	  else if (dump_file && (dump_flags & TDF_DETAILS))
> > -	    dump_jump_thread_path (dump_file, *path, false);
> > +	    {
> > +              (*path)[0]->e->aux = path;
> > +	    }
> > +	  else
> > +	    {
> > +	      paths.unordered_remove (i);
> 
> Here we are part-way through iterating through PATHS. With unordered_remove
> we are going to move the end element of the vector to position 'i'. We'll
> then move on and look at 'i + 1' and so never look at the element we just
> moved.
> 
> This manifests as a lower number of cancelled jump-threads, and in
> turn some extra jumps threaded - some of which may no longer be safe.
> 
> For a particular workload we've talked about before in relation to
> jump-threading, dom1 ends up cancelling and threading these edges with
> your patch applied:
> 
>   Cancelling jump thread: (28, 32) incoming edge;  (32, 36) joiner;  (36, 61) normal;
>   Cancelling jump thread: (31, 7) incoming edge;  (7, 92) joiner;  (92, 8) normal;
>   Cancelling jump thread: (63, 39) incoming edge;  (39, 89) joiner;  (89, 40) normal;
>   Cancelling jump thread: (67, 68) incoming edge;  (68, 69) joiner;  (69, 93) normal;
>   Cancelling jump thread: (4, 32) incoming edge;  (32, 36) joiner;  (36, 64) normal;
>   Threaded jump 30 --> 28 to 299
>   Threaded jump 91 --> 28 to 300
>   Threaded jump 35 --> 36 to 302
>   Threaded jump 88 --> 60 to 305
>   Threaded jump 62 --> 60 to 306
>   Threaded jump 32 --> 36 to 304
> 
> Reverting the patch we get these edges cancelled and threaded:
> 
>   Cancelling jump thread: (28, 32) incoming edge;  (32, 36) joiner;  (36, 61) normal;
>   Cancelling jump thread: (31, 7) incoming edge;  (7, 92) joiner;  (92, 8) normal;
>   Cancelling jump thread: (63, 39) incoming edge;  (39, 89) joiner;  (89, 40) normal;
>   Cancelling jump thread: (67, 68) incoming edge;  (68, 69) joiner;  (69, 93) normal;
>   Cancelling jump thread: (4, 32) incoming edge;  (32, 36) joiner;  (36, 64) normal;
>   Cancelling jump thread: (4, 29) incoming edge;  (29, 91) joiner;  (91, 30) normal; (30, 31) nocopy;
>   Cancelling jump thread: (32, 36) incoming edge;  (36, 64) joiner;  (64, 68) normal;
>   Cancelling jump thread: (36, 61) incoming edge;  (61, 88) joiner;  (88, 62) normal; (62, 63) nocopy;
>   Cancelling jump thread: (64, 68) incoming edge;  (68, 69) joiner;  (69, 93) normal;
>   Threaded jump 30 --> 28 to 299
>   Threaded jump 91 --> 28 to 300
>   Threaded jump 35 --> 36 to 302
>   Threaded jump 88 --> 60 to 303
>   Threaded jump 62 --> 60 to 304
> 
> Note the extra thread of 32 --> 36 to 304 with this patch applied.
> 
> I think we either want to defer the unordered_remove until we're done
> processing all the vector elements, or make sure to look at element 'i'
> again after we've moved something new in to it.
> 
> A testcase would need to expose at least two threads which we later want
> to cancel, one of which ends up at the end of the vector of threading
> opportunities. We should then see only the first of those threads
> actually get cancelled, and the second skipped over. Reproducing these
> conditions is quite tough, which has stopped me finding a useful example
> for the list, I'll be sure to follow-up this thread if I do get to one.
 
Yes, there's something wrong about this patch, see PR66372.
I guess what you wrote above is the problem in this PR.

	Marek

Re: Fix two more memory leaks in threader

[Jeff Law]

On 07/20/2015 08:19 AM, James Greenhalgh wrote:
> On Wed, May 20, 2015 at 05:36:25PM +0100, Jeff Law wrote:
>>
>> These fix the remaining leaks in the threader that I'm aware of.  We
>> failed to properly clean-up when we had to cancel certain jump threading
>> opportunities.  So thankfully this wasn't a big leak.
>
> Hi Jeff,
>
> I don't have a reduced testcase to go on, but by inspection this patch
> looks wrong to me...
>
>> diff --git a/gcc/tree-ssa-threadupdate.c b/gcc/tree-ssa-threadupdate.c
>> index c5b78a4..4bccad0 100644
>> --- a/gcc/tree-ssa-threadupdate.c
>> +++ b/gcc/tree-ssa-threadupdate.c
>> @@ -2159,9 +2159,16 @@ mark_threaded_blocks (bitmap threaded_blocks)
>>           {
>>   	  /* Attach the path to the starting edge if none is yet recorded.  */
>>             if ((*path)[0]->e->aux == NULL)
>> -            (*path)[0]->e->aux = path;
>> -	  else if (dump_file && (dump_flags & TDF_DETAILS))
>> -	    dump_jump_thread_path (dump_file, *path, false);
>> +	    {
>> +              (*path)[0]->e->aux = path;
>> +	    }
>> +	  else
>> +	    {
>> +	      paths.unordered_remove (i);
>
> Here we are part-way through iterating through PATHS. With unordered_remove
> we are going to move the end element of the vector to position 'i'. We'll
> then move on and look at 'i + 1' and so never look at the element we just
> moved.
>
> This manifests as a lower number of cancelled jump-threads, and in
> turn some extra jumps threaded - some of which may no longer be safe.
>
> For a particular workload we've talked about before in relation to
> jump-threading, dom1 ends up cancelling and threading these edges with
> your patch applied:
>
>    Cancelling jump thread: (28, 32) incoming edge;  (32, 36) joiner;  (36, 61) normal;
>    Cancelling jump thread: (31, 7) incoming edge;  (7, 92) joiner;  (92, 8) normal;
>    Cancelling jump thread: (63, 39) incoming edge;  (39, 89) joiner;  (89, 40) normal;
>    Cancelling jump thread: (67, 68) incoming edge;  (68, 69) joiner;  (69, 93) normal;
>    Cancelling jump thread: (4, 32) incoming edge;  (32, 36) joiner;  (36, 64) normal;
>    Threaded jump 30 --> 28 to 299
>    Threaded jump 91 --> 28 to 300
>    Threaded jump 35 --> 36 to 302
>    Threaded jump 88 --> 60 to 305
>    Threaded jump 62 --> 60 to 306
>    Threaded jump 32 --> 36 to 304
>
> Reverting the patch we get these edges cancelled and threaded:
>
>    Cancelling jump thread: (28, 32) incoming edge;  (32, 36) joiner;  (36, 61) normal;
>    Cancelling jump thread: (31, 7) incoming edge;  (7, 92) joiner;  (92, 8) normal;
>    Cancelling jump thread: (63, 39) incoming edge;  (39, 89) joiner;  (89, 40) normal;
>    Cancelling jump thread: (67, 68) incoming edge;  (68, 69) joiner;  (69, 93) normal;
>    Cancelling jump thread: (4, 32) incoming edge;  (32, 36) joiner;  (36, 64) normal;
>    Cancelling jump thread: (4, 29) incoming edge;  (29, 91) joiner;  (91, 30) normal; (30, 31) nocopy;
>    Cancelling jump thread: (32, 36) incoming edge;  (36, 64) joiner;  (64, 68) normal;
>    Cancelling jump thread: (36, 61) incoming edge;  (61, 88) joiner;  (88, 62) normal; (62, 63) nocopy;
>    Cancelling jump thread: (64, 68) incoming edge;  (68, 69) joiner;  (69, 93) normal;
>    Threaded jump 30 --> 28 to 299
>    Threaded jump 91 --> 28 to 300
>    Threaded jump 35 --> 36 to 302
>    Threaded jump 88 --> 60 to 303
>    Threaded jump 62 --> 60 to 304
>
> Note the extra thread of 32 --> 36 to 304 with this patch applied.
>
> I think we either want to defer the unordered_remove until we're done
> processing all the vector elements, or make sure to look at element 'i'
> again after we've moved something new in to it.
>
> A testcase would need to expose at least two threads which we later want
> to cancel, one of which ends up at the end of the vector of threading
> opportunities. We should then see only the first of those threads
> actually get cancelled, and the second skipped over. Reproducing these
> conditions is quite tough, which has stopped me finding a useful example
> for the list, I'll be sure to follow-up this thread if I do get to one.
I think you're right.  All the uses of unordered_remove ought to be 
audited.  There's also a bug in BZ which I believe has been bisected to 
this change.  Hopefully that'll have a reasonable testcase.

jeff

Re: Fix two more memory leaks in threader

[Jeff Law]

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

On 07/20/2015 08:19 AM, James Greenhalgh wrote:
>
> I think we either want to defer the unordered_remove until we're done
> processing all the vector elements, or make sure to look at element 'i'
> again after we've moved something new in to it.
Correct.  Two loops had this mistake -- while others got it right. 
Sigh.  The easiest fix is to change how we increment "i" in those loops 
so that it's only incremented if we do not delete the path.  That 
happens to also match how other cases are handled.

Thanks for catching these mistakes.

>
> A testcase would need to expose at least two threads which we later want
> to cancel, one of which ends up at the end of the vector of threading
> opportunities. We should then see only the first of those threads
> actually get cancelled, and the second skipped over. Reproducing these
> conditions is quite tough, which has stopped me finding a useful example
> for the list, I'll be sure to follow-up this thread if I do get to one.
Thankfully BZ had two closely related testcases.

Bootstrapped and regression tested on x86_64-unknown-linux-gnu. 
Installed on the trunk.

(Unfortunately it didn't resolve the ppc64 issue I was looking at ;(


Jeff

[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 4117 bytes --]

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index fb9d115..9abde9f 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,11 @@
+2015-08-03  Jeff Law  <law@redhat.com>
+
+	PR middle-end/66314
+	PR gcov-profile/66899
+	* tree-ssa-threadupdate.c (mark_threaded_blocks): Correctly
+	iterate over the jump threading paths when an element in the
+	jump threading paths array is eliminated.
+
 2015-08-03  Segher Boessenkool  <segher@kernel.crashing.org>
 
 	* Makefile.in (OBJS): Put gimple-match.o and generic-match.o first.
diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index a403767..0a841b5 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,10 @@
+2015-08-03  Jeff Law  <law@redhat.com>
+
+	PR middle-end/66314
+	PR gcov-profile/66899
+	* gcc.dg/pr66899.c: New test.
+	* gcc.dg/pr66314.c: New test.
+
 2015-08-03  Marek Polacek  <polacek@redhat.com>
 
 	PR c/67088
diff --git a/gcc/testsuite/gcc.dg/pr66314.c b/gcc/testsuite/gcc.dg/pr66314.c
new file mode 100644
index 0000000..f74ab5b
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr66314.c
@@ -0,0 +1,33 @@
+/* { dg-do compile } */
+/* { dg-options "-std=gnu89 -Os -fprofile-arcs -fsanitize=kernel-address" } */
+
+char *a;
+int d;
+
+static int
+fn1 (int b, int c)
+{
+  while (a)
+    if (*a)
+      return -126;
+  if (b)
+    return -12;
+  if (c == -12)
+    return c;
+}
+
+void
+fn2 (int b, int c)
+{
+  for (;;)
+    {
+      d = fn1 (b, c);
+      switch (d)
+        {
+        case -126:
+          continue;
+        default:
+          return;
+        }
+    }
+}
diff --git a/gcc/testsuite/gcc.dg/pr66899.c b/gcc/testsuite/gcc.dg/pr66899.c
new file mode 100644
index 0000000..1fff181
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr66899.c
@@ -0,0 +1,41 @@
+/* { dg-do compile } */
+/* { dg-options "-Os -fprofile-arcs" } */
+
+struct
+{
+  int authority;
+} * a, *b, c, d;
+int e, f;
+static int
+fn1 ()
+{
+  if (a)
+    goto verified;
+  if (b)
+    goto matched;
+  return -126;
+matched:
+  e = 0;
+verified:
+  if (b)
+    for (; &c != b; c = d)
+      ;
+  return 0;
+}
+
+int
+fn2 ()
+{
+  for (;;)
+    {
+      f = fn1 ();
+      switch (f)
+        {
+        case -126:
+          continue;
+        default:
+          return 0;
+        }
+    }
+}
+
diff --git a/gcc/tree-ssa-threadupdate.c b/gcc/tree-ssa-threadupdate.c
index 31ddf25..5a5f8df 100644
--- a/gcc/tree-ssa-threadupdate.c
+++ b/gcc/tree-ssa-threadupdate.c
@@ -2130,7 +2130,7 @@ mark_threaded_blocks (bitmap threaded_blocks)
      cases where the second path starts at a downstream edge on the same
      path).  First record all joiner paths, deleting any in the unexpected
      case where there is already a path for that incoming edge.  */
-  for (i = 0; i < paths.length (); i++)
+  for (i = 0; i < paths.length ();)
     {
       vec<jump_thread_edge *> *path = paths[i];
 
@@ -2140,6 +2140,7 @@ mark_threaded_blocks (bitmap threaded_blocks)
 	  if ((*path)[0]->e->aux == NULL)
 	    {
 	      (*path)[0]->e->aux = path;
+	      i++;
 	    }
 	  else
 	    {
@@ -2149,10 +2150,15 @@ mark_threaded_blocks (bitmap threaded_blocks)
 	      delete_jump_thread_path (path);
 	    }
 	}
+      else
+	{
+	  i++;
+	}
     }
+
   /* Second, look for paths that have any other jump thread attached to
      them, and either finish converting them or cancel them.  */
-  for (i = 0; i < paths.length (); i++)
+  for (i = 0; i < paths.length ();)
     {
       vec<jump_thread_edge *> *path = paths[i];
       edge e = (*path)[0]->e;
@@ -2167,7 +2173,10 @@ mark_threaded_blocks (bitmap threaded_blocks)
 	  /* If we iterated through the entire path without exiting the loop,
 	     then we are good to go, record it.  */
 	  if (j == path->length ())
-	    bitmap_set_bit (tmp, e->dest->index);
+	    {
+	      bitmap_set_bit (tmp, e->dest->index);
+	      i++;
+	    }
 	  else
 	    {
 	      e->aux = NULL;
@@ -2177,6 +2186,10 @@ mark_threaded_blocks (bitmap threaded_blocks)
 	      delete_jump_thread_path (path);
 	    }
 	}
+      else
+	{
+	  i++;
+	}
     }
 
   /* If optimizing for size, only thread through block if we don't have

Re: Fix two more memory leaks in threader

[Uros Bizjak]

Hello!

+2015-08-03  Jeff Law  <law@redhat.com>
+
+ PR middle-end/66314
+ PR gcov-profile/66899
+ * tree-ssa-threadupdate.c (mark_threaded_blocks): Correctly
+ iterate over the jump threading paths when an element in the
+ jump threading paths array is eliminated.
+
 2015-08-03  Segher Boessenkool  <segher@kernel.crashing.org>

  * Makefile.in (OBJS): Put gimple-match.o and generic-match.o first.
diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index a403767..0a841b5 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,10 @@
+2015-08-03  Jeff Law  <law@redhat.com>
+
+ PR middle-end/66314
+ PR gcov-profile/66899
+ * gcc.dg/pr66899.c: New test.
+ * gcc.dg/pr66314.c: New test.

gcc.dg/pr66314.c testcase should go into gcc.dg/asan directory.
Targets where -fsanitize=address or -fsanitize=kernel-address are not
supported now emit warning about unsupported feature for the mentioned
testcase.

Uros.

Re: Fix two more memory leaks in threader

[Jeff Law]

On 08/11/2015 12:16 AM, Uros Bizjak wrote:
> Hello!
>
> +2015-08-03  Jeff Law  <law@redhat.com>
> +
> + PR middle-end/66314
> + PR gcov-profile/66899
> + * tree-ssa-threadupdate.c (mark_threaded_blocks): Correctly
> + iterate over the jump threading paths when an element in the
> + jump threading paths array is eliminated.
> +
>   2015-08-03  Segher Boessenkool  <segher@kernel.crashing.org>
>
>    * Makefile.in (OBJS): Put gimple-match.o and generic-match.o first.
> diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
> index a403767..0a841b5 100644
> --- a/gcc/testsuite/ChangeLog
> +++ b/gcc/testsuite/ChangeLog
> @@ -1,3 +1,10 @@
> +2015-08-03  Jeff Law  <law@redhat.com>
> +
> + PR middle-end/66314
> + PR gcov-profile/66899
> + * gcc.dg/pr66899.c: New test.
> + * gcc.dg/pr66314.c: New test.
>
> gcc.dg/pr66314.c testcase should go into gcc.dg/asan directory.
> Targets where -fsanitize=address or -fsanitize=kernel-address are not
> supported now emit warning about unsupported feature for the mentioned
> testcase.
Yes.  It's on the TODO list -- I think Andreas pointed that out about a 
week ago.

Jeff