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) ?
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)
2017-05-05 Prathamesh Kulkarni <prathamesh.kulka...@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;
