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.
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
