PR80613

[Richard Biener]

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)