PR80613

Message ID CAAgBjM=DUQ=mxxXscCzNXY1mJ=OsxP3yUAJBF6AZxrL6Zk6QwA@mail.gmail.com
State New
Headers show

Commit Message

Prathamesh Kulkarni May 4, 2017, 4 p.m.
Hi,
As mentioned in PR, the issue is that cddce1 marks the call to
__builtin_strdup as necessary:
marking necessary through .MEM_6 stmt p_7 = __builtin_strdup (&d);

and since p_7 doesn't get added to worklist in propagate_necessity()
because it's used only within free(), it's treated as "dead"
and wrongly gets released.
The patch fixes that by adding strdup/strndup in corresponding condition
in eliminate_unnecessary_stmts().

Another issue, was that my previous patch failed to remove multiple
calls to strdup:
char *f(char **tt)
{
  char *t = *tt;
  char *p;

  p = __builtin_strdup (t);
  p = __builtin_strdup (t);
  return p;
}

That's fixed in patch by adding strdup/strndup to another
corresponding condition in propagate_necessity() so that only one
instance of strdup would be kept.

Bootstrapped+tested on x86_64-unknown-linux-gnu.
Cross-testing on arm*-*-* and aarch64*-*-* in progress.
OK to commit if testing passes ?

Thanks
Prathamesh
2017-05-04  Prathamesh Kulkarni  <prathamesh.kulkarni@linaro.org>

	PR tree-optimization/80613
	* tree-ssa-dce.c (propagate_necessity): Add cases for BUILT_IN_STRDUP
	and BUILT_IN_STRNDUP.
	* (eliminate_unnecessary_stmts): Likewise.

testsuite/
	* gcc.dg/tree-ssa/pr80613-1.c: New test-case.
	* gcc.dg/tree-ssa/pr80613-2.c: New test-case.

Comments

Martin Sebor May 4, 2017, 5:52 p.m. | #1
On 05/04/2017 10:00 AM, Prathamesh Kulkarni wrote:
> Hi,

> As mentioned in PR, the issue is that cddce1 marks the call to

> __builtin_strdup as necessary:

> marking necessary through .MEM_6 stmt p_7 = __builtin_strdup (&d);

>

> and since p_7 doesn't get added to worklist in propagate_necessity()

> because it's used only within free(), it's treated as "dead"

> and wrongly gets released.

> The patch fixes that by adding strdup/strndup in corresponding condition

> in eliminate_unnecessary_stmts().

>

> Another issue, was that my previous patch failed to remove multiple

> calls to strdup:

> char *f(char **tt)

> {

>   char *t = *tt;

>   char *p;

>

>   p = __builtin_strdup (t);

>   p = __builtin_strdup (t);

>   return p;


Since this is clearly a bug in the program -- the first result
leaks -- would it make sense to issue a warning before removing
the duplicate call?  (It would be nice to issue a warning not
just for strdup but also for other memory allocation functions,
so perhaps that should be a separate enhancement request.)

Martin

> }

>

> That's fixed in patch by adding strdup/strndup to another

> corresponding condition in propagate_necessity() so that only one

> instance of strdup would be kept.

>

> Bootstrapped+tested on x86_64-unknown-linux-gnu.

> Cross-testing on arm*-*-* and aarch64*-*-* in progress.

> OK to commit if testing passes ?

>

> Thanks

> Prathamesh

>
Trevor Saunders May 4, 2017, 6:19 p.m. | #2
On Thu, May 04, 2017 at 11:52:31AM -0600, Martin Sebor wrote:
> On 05/04/2017 10:00 AM, Prathamesh Kulkarni wrote:

> > Hi,

> > As mentioned in PR, the issue is that cddce1 marks the call to

> > __builtin_strdup as necessary:

> > marking necessary through .MEM_6 stmt p_7 = __builtin_strdup (&d);

> > 

> > and since p_7 doesn't get added to worklist in propagate_necessity()

> > because it's used only within free(), it's treated as "dead"

> > and wrongly gets released.

> > The patch fixes that by adding strdup/strndup in corresponding condition


so, I think it doesn't actually matter since we can completely remove
the strdup(), but in this casewe could also replace the strdup() with
calloc(1, 1) though I'm not sure it would ever happen in practice.  The
reasoning is that accessing (&d)[] for any x other than 0 would be
invalid.  if (&d)[0] aka d is not '\0' then strdup() will access
(&d)[1].  Therefore we can infer that d is 0, and so the strdup() must
allocate one byte whose value is 0.

> > in eliminate_unnecessary_stmts().

> > 

> > Another issue, was that my previous patch failed to remove multiple

> > calls to strdup:

> > char *f(char **tt)

> > {

> >   char *t = *tt;

> >   char *p;

> > 

> >   p = __builtin_strdup (t);

> >   p = __builtin_strdup (t);

> >   return p;

> 

> Since this is clearly a bug in the program -- the first result

> leaks -- would it make sense to issue a warning before removing

> the duplicate call?  (It would be nice to issue a warning not

> just for strdup but also for other memory allocation functions,

> so perhaps that should be a separate enhancement request.)


I'm actually planning on looking at this this weekend after meaning to
for a while.  My main goal was to catch places where unique_ptr could be
used, but I think some leak detection can live in the same place.

Trev

> 

> Martin

> 

> > }

> > 

> > That's fixed in patch by adding strdup/strndup to another

> > corresponding condition in propagate_necessity() so that only one

> > instance of strdup would be kept.

> > 

> > Bootstrapped+tested on x86_64-unknown-linux-gnu.

> > Cross-testing on arm*-*-* and aarch64*-*-* in progress.

> > OK to commit if testing passes ?

> > 

> > Thanks

> > Prathamesh

> > 

>
Jeff Law May 4, 2017, 6:39 p.m. | #3
On 05/04/2017 11:52 AM, Martin Sebor wrote:
> On 05/04/2017 10:00 AM, Prathamesh Kulkarni wrote:

>> Hi,

>> As mentioned in PR, the issue is that cddce1 marks the call to

>> __builtin_strdup as necessary:

>> marking necessary through .MEM_6 stmt p_7 = __builtin_strdup (&d);

>>

>> and since p_7 doesn't get added to worklist in propagate_necessity()

>> because it's used only within free(), it's treated as "dead"

>> and wrongly gets released.

>> The patch fixes that by adding strdup/strndup in corresponding condition

>> in eliminate_unnecessary_stmts().

>>

>> Another issue, was that my previous patch failed to remove multiple

>> calls to strdup:

>> char *f(char **tt)

>> {

>>   char *t = *tt;

>>   char *p;

>>

>>   p = __builtin_strdup (t);

>>   p = __builtin_strdup (t);

>>   return p;

> 

> Since this is clearly a bug in the program -- the first result

> leaks -- would it make sense to issue a warning before removing

> the duplicate call?  (It would be nice to issue a warning not

> just for strdup but also for other memory allocation functions,

> so perhaps that should be a separate enhancement request.)

Seems like it should be a separate enhancement request.

jeff
Jeff Law May 4, 2017, 7:03 p.m. | #4
On 05/04/2017 10:00 AM, Prathamesh Kulkarni wrote:
> Hi,

> As mentioned in PR, the issue is that cddce1 marks the call to

> __builtin_strdup as necessary:

> marking necessary through .MEM_6 stmt p_7 = __builtin_strdup (&d);

> 

> and since p_7 doesn't get added to worklist in propagate_necessity()

> because it's used only within free(), it's treated as "dead"

> and wrongly gets released.

> The patch fixes that by adding strdup/strndup in corresponding condition

> in eliminate_unnecessary_stmts().

> 

> Another issue, was that my previous patch failed to remove multiple

> calls to strdup:

> char *f(char **tt)

> {

>    char *t = *tt;

>    char *p;

> 

>    p = __builtin_strdup (t);

>    p = __builtin_strdup (t);

>    return p;

> }

> 

> That's fixed in patch by adding strdup/strndup to another

> corresponding condition in propagate_necessity() so that only one

> instance of strdup would be kept.

> 

> Bootstrapped+tested on x86_64-unknown-linux-gnu.

> Cross-testing on arm*-*-* and aarch64*-*-* in progress.

> OK to commit if testing passes ?

> 

> Thanks

> Prathamesh

> 

> 

> pr80613-1.txt

> 

> 

> 2017-05-04  Prathamesh Kulkarni<prathamesh.kulkarni@linaro.org>

> 

> 	PR tree-optimization/80613

> 	* tree-ssa-dce.c (propagate_necessity): Add cases for BUILT_IN_STRDUP

> 	and BUILT_IN_STRNDUP.

> 	* (eliminate_unnecessary_stmts): Likewise.

> 

> testsuite/

> 	* gcc.dg/tree-ssa/pr80613-1.c: New test-case.

> 	* gcc.dg/tree-ssa/pr80613-2.c: New test-case.

So I'm comfortable with the change to eliminate_unnecessary_stmts as 
well as the associated testcase pr80613-1.c.  GIven that addresses the 
core of the bug, I'd go ahead and install that part immediately.

I'm still trying to understand the code in propagate_necessity.



> 

> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr80613-1.c b/gcc/testsuite/gcc.dg/tree-ssa/pr80613-1.c

> new file mode 100644

> index 00000000000..56176427922

> --- /dev/null

> +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr80613-1.c

> @@ -0,0 +1,13 @@

> +/* { dg-do compile } */

> +/* { dg-options "-O2" } */

> +

> +char *a(int);

> +int b;

> +

> +void c() {

> +  for (;;) {

> +    char d = *a(b);

> +    char *e = __builtin_strdup (&d);

> +    __builtin_free(e);

> +  }

> +}

> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr80613-2.c b/gcc/testsuite/gcc.dg/tree-ssa/pr80613-2.c

> new file mode 100644

> index 00000000000..c58cc08d6c5

> --- /dev/null

> +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr80613-2.c

> @@ -0,0 +1,16 @@

> +/* { dg-do compile } */

> +/* { dg-options "-O2 -fdump-tree-cddce1" } */

> +

> +/* There should only be one instance of __builtin_strdup after cddce1.  */

> +

> +char *f(char **tt)

> +{

> +  char *t = *tt;

> +  char *p;

> +

> +  p = __builtin_strdup (t);

> +  p = __builtin_strdup (t);

> +  return p;

> +}

> +

> +/* { dg-final { scan-tree-dump-times "__builtin_strdup" 1 "cddce1" } } */

> diff --git a/gcc/tree-ssa-dce.c b/gcc/tree-ssa-dce.c

> index e17659df91f..7c05f981307 100644

> --- a/gcc/tree-ssa-dce.c

> +++ b/gcc/tree-ssa-dce.c

> @@ -852,7 +852,9 @@ propagate_necessity (bool aggressive)

>   			  == BUILT_IN_ALLOCA_WITH_ALIGN)

>   		      || DECL_FUNCTION_CODE (callee) == BUILT_IN_STACK_SAVE

>   		      || DECL_FUNCTION_CODE (callee) == BUILT_IN_STACK_RESTORE

> -		      || DECL_FUNCTION_CODE (callee) == BUILT_IN_ASSUME_ALIGNED))

> +		      || DECL_FUNCTION_CODE (callee) == BUILT_IN_ASSUME_ALIGNED

> +		      || DECL_FUNCTION_CODE (callee) == BUILT_IN_STRDUP

> +		      || DECL_FUNCTION_CODE (callee) == BUILT_IN_STRNDUP))

>   		continue;

What I'm struggling with is that str[n]dup read from the memory pointed 
to by their incoming argument, so ISTM they are not "merely acting are 
barriers or that only store to memory" and thus shouldn't be treated 
like memset, malloc & friends.  Otherwise couldn't we end up incorrectly 
removing a store to memory that is only read by a live strdup?

So while I agree you ought to be able to remove the first call to strdup 
in the second testcase, I'm not sure the approach you've used works 
correctly.

jeff
Richard Biener May 5, 2017, 7:16 a.m. | #5
On Thu, 4 May 2017, Jeff Law wrote:

> On 05/04/2017 10:00 AM, Prathamesh Kulkarni wrote:

> > Hi,

> > As mentioned in PR, the issue is that cddce1 marks the call to

> > __builtin_strdup as necessary:

> > marking necessary through .MEM_6 stmt p_7 = __builtin_strdup (&d);

> > 

> > and since p_7 doesn't get added to worklist in propagate_necessity()

> > because it's used only within free(), it's treated as "dead"

> > and wrongly gets released.

> > The patch fixes that by adding strdup/strndup in corresponding condition

> > in eliminate_unnecessary_stmts().

> > 

> > Another issue, was that my previous patch failed to remove multiple

> > calls to strdup:

> > char *f(char **tt)

> > {

> >    char *t = *tt;

> >    char *p;

> > 

> >    p = __builtin_strdup (t);

> >    p = __builtin_strdup (t);

> >    return p;

> > }

> > 

> > That's fixed in patch by adding strdup/strndup to another

> > corresponding condition in propagate_necessity() so that only one

> > instance of strdup would be kept.

> > 

> > Bootstrapped+tested on x86_64-unknown-linux-gnu.

> > Cross-testing on arm*-*-* and aarch64*-*-* in progress.

> > OK to commit if testing passes ?

> > 

> > Thanks

> > Prathamesh

> > 

> > 

> > pr80613-1.txt

> > 

> > 

> > 2017-05-04  Prathamesh Kulkarni<prathamesh.kulkarni@linaro.org>

> > 

> > 	PR tree-optimization/80613

> > 	* tree-ssa-dce.c (propagate_necessity): Add cases for BUILT_IN_STRDUP

> > 	and BUILT_IN_STRNDUP.

> > 	* (eliminate_unnecessary_stmts): Likewise.

> > 

> > testsuite/

> > 	* gcc.dg/tree-ssa/pr80613-1.c: New test-case.

> > 	* gcc.dg/tree-ssa/pr80613-2.c: New test-case.

> So I'm comfortable with the change to eliminate_unnecessary_stmts as well as

> the associated testcase pr80613-1.c.  GIven that addresses the core of the

> bug, I'd go ahead and install that part immediately.

> 

> I'm still trying to understand the code in propagate_necessity.


That part of the patch is clearly wrong unless compensation code is
added elsehwere.

I think adding str[n]dup within the existing mechanism to remove
allocate/free pairs was wrong given str[n]dup have a use and there's
no code in DCE that can compensate for str[n]dup only becoming
necessary late.

I don't see how such compenstation code would work reliably without
becoming too gross (re-start iteration).

So I think the best is to revert the initial patch and look for a
pattern-matching approach instead.

Thanks,
Richard.



> 

> 

> > 

> > diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr80613-1.c

> > b/gcc/testsuite/gcc.dg/tree-ssa/pr80613-1.c

> > new file mode 100644

> > index 00000000000..56176427922

> > --- /dev/null

> > +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr80613-1.c

> > @@ -0,0 +1,13 @@

> > +/* { dg-do compile } */

> > +/* { dg-options "-O2" } */

> > +

> > +char *a(int);

> > +int b;

> > +

> > +void c() {

> > +  for (;;) {

> > +    char d = *a(b);

> > +    char *e = __builtin_strdup (&d);

> > +    __builtin_free(e);

> > +  }

> > +}

> > diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr80613-2.c

> > b/gcc/testsuite/gcc.dg/tree-ssa/pr80613-2.c

> > new file mode 100644

> > index 00000000000..c58cc08d6c5

> > --- /dev/null

> > +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr80613-2.c

> > @@ -0,0 +1,16 @@

> > +/* { dg-do compile } */

> > +/* { dg-options "-O2 -fdump-tree-cddce1" } */

> > +

> > +/* There should only be one instance of __builtin_strdup after cddce1.  */

> > +

> > +char *f(char **tt)

> > +{

> > +  char *t = *tt;

> > +  char *p;

> > +

> > +  p = __builtin_strdup (t);

> > +  p = __builtin_strdup (t);

> > +  return p;

> > +}

> > +

> > +/* { dg-final { scan-tree-dump-times "__builtin_strdup" 1 "cddce1" } } */

> > diff --git a/gcc/tree-ssa-dce.c b/gcc/tree-ssa-dce.c

> > index e17659df91f..7c05f981307 100644

> > --- a/gcc/tree-ssa-dce.c

> > +++ b/gcc/tree-ssa-dce.c

> > @@ -852,7 +852,9 @@ propagate_necessity (bool aggressive)

> >   			  == BUILT_IN_ALLOCA_WITH_ALIGN)

> >   		      || DECL_FUNCTION_CODE (callee) == BUILT_IN_STACK_SAVE

> >   		      || DECL_FUNCTION_CODE (callee) == BUILT_IN_STACK_RESTORE

> > -		      || DECL_FUNCTION_CODE (callee) ==

> > BUILT_IN_ASSUME_ALIGNED))

> > +		      || DECL_FUNCTION_CODE (callee) ==

> > BUILT_IN_ASSUME_ALIGNED

> > +		      || DECL_FUNCTION_CODE (callee) == BUILT_IN_STRDUP

> > +		      || DECL_FUNCTION_CODE (callee) == BUILT_IN_STRNDUP))

> >   		continue;

> What I'm struggling with is that str[n]dup read from the memory pointed to by

> their incoming argument, so ISTM they are not "merely acting are barriers or

> that only store to memory" and thus shouldn't be treated like memset, malloc &

> friends.  Otherwise couldn't we end up incorrectly removing a store to memory

> that is only read by a live strdup?

> 

> So while I agree you ought to be able to remove the first call to strdup in

> the second testcase, I'm not sure the approach you've used works correctly.

> 

> jeff

> 

> 

> 


-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)
Prathamesh Kulkarni May 5, 2017, 9:05 a.m. | #6
On 5 May 2017 at 12:46, Richard Biener <rguenther@suse.de> wrote:
> On Thu, 4 May 2017, Jeff Law wrote:

>

>> On 05/04/2017 10:00 AM, Prathamesh Kulkarni wrote:

>> > Hi,

>> > As mentioned in PR, the issue is that cddce1 marks the call to

>> > __builtin_strdup as necessary:

>> > marking necessary through .MEM_6 stmt p_7 = __builtin_strdup (&d);

>> >

>> > and since p_7 doesn't get added to worklist in propagate_necessity()

>> > because it's used only within free(), it's treated as "dead"

>> > and wrongly gets released.

>> > The patch fixes that by adding strdup/strndup in corresponding condition

>> > in eliminate_unnecessary_stmts().

>> >

>> > Another issue, was that my previous patch failed to remove multiple

>> > calls to strdup:

>> > char *f(char **tt)

>> > {

>> >    char *t = *tt;

>> >    char *p;

>> >

>> >    p = __builtin_strdup (t);

>> >    p = __builtin_strdup (t);

>> >    return p;

>> > }

>> >

>> > That's fixed in patch by adding strdup/strndup to another

>> > corresponding condition in propagate_necessity() so that only one

>> > instance of strdup would be kept.

>> >

>> > Bootstrapped+tested on x86_64-unknown-linux-gnu.

>> > Cross-testing on arm*-*-* and aarch64*-*-* in progress.

>> > OK to commit if testing passes ?

>> >

>> > Thanks

>> > Prathamesh

>> >

>> >

>> > pr80613-1.txt

>> >

>> >

>> > 2017-05-04  Prathamesh Kulkarni<prathamesh.kulkarni@linaro.org>

>> >

>> >     PR tree-optimization/80613

>> >     * tree-ssa-dce.c (propagate_necessity): Add cases for BUILT_IN_STRDUP

>> >     and BUILT_IN_STRNDUP.

>> >     * (eliminate_unnecessary_stmts): Likewise.

>> >

>> > testsuite/

>> >     * gcc.dg/tree-ssa/pr80613-1.c: New test-case.

>> >     * gcc.dg/tree-ssa/pr80613-2.c: New test-case.

>> So I'm comfortable with the change to eliminate_unnecessary_stmts as well as

>> the associated testcase pr80613-1.c.  GIven that addresses the core of the

>> bug, I'd go ahead and install that part immediately.

>>

>> I'm still trying to understand the code in propagate_necessity.

>

> That part of the patch is clearly wrong unless compensation code is

> added elsehwere.

>

> I think adding str[n]dup within the existing mechanism to remove

> allocate/free pairs was wrong given str[n]dup have a use and there's

> no code in DCE that can compensate for str[n]dup only becoming

> necessary late.

>

> I don't see how such compenstation code would work reliably without

> becoming too gross (re-start iteration).

>

> So I think the best is to revert the initial patch and look for a

> pattern-matching approach instead.

Hi Richard,
The attached patch removes str[n]dup in propagate_necessity() for
allocation/free pair
removal.
I assume it'd be OK to leave str[n]dup in
mark_stmt_if_obviously_necessary(), so dce
removes calls to strn[n]dup if lhs is dead (or not present) ?

Thanks,
Prathamesh
>

> Thanks,

> Richard.

>

>

>

>>

>>

>> >

>> > diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr80613-1.c

>> > b/gcc/testsuite/gcc.dg/tree-ssa/pr80613-1.c

>> > new file mode 100644

>> > index 00000000000..56176427922

>> > --- /dev/null

>> > +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr80613-1.c

>> > @@ -0,0 +1,13 @@

>> > +/* { dg-do compile } */

>> > +/* { dg-options "-O2" } */

>> > +

>> > +char *a(int);

>> > +int b;

>> > +

>> > +void c() {

>> > +  for (;;) {

>> > +    char d = *a(b);

>> > +    char *e = __builtin_strdup (&d);

>> > +    __builtin_free(e);

>> > +  }

>> > +}

>> > diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr80613-2.c

>> > b/gcc/testsuite/gcc.dg/tree-ssa/pr80613-2.c

>> > new file mode 100644

>> > index 00000000000..c58cc08d6c5

>> > --- /dev/null

>> > +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr80613-2.c

>> > @@ -0,0 +1,16 @@

>> > +/* { dg-do compile } */

>> > +/* { dg-options "-O2 -fdump-tree-cddce1" } */

>> > +

>> > +/* There should only be one instance of __builtin_strdup after cddce1.  */

>> > +

>> > +char *f(char **tt)

>> > +{

>> > +  char *t = *tt;

>> > +  char *p;

>> > +

>> > +  p = __builtin_strdup (t);

>> > +  p = __builtin_strdup (t);

>> > +  return p;

>> > +}

>> > +

>> > +/* { dg-final { scan-tree-dump-times "__builtin_strdup" 1 "cddce1" } } */

>> > diff --git a/gcc/tree-ssa-dce.c b/gcc/tree-ssa-dce.c

>> > index e17659df91f..7c05f981307 100644

>> > --- a/gcc/tree-ssa-dce.c

>> > +++ b/gcc/tree-ssa-dce.c

>> > @@ -852,7 +852,9 @@ propagate_necessity (bool aggressive)

>> >                       == BUILT_IN_ALLOCA_WITH_ALIGN)

>> >                   || DECL_FUNCTION_CODE (callee) == BUILT_IN_STACK_SAVE

>> >                   || DECL_FUNCTION_CODE (callee) == BUILT_IN_STACK_RESTORE

>> > -                 || DECL_FUNCTION_CODE (callee) ==

>> > BUILT_IN_ASSUME_ALIGNED))

>> > +                 || DECL_FUNCTION_CODE (callee) ==

>> > BUILT_IN_ASSUME_ALIGNED

>> > +                 || DECL_FUNCTION_CODE (callee) == BUILT_IN_STRDUP

>> > +                 || DECL_FUNCTION_CODE (callee) == BUILT_IN_STRNDUP))

>> >             continue;

>> What I'm struggling with is that str[n]dup read from the memory pointed to by

>> their incoming argument, so ISTM they are not "merely acting are barriers or

>> that only store to memory" and thus shouldn't be treated like memset, malloc &

>> friends.  Otherwise couldn't we end up incorrectly removing a store to memory

>> that is only read by a live strdup?

>>

>> So while I agree you ought to be able to remove the first call to strdup in

>> the second testcase, I'm not sure the approach you've used works correctly.

>>

>> jeff

>>

>>

>>

>

> --

> Richard Biener <rguenther@suse.de>

> SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)
2017-05-05  Prathamesh Kulkarni  <prathamesh.kulkarni@linaro.org>

	PR tree-optimization/80613
	* tree-ssa-dce.c (propagate_necessity): Remove cases for
	BUILT_IN_STRDUP and BUILT_IN_STRNDUP.

testsuite/
	* gcc.dg/tree-ssa/pr79697.c (k): Remove.diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr79697.c b/gcc/testsuite/gcc.dg/tree-ssa/pr79697.c
index d4f64739787..fc3580e3ce3 100644
--- a/gcc/testsuite/gcc.dg/tree-ssa/pr79697.c
+++ b/gcc/testsuite/gcc.dg/tree-ssa/pr79697.c
@@ -16,18 +16,6 @@ void h(void)
   __builtin_realloc (0, 10);
 }
 
-void k(void)
-{
-  char *p = __builtin_strdup ("abc");
-  __builtin_free (p);
-
-  char *q = __builtin_strndup ("abc", 3);
-  __builtin_free (q);
-}
-
 /* { dg-final { scan-tree-dump "Deleting : __builtin_strdup" "cddce1" } } */
 /* { dg-final { scan-tree-dump "Deleting : __builtin_strndup" "cddce1" } } */
 /* { dg-final { scan-tree-dump "__builtin_malloc" "gimple" } } */
-/* { dg-final { scan-tree-dump-not "__builtin_strdup" "optimized" } } */
-/* { dg-final { scan-tree-dump-not "__builtin_strndup" "optimized" } } */
-/* { dg-final { scan-tree-dump-not "__builtin_free" "optimized" } } */
diff --git a/gcc/tree-ssa-dce.c b/gcc/tree-ssa-dce.c
index e17659df91f..4225c3cc1a1 100644
--- a/gcc/tree-ssa-dce.c
+++ b/gcc/tree-ssa-dce.c
@@ -782,9 +782,7 @@ propagate_necessity (bool aggressive)
 		  && DECL_BUILT_IN_CLASS (def_callee) == BUILT_IN_NORMAL
 		  && (DECL_FUNCTION_CODE (def_callee) == BUILT_IN_ALIGNED_ALLOC
 		      || DECL_FUNCTION_CODE (def_callee) == BUILT_IN_MALLOC
-		      || DECL_FUNCTION_CODE (def_callee) == BUILT_IN_CALLOC
-		      || DECL_FUNCTION_CODE (def_callee) == BUILT_IN_STRDUP
-		      || DECL_FUNCTION_CODE (def_callee) == BUILT_IN_STRNDUP))
+		      || DECL_FUNCTION_CODE (def_callee) == BUILT_IN_CALLOC))
 		{
 		  gimple *bounds_def_stmt;
 		  tree bounds;

Richard Biener May 5, 2017, 9:16 a.m. | #7
On Fri, 5 May 2017, Prathamesh Kulkarni wrote:

> On 5 May 2017 at 12:46, Richard Biener <rguenther@suse.de> wrote:

> > On Thu, 4 May 2017, Jeff Law wrote:

> >

> >> On 05/04/2017 10:00 AM, Prathamesh Kulkarni wrote:

> >> > Hi,

> >> > As mentioned in PR, the issue is that cddce1 marks the call to

> >> > __builtin_strdup as necessary:

> >> > marking necessary through .MEM_6 stmt p_7 = __builtin_strdup (&d);

> >> >

> >> > and since p_7 doesn't get added to worklist in propagate_necessity()

> >> > because it's used only within free(), it's treated as "dead"

> >> > and wrongly gets released.

> >> > The patch fixes that by adding strdup/strndup in corresponding condition

> >> > in eliminate_unnecessary_stmts().

> >> >

> >> > Another issue, was that my previous patch failed to remove multiple

> >> > calls to strdup:

> >> > char *f(char **tt)

> >> > {

> >> >    char *t = *tt;

> >> >    char *p;

> >> >

> >> >    p = __builtin_strdup (t);

> >> >    p = __builtin_strdup (t);

> >> >    return p;

> >> > }

> >> >

> >> > That's fixed in patch by adding strdup/strndup to another

> >> > corresponding condition in propagate_necessity() so that only one

> >> > instance of strdup would be kept.

> >> >

> >> > Bootstrapped+tested on x86_64-unknown-linux-gnu.

> >> > Cross-testing on arm*-*-* and aarch64*-*-* in progress.

> >> > OK to commit if testing passes ?

> >> >

> >> > Thanks

> >> > Prathamesh

> >> >

> >> >

> >> > pr80613-1.txt

> >> >

> >> >

> >> > 2017-05-04  Prathamesh Kulkarni<prathamesh.kulkarni@linaro.org>

> >> >

> >> >     PR tree-optimization/80613

> >> >     * tree-ssa-dce.c (propagate_necessity): Add cases for BUILT_IN_STRDUP

> >> >     and BUILT_IN_STRNDUP.

> >> >     * (eliminate_unnecessary_stmts): Likewise.

> >> >

> >> > testsuite/

> >> >     * gcc.dg/tree-ssa/pr80613-1.c: New test-case.

> >> >     * gcc.dg/tree-ssa/pr80613-2.c: New test-case.

> >> So I'm comfortable with the change to eliminate_unnecessary_stmts as well as

> >> the associated testcase pr80613-1.c.  GIven that addresses the core of the

> >> bug, I'd go ahead and install that part immediately.

> >>

> >> I'm still trying to understand the code in propagate_necessity.

> >

> > That part of the patch is clearly wrong unless compensation code is

> > added elsehwere.

> >

> > I think adding str[n]dup within the existing mechanism to remove

> > allocate/free pairs was wrong given str[n]dup have a use and there's

> > no code in DCE that can compensate for str[n]dup only becoming

> > necessary late.

> >

> > I don't see how such compenstation code would work reliably without

> > becoming too gross (re-start iteration).

> >

> > So I think the best is to revert the initial patch and look for a

> > pattern-matching approach instead.

> Hi Richard,

> The attached patch removes str[n]dup in propagate_necessity() for

> allocation/free pair

> removal.

> I assume it'd be OK to leave str[n]dup in

> mark_stmt_if_obviously_necessary(), so dce

> removes calls to strn[n]dup if lhs is dead (or not present) ?


Ok, so I revisited the DCE code and I think your original fix is
fine if you exclude the propagate_necessity hunk.

Thanks,
Richard.

> Thanks,

> Prathamesh

> >

> > Thanks,

> > Richard.

> >

> >

> >

> >>

> >>

> >> >

> >> > diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr80613-1.c

> >> > b/gcc/testsuite/gcc.dg/tree-ssa/pr80613-1.c

> >> > new file mode 100644

> >> > index 00000000000..56176427922

> >> > --- /dev/null

> >> > +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr80613-1.c

> >> > @@ -0,0 +1,13 @@

> >> > +/* { dg-do compile } */

> >> > +/* { dg-options "-O2" } */

> >> > +

> >> > +char *a(int);

> >> > +int b;

> >> > +

> >> > +void c() {

> >> > +  for (;;) {

> >> > +    char d = *a(b);

> >> > +    char *e = __builtin_strdup (&d);

> >> > +    __builtin_free(e);

> >> > +  }

> >> > +}

> >> > diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr80613-2.c

> >> > b/gcc/testsuite/gcc.dg/tree-ssa/pr80613-2.c

> >> > new file mode 100644

> >> > index 00000000000..c58cc08d6c5

> >> > --- /dev/null

> >> > +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr80613-2.c

> >> > @@ -0,0 +1,16 @@

> >> > +/* { dg-do compile } */

> >> > +/* { dg-options "-O2 -fdump-tree-cddce1" } */

> >> > +

> >> > +/* There should only be one instance of __builtin_strdup after cddce1.  */

> >> > +

> >> > +char *f(char **tt)

> >> > +{

> >> > +  char *t = *tt;

> >> > +  char *p;

> >> > +

> >> > +  p = __builtin_strdup (t);

> >> > +  p = __builtin_strdup (t);

> >> > +  return p;

> >> > +}

> >> > +

> >> > +/* { dg-final { scan-tree-dump-times "__builtin_strdup" 1 "cddce1" } } */

> >> > diff --git a/gcc/tree-ssa-dce.c b/gcc/tree-ssa-dce.c

> >> > index e17659df91f..7c05f981307 100644

> >> > --- a/gcc/tree-ssa-dce.c

> >> > +++ b/gcc/tree-ssa-dce.c

> >> > @@ -852,7 +852,9 @@ propagate_necessity (bool aggressive)

> >> >                       == BUILT_IN_ALLOCA_WITH_ALIGN)

> >> >                   || DECL_FUNCTION_CODE (callee) == BUILT_IN_STACK_SAVE

> >> >                   || DECL_FUNCTION_CODE (callee) == BUILT_IN_STACK_RESTORE

> >> > -                 || DECL_FUNCTION_CODE (callee) ==

> >> > BUILT_IN_ASSUME_ALIGNED))

> >> > +                 || DECL_FUNCTION_CODE (callee) ==

> >> > BUILT_IN_ASSUME_ALIGNED

> >> > +                 || DECL_FUNCTION_CODE (callee) == BUILT_IN_STRDUP

> >> > +                 || DECL_FUNCTION_CODE (callee) == BUILT_IN_STRNDUP))

> >> >             continue;

> >> What I'm struggling with is that str[n]dup read from the memory pointed to by

> >> their incoming argument, so ISTM they are not "merely acting are barriers or

> >> that only store to memory" and thus shouldn't be treated like memset, malloc &

> >> friends.  Otherwise couldn't we end up incorrectly removing a store to memory

> >> that is only read by a live strdup?

> >>

> >> So while I agree you ought to be able to remove the first call to strdup in

> >> the second testcase, I'm not sure the approach you've used works correctly.

> >>

> >> jeff

> >>

> >>

> >>

> >

> > --

> > Richard Biener <rguenther@suse.de>

> > SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)

> 


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

Patch hide | download patch | download mbox

diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr80613-1.c b/gcc/testsuite/gcc.dg/tree-ssa/pr80613-1.c
new file mode 100644
index 00000000000..56176427922
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/pr80613-1.c
@@ -0,0 +1,13 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+char *a(int);
+int b;
+
+void c() {
+  for (;;) {
+    char d = *a(b);
+    char *e = __builtin_strdup (&d);
+    __builtin_free(e);
+  }
+}
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr80613-2.c b/gcc/testsuite/gcc.dg/tree-ssa/pr80613-2.c
new file mode 100644
index 00000000000..c58cc08d6c5
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/pr80613-2.c
@@ -0,0 +1,16 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-tree-cddce1" } */
+
+/* There should only be one instance of __builtin_strdup after cddce1.  */
+
+char *f(char **tt)
+{
+  char *t = *tt;
+  char *p;
+
+  p = __builtin_strdup (t);
+  p = __builtin_strdup (t);
+  return p;
+}
+
+/* { dg-final { scan-tree-dump-times "__builtin_strdup" 1 "cddce1" } } */
diff --git a/gcc/tree-ssa-dce.c b/gcc/tree-ssa-dce.c
index e17659df91f..7c05f981307 100644
--- a/gcc/tree-ssa-dce.c
+++ b/gcc/tree-ssa-dce.c
@@ -852,7 +852,9 @@  propagate_necessity (bool aggressive)
 			  == BUILT_IN_ALLOCA_WITH_ALIGN)
 		      || DECL_FUNCTION_CODE (callee) == BUILT_IN_STACK_SAVE
 		      || DECL_FUNCTION_CODE (callee) == BUILT_IN_STACK_RESTORE
-		      || DECL_FUNCTION_CODE (callee) == BUILT_IN_ASSUME_ALIGNED))
+		      || DECL_FUNCTION_CODE (callee) == BUILT_IN_ASSUME_ALIGNED
+		      || DECL_FUNCTION_CODE (callee) == BUILT_IN_STRDUP
+		      || DECL_FUNCTION_CODE (callee) == BUILT_IN_STRNDUP))
 		continue;
 
 	      /* Calls implicitly load from memory, their arguments
@@ -1353,6 +1355,8 @@  eliminate_unnecessary_stmts (void)
 			  && DECL_FUNCTION_CODE (call) != BUILT_IN_MALLOC
 			  && DECL_FUNCTION_CODE (call) != BUILT_IN_CALLOC
 			  && DECL_FUNCTION_CODE (call) != BUILT_IN_ALLOCA
+			  && DECL_FUNCTION_CODE (call) != BUILT_IN_STRDUP
+			  && DECL_FUNCTION_CODE (call) != BUILT_IN_STRNDUP
 			  && (DECL_FUNCTION_CODE (call)
 			      != BUILT_IN_ALLOCA_WITH_ALIGN)))
 		  /* Avoid doing so for bndret calls for the same reason.  */