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)
