Re: [PATCH] Fix PR/63841: empty constructor doesn't zero-initialize
Here is the new patch. Bootstrapped and tested on
x86_64-unknown-linux-gnu. OK for trunk?
Thanks,
Teresa
2014-11-13tejohn...@google.com
gcc:
PR tree-optimization/63841
* tree.c (initializer_zerop): A constructor with no elements
does not zero initialize.
gcc/testsuite:
PR tree-optimization/63841
* g++.dg/tree-ssa/pr63841.C: New test.
Index: tree.c
===
--- tree.c (revision 217190)
+++ tree.c (working copy)
@@ -10330,6 +10330,8 @@ initializer_zerop (const_tree init)
{
unsigned HOST_WIDE_INT idx;
+if (TREE_CLOBBER_P (init))
+ return false;
FOR_EACH_CONSTRUCTOR_VALUE (CONSTRUCTOR_ELTS (init), idx, elt)
if (!initializer_zerop (elt))
return false;
Index: testsuite/g++.dg/tree-ssa/pr63841.C
===
--- testsuite/g++.dg/tree-ssa/pr63841.C (revision 0)
+++ testsuite/g++.dg/tree-ssa/pr63841.C (working copy)
@@ -0,0 +1,38 @@
+/* { dg-do run } */
+/* { dg-options -O2 } */
+
+#include cstdio
+#include string
+
+std::string __attribute__ ((noinline)) comp_test_write() {
+ std::string data;
+
+ for (int i = 0; i 2; ++i) {
+char b = 1 (i * 8);
+data.append(b, 1);
+ }
+
+ return data;
+}
+
+std::string __attribute__ ((noinline)) comp_test_write_good() {
+ std::string data;
+
+ char b;
+ for (int i = 0; i 2; ++i) {
+b = 1 (i * 8);
+data.append(b, 1);
+ }
+
+ return data;
+}
+
+int main() {
+ std::string good = comp_test_write_good();
+ printf(expected: %hx\n, *(short*)good.c_str());
+
+ std::string bad = comp_test_write();
+ printf(got: %hx\n, *(short*)bad.c_str());
+
+ return good != bad;
+}
On Wed, Nov 12, 2014 at 9:40 PM, Andrew Pinski pins...@gmail.com wrote:
On Wed, Nov 12, 2014 at 9:38 PM, Teresa Johnson tejohn...@google.com wrote:
On Wed, Nov 12, 2014 at 9:30 PM, Andrew Pinski pins...@gmail.com wrote:
On Wed, Nov 12, 2014 at 9:25 PM, Teresa Johnson tejohn...@google.com
wrote:
Added testcase. Here is the new patch:
2014-11-12tejohn...@google.com
gcc:
PR tree-optimization/63841
* tree.c (initializer_zerop): A constructor with no elements
does not zero initialize.
Actually an empty constructor does zero initialize. A clobber does not.
Ok, thanks, I wasn't sure.
The check you want is TREE_CLOBBER_P instead.
So in initializer_zerop, a constructor that is a TREE_CLOBBER_P would
return false, right? I will make that change and retest.
Yes that is correct. Note TREE_CLOBBER_P already checks if it is an
constructor.
Thanks,
Andrew
Thanks,
Teresa
Thanks,
Andrew
gcc/testsuite:
* g++.dg/tree-ssa/pr63841.C: New test.
Index: tree.c
===
--- tree.c (revision 217190)
+++ tree.c (working copy)
@@ -10330,6 +10330,8 @@ initializer_zerop (const_tree init)
{
unsigned HOST_WIDE_INT idx;
+if (!CONSTRUCTOR_NELTS (init))
+ return false;
FOR_EACH_CONSTRUCTOR_VALUE (CONSTRUCTOR_ELTS (init), idx, elt)
if (!initializer_zerop (elt))
return false;
Index: testsuite/g++.dg/tree-ssa/pr63841.C
===
--- testsuite/g++.dg/tree-ssa/pr63841.C (revision 0)
+++ testsuite/g++.dg/tree-ssa/pr63841.C (working copy)
@@ -0,0 +1,38 @@
+/* { dg-do run } */
+/* { dg-options -O2 } */
+
+#include cstdio
+#include string
+
+std::string __attribute__ ((noinline)) comp_test_write() {
+ std::string data;
+
+ for (int i = 0; i 2; ++i) {
+char b = 1 (i * 8);
+data.append(b, 1);
+ }
+
+ return data;
+}
+
+std::string __attribute__ ((noinline)) comp_test_write_good() {
+ std::string data;
+
+ char b;
+ for (int i = 0; i 2; ++i) {
+b = 1 (i * 8);
+data.append(b, 1);
+ }
+
+ return data;
+}
+
+int main() {
+ std::string good = comp_test_write_good();
+ printf(expected: %hx\n, *(short*)good.c_str());
+
+ std::string bad = comp_test_write();
+ printf(got: %hx\n, *(short*)bad.c_str());
+
+ return good != bad;
+}
On Wed, Nov 12, 2014 at 2:17 PM, Xinliang David Li davi...@google.com
wrote:
missing test case?
David
On Wed, Nov 12, 2014 at 2:13 PM, Teresa Johnson tejohn...@google.com
wrote:
This patch fixes an issue where tree-strlen was incorrectly removing a
store of 0 into a string because it thought a prior CLOBBER (which is
an empty constructor with no elements) was zero-initializing the
string.
Bootstrapped and tested on x86_64-unknown-linux-gnu. OK for trunk?
Thanks,
Teresa
2014-11-12tejohn...@google.com
PR tree-optimization/63841
* tree.c (initializer_zerop): A constructor with no elements
does not zero initialize.
Index: tree.c
Re: [PATCH] Fix PR/63841: empty constructor doesn't zero-initialize
On Thu, Nov 13, 2014 at 3:20 PM, Teresa Johnson tejohn...@google.com wrote:
Here is the new patch. Bootstrapped and tested on
x86_64-unknown-linux-gnu. OK for trunk?
Ok for trunk and branches.
Thanks,
Richard.
Thanks,
Teresa
2014-11-13tejohn...@google.com
gcc:
PR tree-optimization/63841
* tree.c (initializer_zerop): A constructor with no elements
does not zero initialize.
gcc/testsuite:
PR tree-optimization/63841
* g++.dg/tree-ssa/pr63841.C: New test.
Index: tree.c
===
--- tree.c (revision 217190)
+++ tree.c (working copy)
@@ -10330,6 +10330,8 @@ initializer_zerop (const_tree init)
{
unsigned HOST_WIDE_INT idx;
+if (TREE_CLOBBER_P (init))
+ return false;
FOR_EACH_CONSTRUCTOR_VALUE (CONSTRUCTOR_ELTS (init), idx, elt)
if (!initializer_zerop (elt))
return false;
Index: testsuite/g++.dg/tree-ssa/pr63841.C
===
--- testsuite/g++.dg/tree-ssa/pr63841.C (revision 0)
+++ testsuite/g++.dg/tree-ssa/pr63841.C (working copy)
@@ -0,0 +1,38 @@
+/* { dg-do run } */
+/* { dg-options -O2 } */
+
+#include cstdio
+#include string
+
+std::string __attribute__ ((noinline)) comp_test_write() {
+ std::string data;
+
+ for (int i = 0; i 2; ++i) {
+char b = 1 (i * 8);
+data.append(b, 1);
+ }
+
+ return data;
+}
+
+std::string __attribute__ ((noinline)) comp_test_write_good() {
+ std::string data;
+
+ char b;
+ for (int i = 0; i 2; ++i) {
+b = 1 (i * 8);
+data.append(b, 1);
+ }
+
+ return data;
+}
+
+int main() {
+ std::string good = comp_test_write_good();
+ printf(expected: %hx\n, *(short*)good.c_str());
+
+ std::string bad = comp_test_write();
+ printf(got: %hx\n, *(short*)bad.c_str());
+
+ return good != bad;
+}
On Wed, Nov 12, 2014 at 9:40 PM, Andrew Pinski pins...@gmail.com wrote:
On Wed, Nov 12, 2014 at 9:38 PM, Teresa Johnson tejohn...@google.com wrote:
On Wed, Nov 12, 2014 at 9:30 PM, Andrew Pinski pins...@gmail.com wrote:
On Wed, Nov 12, 2014 at 9:25 PM, Teresa Johnson tejohn...@google.com
wrote:
Added testcase. Here is the new patch:
2014-11-12tejohn...@google.com
gcc:
PR tree-optimization/63841
* tree.c (initializer_zerop): A constructor with no elements
does not zero initialize.
Actually an empty constructor does zero initialize. A clobber does not.
Ok, thanks, I wasn't sure.
The check you want is TREE_CLOBBER_P instead.
So in initializer_zerop, a constructor that is a TREE_CLOBBER_P would
return false, right? I will make that change and retest.
Yes that is correct. Note TREE_CLOBBER_P already checks if it is an
constructor.
Thanks,
Andrew
Thanks,
Teresa
Thanks,
Andrew
gcc/testsuite:
* g++.dg/tree-ssa/pr63841.C: New test.
Index: tree.c
===
--- tree.c (revision 217190)
+++ tree.c (working copy)
@@ -10330,6 +10330,8 @@ initializer_zerop (const_tree init)
{
unsigned HOST_WIDE_INT idx;
+if (!CONSTRUCTOR_NELTS (init))
+ return false;
FOR_EACH_CONSTRUCTOR_VALUE (CONSTRUCTOR_ELTS (init), idx, elt)
if (!initializer_zerop (elt))
return false;
Index: testsuite/g++.dg/tree-ssa/pr63841.C
===
--- testsuite/g++.dg/tree-ssa/pr63841.C (revision 0)
+++ testsuite/g++.dg/tree-ssa/pr63841.C (working copy)
@@ -0,0 +1,38 @@
+/* { dg-do run } */
+/* { dg-options -O2 } */
+
+#include cstdio
+#include string
+
+std::string __attribute__ ((noinline)) comp_test_write() {
+ std::string data;
+
+ for (int i = 0; i 2; ++i) {
+char b = 1 (i * 8);
+data.append(b, 1);
+ }
+
+ return data;
+}
+
+std::string __attribute__ ((noinline)) comp_test_write_good() {
+ std::string data;
+
+ char b;
+ for (int i = 0; i 2; ++i) {
+b = 1 (i * 8);
+data.append(b, 1);
+ }
+
+ return data;
+}
+
+int main() {
+ std::string good = comp_test_write_good();
+ printf(expected: %hx\n, *(short*)good.c_str());
+
+ std::string bad = comp_test_write();
+ printf(got: %hx\n, *(short*)bad.c_str());
+
+ return good != bad;
+}
On Wed, Nov 12, 2014 at 2:17 PM, Xinliang David Li davi...@google.com
wrote:
missing test case?
David
On Wed, Nov 12, 2014 at 2:13 PM, Teresa Johnson tejohn...@google.com
wrote:
This patch fixes an issue where tree-strlen was incorrectly removing a
store of 0 into a string because it thought a prior CLOBBER (which is
an empty constructor with no elements) was zero-initializing the
string.
Bootstrapped and tested on x86_64-unknown-linux-gnu. OK for trunk?
Thanks,
Teresa
2014-11-12tejohn...@google.com
PR
Re: [PATCH] Fix PR/63841: empty constructor doesn't zero-initialize
On Thu, Nov 13, 2014 at 3:32 PM, Richard Biener
richard.guent...@gmail.com wrote:
On Thu, Nov 13, 2014 at 3:20 PM, Teresa Johnson tejohn...@google.com wrote:
Here is the new patch. Bootstrapped and tested on
x86_64-unknown-linux-gnu. OK for trunk?
Ok for trunk and branches.
Err - please fix the changelog entry wording to A clobber does
not zero inintiaize
Thanks,
Richard.
Thanks,
Teresa
2014-11-13tejohn...@google.com
gcc:
PR tree-optimization/63841
* tree.c (initializer_zerop): A constructor with no elements
does not zero initialize.
gcc/testsuite:
PR tree-optimization/63841
* g++.dg/tree-ssa/pr63841.C: New test.
Index: tree.c
===
--- tree.c (revision 217190)
+++ tree.c (working copy)
@@ -10330,6 +10330,8 @@ initializer_zerop (const_tree init)
{
unsigned HOST_WIDE_INT idx;
+if (TREE_CLOBBER_P (init))
+ return false;
FOR_EACH_CONSTRUCTOR_VALUE (CONSTRUCTOR_ELTS (init), idx, elt)
if (!initializer_zerop (elt))
return false;
Index: testsuite/g++.dg/tree-ssa/pr63841.C
===
--- testsuite/g++.dg/tree-ssa/pr63841.C (revision 0)
+++ testsuite/g++.dg/tree-ssa/pr63841.C (working copy)
@@ -0,0 +1,38 @@
+/* { dg-do run } */
+/* { dg-options -O2 } */
+
+#include cstdio
+#include string
+
+std::string __attribute__ ((noinline)) comp_test_write() {
+ std::string data;
+
+ for (int i = 0; i 2; ++i) {
+char b = 1 (i * 8);
+data.append(b, 1);
+ }
+
+ return data;
+}
+
+std::string __attribute__ ((noinline)) comp_test_write_good() {
+ std::string data;
+
+ char b;
+ for (int i = 0; i 2; ++i) {
+b = 1 (i * 8);
+data.append(b, 1);
+ }
+
+ return data;
+}
+
+int main() {
+ std::string good = comp_test_write_good();
+ printf(expected: %hx\n, *(short*)good.c_str());
+
+ std::string bad = comp_test_write();
+ printf(got: %hx\n, *(short*)bad.c_str());
+
+ return good != bad;
+}
On Wed, Nov 12, 2014 at 9:40 PM, Andrew Pinski pins...@gmail.com wrote:
On Wed, Nov 12, 2014 at 9:38 PM, Teresa Johnson tejohn...@google.com
wrote:
On Wed, Nov 12, 2014 at 9:30 PM, Andrew Pinski pins...@gmail.com wrote:
On Wed, Nov 12, 2014 at 9:25 PM, Teresa Johnson tejohn...@google.com
wrote:
Added testcase. Here is the new patch:
2014-11-12tejohn...@google.com
gcc:
PR tree-optimization/63841
* tree.c (initializer_zerop): A constructor with no elements
does not zero initialize.
Actually an empty constructor does zero initialize. A clobber does not.
Ok, thanks, I wasn't sure.
The check you want is TREE_CLOBBER_P instead.
So in initializer_zerop, a constructor that is a TREE_CLOBBER_P would
return false, right? I will make that change and retest.
Yes that is correct. Note TREE_CLOBBER_P already checks if it is an
constructor.
Thanks,
Andrew
Thanks,
Teresa
Thanks,
Andrew
gcc/testsuite:
* g++.dg/tree-ssa/pr63841.C: New test.
Index: tree.c
===
--- tree.c (revision 217190)
+++ tree.c (working copy)
@@ -10330,6 +10330,8 @@ initializer_zerop (const_tree init)
{
unsigned HOST_WIDE_INT idx;
+if (!CONSTRUCTOR_NELTS (init))
+ return false;
FOR_EACH_CONSTRUCTOR_VALUE (CONSTRUCTOR_ELTS (init), idx, elt)
if (!initializer_zerop (elt))
return false;
Index: testsuite/g++.dg/tree-ssa/pr63841.C
===
--- testsuite/g++.dg/tree-ssa/pr63841.C (revision 0)
+++ testsuite/g++.dg/tree-ssa/pr63841.C (working copy)
@@ -0,0 +1,38 @@
+/* { dg-do run } */
+/* { dg-options -O2 } */
+
+#include cstdio
+#include string
+
+std::string __attribute__ ((noinline)) comp_test_write() {
+ std::string data;
+
+ for (int i = 0; i 2; ++i) {
+char b = 1 (i * 8);
+data.append(b, 1);
+ }
+
+ return data;
+}
+
+std::string __attribute__ ((noinline)) comp_test_write_good() {
+ std::string data;
+
+ char b;
+ for (int i = 0; i 2; ++i) {
+b = 1 (i * 8);
+data.append(b, 1);
+ }
+
+ return data;
+}
+
+int main() {
+ std::string good = comp_test_write_good();
+ printf(expected: %hx\n, *(short*)good.c_str());
+
+ std::string bad = comp_test_write();
+ printf(got: %hx\n, *(short*)bad.c_str());
+
+ return good != bad;
+}
On Wed, Nov 12, 2014 at 2:17 PM, Xinliang David Li davi...@google.com
wrote:
missing test case?
David
On Wed, Nov 12, 2014 at 2:13 PM, Teresa Johnson tejohn...@google.com
wrote:
This patch fixes an issue where tree-strlen was incorrectly removing a
store of 0 into a string because it thought a prior CLOBBER (which is
an empty constructor with no elements) was
Re: [PATCH] Fix PR/63841: empty constructor doesn't zero-initialize
On Thu, Nov 13, 2014 at 6:32 AM, Richard Biener
richard.guent...@gmail.com wrote:
On Thu, Nov 13, 2014 at 3:32 PM, Richard Biener
richard.guent...@gmail.com wrote:
On Thu, Nov 13, 2014 at 3:20 PM, Teresa Johnson tejohn...@google.com wrote:
Here is the new patch. Bootstrapped and tested on
x86_64-unknown-linux-gnu. OK for trunk?
Ok for trunk and branches.
Err - please fix the changelog entry wording to A clobber does
not zero inintiaize
Thanks for catching that - copied from the original patch.
Will commit to trunk now then to gcc-4_9 after regression testing with
the branch.
Thanks,
Teresa
Thanks,
Richard.
Thanks,
Teresa
2014-11-13tejohn...@google.com
gcc:
PR tree-optimization/63841
* tree.c (initializer_zerop): A constructor with no elements
does not zero initialize.
gcc/testsuite:
PR tree-optimization/63841
* g++.dg/tree-ssa/pr63841.C: New test.
Index: tree.c
===
--- tree.c (revision 217190)
+++ tree.c (working copy)
@@ -10330,6 +10330,8 @@ initializer_zerop (const_tree init)
{
unsigned HOST_WIDE_INT idx;
+if (TREE_CLOBBER_P (init))
+ return false;
FOR_EACH_CONSTRUCTOR_VALUE (CONSTRUCTOR_ELTS (init), idx, elt)
if (!initializer_zerop (elt))
return false;
Index: testsuite/g++.dg/tree-ssa/pr63841.C
===
--- testsuite/g++.dg/tree-ssa/pr63841.C (revision 0)
+++ testsuite/g++.dg/tree-ssa/pr63841.C (working copy)
@@ -0,0 +1,38 @@
+/* { dg-do run } */
+/* { dg-options -O2 } */
+
+#include cstdio
+#include string
+
+std::string __attribute__ ((noinline)) comp_test_write() {
+ std::string data;
+
+ for (int i = 0; i 2; ++i) {
+char b = 1 (i * 8);
+data.append(b, 1);
+ }
+
+ return data;
+}
+
+std::string __attribute__ ((noinline)) comp_test_write_good() {
+ std::string data;
+
+ char b;
+ for (int i = 0; i 2; ++i) {
+b = 1 (i * 8);
+data.append(b, 1);
+ }
+
+ return data;
+}
+
+int main() {
+ std::string good = comp_test_write_good();
+ printf(expected: %hx\n, *(short*)good.c_str());
+
+ std::string bad = comp_test_write();
+ printf(got: %hx\n, *(short*)bad.c_str());
+
+ return good != bad;
+}
On Wed, Nov 12, 2014 at 9:40 PM, Andrew Pinski pins...@gmail.com wrote:
On Wed, Nov 12, 2014 at 9:38 PM, Teresa Johnson tejohn...@google.com
wrote:
On Wed, Nov 12, 2014 at 9:30 PM, Andrew Pinski pins...@gmail.com wrote:
On Wed, Nov 12, 2014 at 9:25 PM, Teresa Johnson tejohn...@google.com
wrote:
Added testcase. Here is the new patch:
2014-11-12tejohn...@google.com
gcc:
PR tree-optimization/63841
* tree.c (initializer_zerop): A constructor with no elements
does not zero initialize.
Actually an empty constructor does zero initialize. A clobber does not.
Ok, thanks, I wasn't sure.
The check you want is TREE_CLOBBER_P instead.
So in initializer_zerop, a constructor that is a TREE_CLOBBER_P would
return false, right? I will make that change and retest.
Yes that is correct. Note TREE_CLOBBER_P already checks if it is an
constructor.
Thanks,
Andrew
Thanks,
Teresa
Thanks,
Andrew
gcc/testsuite:
* g++.dg/tree-ssa/pr63841.C: New test.
Index: tree.c
===
--- tree.c (revision 217190)
+++ tree.c (working copy)
@@ -10330,6 +10330,8 @@ initializer_zerop (const_tree init)
{
unsigned HOST_WIDE_INT idx;
+if (!CONSTRUCTOR_NELTS (init))
+ return false;
FOR_EACH_CONSTRUCTOR_VALUE (CONSTRUCTOR_ELTS (init), idx, elt)
if (!initializer_zerop (elt))
return false;
Index: testsuite/g++.dg/tree-ssa/pr63841.C
===
--- testsuite/g++.dg/tree-ssa/pr63841.C (revision 0)
+++ testsuite/g++.dg/tree-ssa/pr63841.C (working copy)
@@ -0,0 +1,38 @@
+/* { dg-do run } */
+/* { dg-options -O2 } */
+
+#include cstdio
+#include string
+
+std::string __attribute__ ((noinline)) comp_test_write() {
+ std::string data;
+
+ for (int i = 0; i 2; ++i) {
+char b = 1 (i * 8);
+data.append(b, 1);
+ }
+
+ return data;
+}
+
+std::string __attribute__ ((noinline)) comp_test_write_good() {
+ std::string data;
+
+ char b;
+ for (int i = 0; i 2; ++i) {
+b = 1 (i * 8);
+data.append(b, 1);
+ }
+
+ return data;
+}
+
+int main() {
+ std::string good = comp_test_write_good();
+ printf(expected: %hx\n, *(short*)good.c_str());
+
+ std::string bad = comp_test_write();
+ printf(got: %hx\n, *(short*)bad.c_str());
+
+ return good != bad;
+}
On Wed, Nov 12, 2014 at 2:17 PM, Xinliang David Li davi...@google.com
wrote:
missing test case?
David
On Wed, Nov 12, 2014 at
Re: [PATCH] Fix PR/63841: empty constructor doesn't zero-initialize
On Thu, Nov 13, 2014 at 06:20:16AM -0800, Teresa Johnson wrote:
Here is the new patch. Bootstrapped and tested on
x86_64-unknown-linux-gnu. OK for trunk?
Thanks,
Teresa
2014-11-13tejohn...@google.com
gcc:
PR tree-optimization/63841
* tree.c (initializer_zerop): A constructor with no elements
does not zero initialize.
gcc/testsuite:
PR tree-optimization/63841
* g++.dg/tree-ssa/pr63841.C: New test.
Index: tree.c
===
--- tree.c (revision 217190)
+++ tree.c (working copy)
@@ -10330,6 +10330,8 @@ initializer_zerop (const_tree init)
{
unsigned HOST_WIDE_INT idx;
+if (TREE_CLOBBER_P (init))
+ return false;
Wrong formatting.
Also, while this perhaps is useful, I'd say the right fix is that
strlen_optimize_stmt
should just ignore gimple_clobber_p (stmt) statements (well, call
the maybe_invalidate at the end for them).
So
- else if (is_gimple_assign (stmt))
+ else if (is_gimple_assign (stmt) !gimple_clobber_p (stmt))
in strlen_optimize_stmt.
Jakub
Re: [PATCH] Fix PR/63841: empty constructor doesn't zero-initialize
On Thu, Nov 13, 2014 at 6:36 AM, Jakub Jelinek ja...@redhat.com wrote:
On Thu, Nov 13, 2014 at 06:20:16AM -0800, Teresa Johnson wrote:
Here is the new patch. Bootstrapped and tested on
x86_64-unknown-linux-gnu. OK for trunk?
Thanks,
Teresa
2014-11-13tejohn...@google.com
gcc:
PR tree-optimization/63841
* tree.c (initializer_zerop): A constructor with no elements
does not zero initialize.
gcc/testsuite:
PR tree-optimization/63841
* g++.dg/tree-ssa/pr63841.C: New test.
Index: tree.c
===
--- tree.c (revision 217190)
+++ tree.c (working copy)
@@ -10330,6 +10330,8 @@ initializer_zerop (const_tree init)
{
unsigned HOST_WIDE_INT idx;
+if (TREE_CLOBBER_P (init))
+ return false;
Wrong formatting.
Sorry, not sure I understand why? My mailer does tend to eat spaces,
but it is indented the correct amount and I think has the right spaces
within the line.
Also, while this perhaps is useful, I'd say the right fix is that
strlen_optimize_stmt
should just ignore gimple_clobber_p (stmt) statements (well, call
the maybe_invalidate at the end for them).
So
- else if (is_gimple_assign (stmt))
+ else if (is_gimple_assign (stmt) !gimple_clobber_p (stmt))
in strlen_optimize_stmt.
Ok, I have held off on my commit for now. I considered fixing this in
tree-ssa-strlen, but thought this was a potentially larger problem
with initializer_zerop, where it shouldn't be considering a clobber to
be a zero init and might be affecting other callers as well.
If we make the change to initializer_zerop is it still useful to
change tree-strlen?
Teresa
Jakub
--
Teresa Johnson | Software Engineer | tejohn...@google.com | 408-460-2413
Re: [PATCH] Fix PR/63841: empty constructor doesn't zero-initialize
On Thu, Nov 13, 2014 at 3:46 PM, Teresa Johnson tejohn...@google.com wrote:
On Thu, Nov 13, 2014 at 6:36 AM, Jakub Jelinek ja...@redhat.com wrote:
On Thu, Nov 13, 2014 at 06:20:16AM -0800, Teresa Johnson wrote:
Here is the new patch. Bootstrapped and tested on
x86_64-unknown-linux-gnu. OK for trunk?
Thanks,
Teresa
2014-11-13tejohn...@google.com
gcc:
PR tree-optimization/63841
* tree.c (initializer_zerop): A constructor with no elements
does not zero initialize.
gcc/testsuite:
PR tree-optimization/63841
* g++.dg/tree-ssa/pr63841.C: New test.
Index: tree.c
===
--- tree.c (revision 217190)
+++ tree.c (working copy)
@@ -10330,6 +10330,8 @@ initializer_zerop (const_tree init)
{
unsigned HOST_WIDE_INT idx;
+if (TREE_CLOBBER_P (init))
+ return false;
Wrong formatting.
Sorry, not sure I understand why? My mailer does tend to eat spaces,
but it is indented the correct amount and I think has the right spaces
within the line.
Also, while this perhaps is useful, I'd say the right fix is that
strlen_optimize_stmt
should just ignore gimple_clobber_p (stmt) statements (well, call
the maybe_invalidate at the end for them).
So
- else if (is_gimple_assign (stmt))
+ else if (is_gimple_assign (stmt) !gimple_clobber_p (stmt))
in strlen_optimize_stmt.
Ok, I have held off on my commit for now. I considered fixing this in
tree-ssa-strlen, but thought this was a potentially larger problem
with initializer_zerop, where it shouldn't be considering a clobber to
be a zero init and might be affecting other callers as well.
I think it is.
Richard.
If we make the change to initializer_zerop is it still useful to
change tree-strlen?
Teresa
Jakub
--
Teresa Johnson | Software Engineer | tejohn...@google.com | 408-460-2413
Re: [PATCH] Fix PR/63841: empty constructor doesn't zero-initialize
On Thu, Nov 13, 2014 at 06:46:23AM -0800, Teresa Johnson wrote:
--- tree.c (revision 217190)
+++ tree.c (working copy)
@@ -10330,6 +10330,8 @@ initializer_zerop (const_tree init)
{
unsigned HOST_WIDE_INT idx;
+if (TREE_CLOBBER_P (init))
+ return false;
Wrong formatting.
Sorry, not sure I understand why? My mailer does tend to eat spaces,
but it is indented the correct amount and I think has the right spaces
within the line.
I meant that you are using spaces instead of tabs and the unsigned line
above it being one char off suggested to me visually it has the tab in there
(apparently it is eaten too). Use MUA that doesn't eat it, or convince your
employer that tabs are important in e-mails? ;)
If you commit tabs in there, it is fine with me.
Ok, I have held off on my commit for now. I considered fixing this in
tree-ssa-strlen, but thought this was a potentially larger problem
with initializer_zerop, where it shouldn't be considering a clobber to
be a zero init and might be affecting other callers as well.
As I said, I think your tree.c change is useful, but perhaps too risky
for the release branches, because we don't know what all it will affect?
If we make the change to initializer_zerop is it still useful to
change tree-strlen?
I think it is better to be clear on it that right now we want clobbers
to clobber the memory for strlen pass purposes, maybe in the future we want
to perform some optimizations for them as Richard suggested in the PR
(though, extending the lifetime in that case by removing the clobber
shouldn't be done unconditionally (i.e. limited to when we'd actually want
to benefit from the known length) and assuming we are close to the clobber
(i.e. it is fine to extend it a little bit, but not extend the lifetime
across multi-megabyte function e.g.).
And for release branches I'd really prefer tree-ssa-strlen.c change.
Jakub
Re: [PATCH] Fix PR/63841: empty constructor doesn't zero-initialize
On Thu, Nov 13, 2014 at 7:12 AM, Jakub Jelinek ja...@redhat.com wrote:
On Thu, Nov 13, 2014 at 06:46:23AM -0800, Teresa Johnson wrote:
--- tree.c (revision 217190)
+++ tree.c (working copy)
@@ -10330,6 +10330,8 @@ initializer_zerop (const_tree init)
{
unsigned HOST_WIDE_INT idx;
+if (TREE_CLOBBER_P (init))
+ return false;
Wrong formatting.
Sorry, not sure I understand why? My mailer does tend to eat spaces,
but it is indented the correct amount and I think has the right spaces
within the line.
I meant that you are using spaces instead of tabs and the unsigned line
above it being one char off suggested to me visually it has the tab in there
(apparently it is eaten too). Use MUA that doesn't eat it, or convince your
employer that tabs are important in e-mails? ;)
If you commit tabs in there, it is fine with me.
Changed the spaces to tabs to match the surrounding lines and
committed to trunk (r217505).
Ok, I have held off on my commit for now. I considered fixing this in
tree-ssa-strlen, but thought this was a potentially larger problem
with initializer_zerop, where it shouldn't be considering a clobber to
be a zero init and might be affecting other callers as well.
As I said, I think your tree.c change is useful, but perhaps too risky
for the release branches, because we don't know what all it will affect?
If we make the change to initializer_zerop is it still useful to
change tree-strlen?
I think it is better to be clear on it that right now we want clobbers
to clobber the memory for strlen pass purposes, maybe in the future we want
to perform some optimizations for them as Richard suggested in the PR
(though, extending the lifetime in that case by removing the clobber
shouldn't be done unconditionally (i.e. limited to when we'd actually want
to benefit from the known length) and assuming we are close to the clobber
(i.e. it is fine to extend it a little bit, but not extend the lifetime
across multi-megabyte function e.g.).
And for release branches I'd really prefer tree-ssa-strlen.c change.
Ok, I started testing the initializer_zerop change on the 4_9 branch,
will also test the strlen fix and send that patch for review here when
it completes.
Thanks,
Teresa
Jakub
--
Teresa Johnson | Software Engineer | tejohn...@google.com | 408-460-2413
Re: [PATCH] Fix PR/63841: empty constructor doesn't zero-initialize
On Thu, Nov 13, 2014 at 7:39 AM, Teresa Johnson tejohn...@google.com wrote:
On Thu, Nov 13, 2014 at 7:12 AM, Jakub Jelinek ja...@redhat.com wrote:
And for release branches I'd really prefer tree-ssa-strlen.c change.
Ok, I started testing the initializer_zerop change on the 4_9 branch,
will also test the strlen fix and send that patch for review here when
it completes.
Here is the more conservative patch for 4_9. Bootstrapped and tested
on x86_64-unknown-linux-gnu. Ok for gcc-4_9 branch?
Thanks,
Teresa
2014-11-13 Teresa Johnson tejohn...@google.com
gcc:
PR tree-optimization/63841
* tree-ssa-strlen.c (strlen_optimize_stmt): Ignore clobbers.
2014-11-13 Teresa Johnson tejohn...@google.com
gcc/testsuite:
PR tree-optimization/63841
* testsuite/g++.dg/tree-ssa/pr63841.C: New test.
Index: tree-ssa-strlen.c
===
--- tree-ssa-strlen.c (revision 217503)
+++ tree-ssa-strlen.c (working copy)
@@ -1856,7 +1856,7 @@ strlen_optimize_stmt (gimple_stmt_iterator *gsi)
break;
}
}
- else if (is_gimple_assign (stmt))
+ else if (is_gimple_assign (stmt) !gimple_clobber_p (stmt))
{
tree lhs = gimple_assign_lhs (stmt);
Index: testsuite/g++.dg/tree-ssa/pr63841.C
===
--- testsuite/g++.dg/tree-ssa/pr63841.C (revision 0)
+++ testsuite/g++.dg/tree-ssa/pr63841.C (working copy)
@@ -0,0 +1,38 @@
+/* { dg-do run } */
+/* { dg-options -O2 } */
+
+#include cstdio
+#include string
+
+std::string __attribute__ ((noinline)) comp_test_write() {
+ std::string data;
+
+ for (int i = 0; i 2; ++i) {
+char b = 1 (i * 8);
+data.append(b, 1);
+ }
+
+ return data;
+}
+
+std::string __attribute__ ((noinline)) comp_test_write_good() {
+ std::string data;
+
+ char b;
+ for (int i = 0; i 2; ++i) {
+b = 1 (i * 8);
+data.append(b, 1);
+ }
+
+ return data;
+}
+
+int main() {
+ std::string good = comp_test_write_good();
+ printf(expected: %hx\n, *(short*)good.c_str());
+
+ std::string bad = comp_test_write();
+ printf(got: %hx\n, *(short*)bad.c_str());
+
+ return good != bad;
+}
--
Teresa Johnson | Software Engineer | tejohn...@google.com | 408-460-2413
Re: [PATCH] Fix PR/63841: empty constructor doesn't zero-initialize
On Thu, Nov 13, 2014 at 12:51:05PM -0800, Teresa Johnson wrote:
On Thu, Nov 13, 2014 at 7:39 AM, Teresa Johnson tejohn...@google.com wrote:
On Thu, Nov 13, 2014 at 7:12 AM, Jakub Jelinek ja...@redhat.com wrote:
And for release branches I'd really prefer tree-ssa-strlen.c change.
Ok, I started testing the initializer_zerop change on the 4_9 branch,
will also test the strlen fix and send that patch for review here when
it completes.
Here is the more conservative patch for 4_9. Bootstrapped and tested
on x86_64-unknown-linux-gnu. Ok for gcc-4_9 branch?
Ok, thanks. But I have a comment regarding the test below:
2014-11-13 Teresa Johnson tejohn...@google.com
gcc:
PR tree-optimization/63841
* tree-ssa-strlen.c (strlen_optimize_stmt): Ignore clobbers.
2014-11-13 Teresa Johnson tejohn...@google.com
gcc/testsuite:
PR tree-optimization/63841
* testsuite/g++.dg/tree-ssa/pr63841.C: New test.
Index: tree-ssa-strlen.c
===
--- tree-ssa-strlen.c (revision 217503)
+++ tree-ssa-strlen.c (working copy)
@@ -1856,7 +1856,7 @@ strlen_optimize_stmt (gimple_stmt_iterator *gsi)
break;
}
}
- else if (is_gimple_assign (stmt))
+ else if (is_gimple_assign (stmt) !gimple_clobber_p (stmt))
{
tree lhs = gimple_assign_lhs (stmt);
Index: testsuite/g++.dg/tree-ssa/pr63841.C
===
--- testsuite/g++.dg/tree-ssa/pr63841.C (revision 0)
+++ testsuite/g++.dg/tree-ssa/pr63841.C (working copy)
@@ -0,0 +1,38 @@
+/* { dg-do run } */
+/* { dg-options -O2 } */
+
+#include cstdio
+#include string
+
+std::string __attribute__ ((noinline)) comp_test_write() {
+ std::string data;
+
+ for (int i = 0; i 2; ++i) {
+char b = 1 (i * 8);
+data.append(b, 1);
+ }
+
+ return data;
+}
+
+std::string __attribute__ ((noinline)) comp_test_write_good() {
+ std::string data;
+
+ char b;
+ for (int i = 0; i 2; ++i) {
+b = 1 (i * 8);
+data.append(b, 1);
+ }
+
+ return data;
+}
+
+int main() {
+ std::string good = comp_test_write_good();
+ printf(expected: %hx\n, *(short*)good.c_str());
+
+ std::string bad = comp_test_write();
+ printf(got: %hx\n, *(short*)bad.c_str());
Supposedly the printfs should have been removed and the #include cstdio
isn't needed then either. No need to clutter the test output and log files.
On the other side, tests should abort (); or __builtin_abort (); on failure,
not just return non-zero.
+
+ return good != bad;
+}
Jakub
Re: [PATCH] Fix PR/63841: empty constructor doesn't zero-initialize
On Thu, Nov 13, 2014 at 12:55 PM, Jakub Jelinek ja...@redhat.com wrote:
On Thu, Nov 13, 2014 at 12:51:05PM -0800, Teresa Johnson wrote:
On Thu, Nov 13, 2014 at 7:39 AM, Teresa Johnson tejohn...@google.com wrote:
On Thu, Nov 13, 2014 at 7:12 AM, Jakub Jelinek ja...@redhat.com wrote:
And for release branches I'd really prefer tree-ssa-strlen.c change.
Ok, I started testing the initializer_zerop change on the 4_9 branch,
will also test the strlen fix and send that patch for review here when
it completes.
Here is the more conservative patch for 4_9. Bootstrapped and tested
on x86_64-unknown-linux-gnu. Ok for gcc-4_9 branch?
Ok, thanks. But I have a comment regarding the test below:
2014-11-13 Teresa Johnson tejohn...@google.com
gcc:
PR tree-optimization/63841
* tree-ssa-strlen.c (strlen_optimize_stmt): Ignore clobbers.
2014-11-13 Teresa Johnson tejohn...@google.com
gcc/testsuite:
PR tree-optimization/63841
* testsuite/g++.dg/tree-ssa/pr63841.C: New test.
Index: tree-ssa-strlen.c
===
--- tree-ssa-strlen.c (revision 217503)
+++ tree-ssa-strlen.c (working copy)
@@ -1856,7 +1856,7 @@ strlen_optimize_stmt (gimple_stmt_iterator *gsi)
break;
}
}
- else if (is_gimple_assign (stmt))
+ else if (is_gimple_assign (stmt) !gimple_clobber_p (stmt))
{
tree lhs = gimple_assign_lhs (stmt);
Index: testsuite/g++.dg/tree-ssa/pr63841.C
===
--- testsuite/g++.dg/tree-ssa/pr63841.C (revision 0)
+++ testsuite/g++.dg/tree-ssa/pr63841.C (working copy)
@@ -0,0 +1,38 @@
+/* { dg-do run } */
+/* { dg-options -O2 } */
+
+#include cstdio
+#include string
+
+std::string __attribute__ ((noinline)) comp_test_write() {
+ std::string data;
+
+ for (int i = 0; i 2; ++i) {
+char b = 1 (i * 8);
+data.append(b, 1);
+ }
+
+ return data;
+}
+
+std::string __attribute__ ((noinline)) comp_test_write_good() {
+ std::string data;
+
+ char b;
+ for (int i = 0; i 2; ++i) {
+b = 1 (i * 8);
+data.append(b, 1);
+ }
+
+ return data;
+}
+
+int main() {
+ std::string good = comp_test_write_good();
+ printf(expected: %hx\n, *(short*)good.c_str());
+
+ std::string bad = comp_test_write();
+ printf(got: %hx\n, *(short*)bad.c_str());
Supposedly the printfs should have been removed and the #include cstdio
isn't needed then either. No need to clutter the test output and log files.
On the other side, tests should abort (); or __builtin_abort (); on failure,
not just return non-zero.
Ok, sounds good. Here is the new patch. Confirmed that the test case
passes with the fix, aborts without it.
If this looks good I will also update the trunk version of the test.
Thanks,
Teresa
2014-11-13 Teresa Johnson tejohn...@google.com
gcc:
PR tree-optimization/63841
* tree-ssa-strlen.c (strlen_optimize_stmt): Ignore clobbers.
2014-11-13 Teresa Johnson tejohn...@google.com
gcc/testsuite:
PR tree-optimization/63841
* testsuite/g++.dg/tree-ssa/pr63841.C: New test.
Index: tree-ssa-strlen.c
===
--- tree-ssa-strlen.c (revision 217503)
+++ tree-ssa-strlen.c (working copy)
@@ -1856,7 +1856,7 @@ strlen_optimize_stmt (gimple_stmt_iterator *gsi)
break;
}
}
- else if (is_gimple_assign (stmt))
+ else if (is_gimple_assign (stmt) !gimple_clobber_p (stmt))
{
tree lhs = gimple_assign_lhs (stmt);
Index: testsuite/g++.dg/tree-ssa/pr63841.C
===
--- testsuite/g++.dg/tree-ssa/pr63841.C (revision 0)
+++ testsuite/g++.dg/tree-ssa/pr63841.C (working copy)
@@ -0,0 +1,35 @@
+/* { dg-do run } */
+/* { dg-options -O2 } */
+
+#include string
+
+std::string __attribute__ ((noinline)) comp_test_write() {
+ std::string data;
+
+ for (int i = 0; i 2; ++i) {
+char b = 1 (i * 8);
+data.append(b, 1);
+ }
+
+ return data;
+}
+
+std::string __attribute__ ((noinline)) comp_test_write_good() {
+ std::string data;
+
+ char b;
+ for (int i = 0; i 2; ++i) {
+b = 1 (i * 8);
+data.append(b, 1);
+ }
+
+ return data;
+}
+
+int main() {
+ std::string good = comp_test_write_good();
+ std::string bad = comp_test_write();
+
+ if (good != bad)
+__builtin_abort ();
+}
+
+ return good != bad;
+}
Jakub
--
Teresa Johnson | Software Engineer | tejohn...@google.com | 408-460-2413
Re: [PATCH] Fix PR/63841: empty constructor doesn't zero-initialize
On Thu, Nov 13, 2014 at 01:33:07PM -0800, Teresa Johnson wrote:
Supposedly the printfs should have been removed and the #include cstdio
isn't needed then either. No need to clutter the test output and log files.
On the other side, tests should abort (); or __builtin_abort (); on failure,
not just return non-zero.
Ok, sounds good. Here is the new patch. Confirmed that the test case
passes with the fix, aborts without it.
If this looks good I will also update the trunk version of the test.
LGTM, thanks.
Please commit also the tree-ssa-strlen.c change to the trunk, it will not
hurt.
2014-11-13 Teresa Johnson tejohn...@google.com
gcc:
PR tree-optimization/63841
* tree-ssa-strlen.c (strlen_optimize_stmt): Ignore clobbers.
2014-11-13 Teresa Johnson tejohn...@google.com
gcc/testsuite:
PR tree-optimization/63841
* testsuite/g++.dg/tree-ssa/pr63841.C: New test.
Index: tree-ssa-strlen.c
===
--- tree-ssa-strlen.c (revision 217503)
+++ tree-ssa-strlen.c (working copy)
@@ -1856,7 +1856,7 @@ strlen_optimize_stmt (gimple_stmt_iterator *gsi)
break;
}
}
- else if (is_gimple_assign (stmt))
+ else if (is_gimple_assign (stmt) !gimple_clobber_p (stmt))
{
tree lhs = gimple_assign_lhs (stmt);
Index: testsuite/g++.dg/tree-ssa/pr63841.C
===
--- testsuite/g++.dg/tree-ssa/pr63841.C (revision 0)
+++ testsuite/g++.dg/tree-ssa/pr63841.C (working copy)
@@ -0,0 +1,35 @@
+/* { dg-do run } */
+/* { dg-options -O2 } */
+
+#include string
+
+std::string __attribute__ ((noinline)) comp_test_write() {
+ std::string data;
+
+ for (int i = 0; i 2; ++i) {
+char b = 1 (i * 8);
+data.append(b, 1);
+ }
+
+ return data;
+}
+
+std::string __attribute__ ((noinline)) comp_test_write_good() {
+ std::string data;
+
+ char b;
+ for (int i = 0; i 2; ++i) {
+b = 1 (i * 8);
+data.append(b, 1);
+ }
+
+ return data;
+}
+
+int main() {
+ std::string good = comp_test_write_good();
+ std::string bad = comp_test_write();
+
+ if (good != bad)
+__builtin_abort ();
+}
Jakub
Re: [PATCH] Fix PR/63841: empty constructor doesn't zero-initialize
On Thu, Nov 13, 2014 at 1:45 PM, Jakub Jelinek ja...@redhat.com wrote:
On Thu, Nov 13, 2014 at 01:33:07PM -0800, Teresa Johnson wrote:
Supposedly the printfs should have been removed and the #include cstdio
isn't needed then either. No need to clutter the test output and log
files.
On the other side, tests should abort (); or __builtin_abort (); on
failure,
not just return non-zero.
Ok, sounds good. Here is the new patch. Confirmed that the test case
passes with the fix, aborts without it.
If this looks good I will also update the trunk version of the test.
LGTM, thanks.
Please commit also the tree-ssa-strlen.c change to the trunk, it will not
hurt.
Ok. It is on gcc-4_9 now, will commit both to trunk as soon as
regression testing completes.
Thanks,
Teresa
2014-11-13 Teresa Johnson tejohn...@google.com
gcc:
PR tree-optimization/63841
* tree-ssa-strlen.c (strlen_optimize_stmt): Ignore clobbers.
2014-11-13 Teresa Johnson tejohn...@google.com
gcc/testsuite:
PR tree-optimization/63841
* testsuite/g++.dg/tree-ssa/pr63841.C: New test.
Index: tree-ssa-strlen.c
===
--- tree-ssa-strlen.c (revision 217503)
+++ tree-ssa-strlen.c (working copy)
@@ -1856,7 +1856,7 @@ strlen_optimize_stmt (gimple_stmt_iterator *gsi)
break;
}
}
- else if (is_gimple_assign (stmt))
+ else if (is_gimple_assign (stmt) !gimple_clobber_p (stmt))
{
tree lhs = gimple_assign_lhs (stmt);
Index: testsuite/g++.dg/tree-ssa/pr63841.C
===
--- testsuite/g++.dg/tree-ssa/pr63841.C (revision 0)
+++ testsuite/g++.dg/tree-ssa/pr63841.C (working copy)
@@ -0,0 +1,35 @@
+/* { dg-do run } */
+/* { dg-options -O2 } */
+
+#include string
+
+std::string __attribute__ ((noinline)) comp_test_write() {
+ std::string data;
+
+ for (int i = 0; i 2; ++i) {
+char b = 1 (i * 8);
+data.append(b, 1);
+ }
+
+ return data;
+}
+
+std::string __attribute__ ((noinline)) comp_test_write_good() {
+ std::string data;
+
+ char b;
+ for (int i = 0; i 2; ++i) {
+b = 1 (i * 8);
+data.append(b, 1);
+ }
+
+ return data;
+}
+
+int main() {
+ std::string good = comp_test_write_good();
+ std::string bad = comp_test_write();
+
+ if (good != bad)
+__builtin_abort ();
+}
Jakub
--
Teresa Johnson | Software Engineer | tejohn...@google.com | 408-460-2413
Re: [PATCH] Fix PR/63841: empty constructor doesn't zero-initialize
missing test case?
David
On Wed, Nov 12, 2014 at 2:13 PM, Teresa Johnson tejohn...@google.com wrote:
This patch fixes an issue where tree-strlen was incorrectly removing a
store of 0 into a string because it thought a prior CLOBBER (which is
an empty constructor with no elements) was zero-initializing the
string.
Bootstrapped and tested on x86_64-unknown-linux-gnu. OK for trunk?
Thanks,
Teresa
2014-11-12tejohn...@google.com
PR tree-optimization/63841
* tree.c (initializer_zerop): A constructor with no elements
does not zero initialize.
Index: tree.c
===
--- tree.c (revision 217190)
+++ tree.c (working copy)
@@ -10330,6 +10330,8 @@ initializer_zerop (const_tree init)
{
unsigned HOST_WIDE_INT idx;
+if (!CONSTRUCTOR_NELTS (init))
+ return false;
FOR_EACH_CONSTRUCTOR_VALUE (CONSTRUCTOR_ELTS (init), idx, elt)
if (!initializer_zerop (elt))
return false;
--
Teresa Johnson | Software Engineer | tejohn...@google.com | 408-460-2413
Re: [PATCH] Fix PR/63841: empty constructor doesn't zero-initialize
Added testcase. Here is the new patch:
2014-11-12tejohn...@google.com
gcc:
PR tree-optimization/63841
* tree.c (initializer_zerop): A constructor with no elements
does not zero initialize.
gcc/testsuite:
* g++.dg/tree-ssa/pr63841.C: New test.
Index: tree.c
===
--- tree.c (revision 217190)
+++ tree.c (working copy)
@@ -10330,6 +10330,8 @@ initializer_zerop (const_tree init)
{
unsigned HOST_WIDE_INT idx;
+if (!CONSTRUCTOR_NELTS (init))
+ return false;
FOR_EACH_CONSTRUCTOR_VALUE (CONSTRUCTOR_ELTS (init), idx, elt)
if (!initializer_zerop (elt))
return false;
Index: testsuite/g++.dg/tree-ssa/pr63841.C
===
--- testsuite/g++.dg/tree-ssa/pr63841.C (revision 0)
+++ testsuite/g++.dg/tree-ssa/pr63841.C (working copy)
@@ -0,0 +1,38 @@
+/* { dg-do run } */
+/* { dg-options -O2 } */
+
+#include cstdio
+#include string
+
+std::string __attribute__ ((noinline)) comp_test_write() {
+ std::string data;
+
+ for (int i = 0; i 2; ++i) {
+char b = 1 (i * 8);
+data.append(b, 1);
+ }
+
+ return data;
+}
+
+std::string __attribute__ ((noinline)) comp_test_write_good() {
+ std::string data;
+
+ char b;
+ for (int i = 0; i 2; ++i) {
+b = 1 (i * 8);
+data.append(b, 1);
+ }
+
+ return data;
+}
+
+int main() {
+ std::string good = comp_test_write_good();
+ printf(expected: %hx\n, *(short*)good.c_str());
+
+ std::string bad = comp_test_write();
+ printf(got: %hx\n, *(short*)bad.c_str());
+
+ return good != bad;
+}
On Wed, Nov 12, 2014 at 2:17 PM, Xinliang David Li davi...@google.com wrote:
missing test case?
David
On Wed, Nov 12, 2014 at 2:13 PM, Teresa Johnson tejohn...@google.com wrote:
This patch fixes an issue where tree-strlen was incorrectly removing a
store of 0 into a string because it thought a prior CLOBBER (which is
an empty constructor with no elements) was zero-initializing the
string.
Bootstrapped and tested on x86_64-unknown-linux-gnu. OK for trunk?
Thanks,
Teresa
2014-11-12tejohn...@google.com
PR tree-optimization/63841
* tree.c (initializer_zerop): A constructor with no elements
does not zero initialize.
Index: tree.c
===
--- tree.c (revision 217190)
+++ tree.c (working copy)
@@ -10330,6 +10330,8 @@ initializer_zerop (const_tree init)
{
unsigned HOST_WIDE_INT idx;
+if (!CONSTRUCTOR_NELTS (init))
+ return false;
FOR_EACH_CONSTRUCTOR_VALUE (CONSTRUCTOR_ELTS (init), idx, elt)
if (!initializer_zerop (elt))
return false;
--
Teresa Johnson | Software Engineer | tejohn...@google.com | 408-460-2413
--
Teresa Johnson | Software Engineer | tejohn...@google.com | 408-460-2413
Re: [PATCH] Fix PR/63841: empty constructor doesn't zero-initialize
On Wed, Nov 12, 2014 at 9:25 PM, Teresa Johnson tejohn...@google.com wrote:
Added testcase. Here is the new patch:
2014-11-12tejohn...@google.com
gcc:
PR tree-optimization/63841
* tree.c (initializer_zerop): A constructor with no elements
does not zero initialize.
Actually an empty constructor does zero initialize. A clobber does not.
The check you want is TREE_CLOBBER_P instead.
Thanks,
Andrew
gcc/testsuite:
* g++.dg/tree-ssa/pr63841.C: New test.
Index: tree.c
===
--- tree.c (revision 217190)
+++ tree.c (working copy)
@@ -10330,6 +10330,8 @@ initializer_zerop (const_tree init)
{
unsigned HOST_WIDE_INT idx;
+if (!CONSTRUCTOR_NELTS (init))
+ return false;
FOR_EACH_CONSTRUCTOR_VALUE (CONSTRUCTOR_ELTS (init), idx, elt)
if (!initializer_zerop (elt))
return false;
Index: testsuite/g++.dg/tree-ssa/pr63841.C
===
--- testsuite/g++.dg/tree-ssa/pr63841.C (revision 0)
+++ testsuite/g++.dg/tree-ssa/pr63841.C (working copy)
@@ -0,0 +1,38 @@
+/* { dg-do run } */
+/* { dg-options -O2 } */
+
+#include cstdio
+#include string
+
+std::string __attribute__ ((noinline)) comp_test_write() {
+ std::string data;
+
+ for (int i = 0; i 2; ++i) {
+char b = 1 (i * 8);
+data.append(b, 1);
+ }
+
+ return data;
+}
+
+std::string __attribute__ ((noinline)) comp_test_write_good() {
+ std::string data;
+
+ char b;
+ for (int i = 0; i 2; ++i) {
+b = 1 (i * 8);
+data.append(b, 1);
+ }
+
+ return data;
+}
+
+int main() {
+ std::string good = comp_test_write_good();
+ printf(expected: %hx\n, *(short*)good.c_str());
+
+ std::string bad = comp_test_write();
+ printf(got: %hx\n, *(short*)bad.c_str());
+
+ return good != bad;
+}
On Wed, Nov 12, 2014 at 2:17 PM, Xinliang David Li davi...@google.com wrote:
missing test case?
David
On Wed, Nov 12, 2014 at 2:13 PM, Teresa Johnson tejohn...@google.com wrote:
This patch fixes an issue where tree-strlen was incorrectly removing a
store of 0 into a string because it thought a prior CLOBBER (which is
an empty constructor with no elements) was zero-initializing the
string.
Bootstrapped and tested on x86_64-unknown-linux-gnu. OK for trunk?
Thanks,
Teresa
2014-11-12tejohn...@google.com
PR tree-optimization/63841
* tree.c (initializer_zerop): A constructor with no elements
does not zero initialize.
Index: tree.c
===
--- tree.c (revision 217190)
+++ tree.c (working copy)
@@ -10330,6 +10330,8 @@ initializer_zerop (const_tree init)
{
unsigned HOST_WIDE_INT idx;
+if (!CONSTRUCTOR_NELTS (init))
+ return false;
FOR_EACH_CONSTRUCTOR_VALUE (CONSTRUCTOR_ELTS (init), idx, elt)
if (!initializer_zerop (elt))
return false;
--
Teresa Johnson | Software Engineer | tejohn...@google.com | 408-460-2413
--
Teresa Johnson | Software Engineer | tejohn...@google.com | 408-460-2413
Re: [PATCH] Fix PR/63841: empty constructor doesn't zero-initialize
On Wed, Nov 12, 2014 at 9:30 PM, Andrew Pinski pins...@gmail.com wrote:
On Wed, Nov 12, 2014 at 9:25 PM, Teresa Johnson tejohn...@google.com wrote:
Added testcase. Here is the new patch:
2014-11-12tejohn...@google.com
gcc:
PR tree-optimization/63841
* tree.c (initializer_zerop): A constructor with no elements
does not zero initialize.
Actually an empty constructor does zero initialize. A clobber does not.
Ok, thanks, I wasn't sure.
The check you want is TREE_CLOBBER_P instead.
So in initializer_zerop, a constructor that is a TREE_CLOBBER_P would
return false, right? I will make that change and retest.
Thanks,
Teresa
Thanks,
Andrew
gcc/testsuite:
* g++.dg/tree-ssa/pr63841.C: New test.
Index: tree.c
===
--- tree.c (revision 217190)
+++ tree.c (working copy)
@@ -10330,6 +10330,8 @@ initializer_zerop (const_tree init)
{
unsigned HOST_WIDE_INT idx;
+if (!CONSTRUCTOR_NELTS (init))
+ return false;
FOR_EACH_CONSTRUCTOR_VALUE (CONSTRUCTOR_ELTS (init), idx, elt)
if (!initializer_zerop (elt))
return false;
Index: testsuite/g++.dg/tree-ssa/pr63841.C
===
--- testsuite/g++.dg/tree-ssa/pr63841.C (revision 0)
+++ testsuite/g++.dg/tree-ssa/pr63841.C (working copy)
@@ -0,0 +1,38 @@
+/* { dg-do run } */
+/* { dg-options -O2 } */
+
+#include cstdio
+#include string
+
+std::string __attribute__ ((noinline)) comp_test_write() {
+ std::string data;
+
+ for (int i = 0; i 2; ++i) {
+char b = 1 (i * 8);
+data.append(b, 1);
+ }
+
+ return data;
+}
+
+std::string __attribute__ ((noinline)) comp_test_write_good() {
+ std::string data;
+
+ char b;
+ for (int i = 0; i 2; ++i) {
+b = 1 (i * 8);
+data.append(b, 1);
+ }
+
+ return data;
+}
+
+int main() {
+ std::string good = comp_test_write_good();
+ printf(expected: %hx\n, *(short*)good.c_str());
+
+ std::string bad = comp_test_write();
+ printf(got: %hx\n, *(short*)bad.c_str());
+
+ return good != bad;
+}
On Wed, Nov 12, 2014 at 2:17 PM, Xinliang David Li davi...@google.com
wrote:
missing test case?
David
On Wed, Nov 12, 2014 at 2:13 PM, Teresa Johnson tejohn...@google.com
wrote:
This patch fixes an issue where tree-strlen was incorrectly removing a
store of 0 into a string because it thought a prior CLOBBER (which is
an empty constructor with no elements) was zero-initializing the
string.
Bootstrapped and tested on x86_64-unknown-linux-gnu. OK for trunk?
Thanks,
Teresa
2014-11-12tejohn...@google.com
PR tree-optimization/63841
* tree.c (initializer_zerop): A constructor with no elements
does not zero initialize.
Index: tree.c
===
--- tree.c (revision 217190)
+++ tree.c (working copy)
@@ -10330,6 +10330,8 @@ initializer_zerop (const_tree init)
{
unsigned HOST_WIDE_INT idx;
+if (!CONSTRUCTOR_NELTS (init))
+ return false;
FOR_EACH_CONSTRUCTOR_VALUE (CONSTRUCTOR_ELTS (init), idx, elt)
if (!initializer_zerop (elt))
return false;
--
Teresa Johnson | Software Engineer | tejohn...@google.com | 408-460-2413
--
Teresa Johnson | Software Engineer | tejohn...@google.com | 408-460-2413
--
Teresa Johnson | Software Engineer | tejohn...@google.com | 408-460-2413
Re: [PATCH] Fix PR/63841: empty constructor doesn't zero-initialize
On Wed, Nov 12, 2014 at 9:38 PM, Teresa Johnson tejohn...@google.com wrote:
On Wed, Nov 12, 2014 at 9:30 PM, Andrew Pinski pins...@gmail.com wrote:
On Wed, Nov 12, 2014 at 9:25 PM, Teresa Johnson tejohn...@google.com wrote:
Added testcase. Here is the new patch:
2014-11-12tejohn...@google.com
gcc:
PR tree-optimization/63841
* tree.c (initializer_zerop): A constructor with no elements
does not zero initialize.
Actually an empty constructor does zero initialize. A clobber does not.
Ok, thanks, I wasn't sure.
The check you want is TREE_CLOBBER_P instead.
So in initializer_zerop, a constructor that is a TREE_CLOBBER_P would
return false, right? I will make that change and retest.
Yes that is correct. Note TREE_CLOBBER_P already checks if it is an
constructor.
Thanks,
Andrew
Thanks,
Teresa
Thanks,
Andrew
gcc/testsuite:
* g++.dg/tree-ssa/pr63841.C: New test.
Index: tree.c
===
--- tree.c (revision 217190)
+++ tree.c (working copy)
@@ -10330,6 +10330,8 @@ initializer_zerop (const_tree init)
{
unsigned HOST_WIDE_INT idx;
+if (!CONSTRUCTOR_NELTS (init))
+ return false;
FOR_EACH_CONSTRUCTOR_VALUE (CONSTRUCTOR_ELTS (init), idx, elt)
if (!initializer_zerop (elt))
return false;
Index: testsuite/g++.dg/tree-ssa/pr63841.C
===
--- testsuite/g++.dg/tree-ssa/pr63841.C (revision 0)
+++ testsuite/g++.dg/tree-ssa/pr63841.C (working copy)
@@ -0,0 +1,38 @@
+/* { dg-do run } */
+/* { dg-options -O2 } */
+
+#include cstdio
+#include string
+
+std::string __attribute__ ((noinline)) comp_test_write() {
+ std::string data;
+
+ for (int i = 0; i 2; ++i) {
+char b = 1 (i * 8);
+data.append(b, 1);
+ }
+
+ return data;
+}
+
+std::string __attribute__ ((noinline)) comp_test_write_good() {
+ std::string data;
+
+ char b;
+ for (int i = 0; i 2; ++i) {
+b = 1 (i * 8);
+data.append(b, 1);
+ }
+
+ return data;
+}
+
+int main() {
+ std::string good = comp_test_write_good();
+ printf(expected: %hx\n, *(short*)good.c_str());
+
+ std::string bad = comp_test_write();
+ printf(got: %hx\n, *(short*)bad.c_str());
+
+ return good != bad;
+}
On Wed, Nov 12, 2014 at 2:17 PM, Xinliang David Li davi...@google.com
wrote:
missing test case?
David
On Wed, Nov 12, 2014 at 2:13 PM, Teresa Johnson tejohn...@google.com
wrote:
This patch fixes an issue where tree-strlen was incorrectly removing a
store of 0 into a string because it thought a prior CLOBBER (which is
an empty constructor with no elements) was zero-initializing the
string.
Bootstrapped and tested on x86_64-unknown-linux-gnu. OK for trunk?
Thanks,
Teresa
2014-11-12tejohn...@google.com
PR tree-optimization/63841
* tree.c (initializer_zerop): A constructor with no elements
does not zero initialize.
Index: tree.c
===
--- tree.c (revision 217190)
+++ tree.c (working copy)
@@ -10330,6 +10330,8 @@ initializer_zerop (const_tree init)
{
unsigned HOST_WIDE_INT idx;
+if (!CONSTRUCTOR_NELTS (init))
+ return false;
FOR_EACH_CONSTRUCTOR_VALUE (CONSTRUCTOR_ELTS (init), idx, elt)
if (!initializer_zerop (elt))
return false;
--
Teresa Johnson | Software Engineer | tejohn...@google.com | 408-460-2413
--
Teresa Johnson | Software Engineer | tejohn...@google.com | 408-460-2413
--
Teresa Johnson | Software Engineer | tejohn...@google.com | 408-460-2413
