| Message ID | CAAgBjM=DUQ=mxxXscCzNXY1mJ=OsxP3yUAJBF6AZxrL6Zk6QwA@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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 >
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 > > >
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
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
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)
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;
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)
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. */
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.