On Fri, 5 May 2017, Prathamesh Kulkarni wrote: > On 5 May 2017 at 12:46, Richard Biener <rguent...@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.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. > 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 <rguent...@suse.de> > > SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB > > 21284 (AG Nuernberg) > -- Richard Biener <rguent...@suse.de> SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)