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.kulka...@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 <rguent...@suse.de> SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)