diff mbox

Fix PR/63841: empty constructor doesn't zero-initialize

Message ID CAAe5K+US+=C6QmhA5A_Jfd_e0-q=ZTxAqk=kXQVNJ7RJY6XByA@mail.gmail.com
State New
Headers show

Commit Message

Teresa Johnson Nov. 13, 2014, 9:33 p.m. UTC 
On Thu, Nov 13, 2014 at 12:55 PM, Jakub Jelinek <jakub@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 <tejohnson@google.com> wrote:
>> > On Thu, Nov 13, 2014 at 7:12 AM, Jakub Jelinek <jakub@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  <tejohnson@google.com>
>>
>> gcc:
>>         PR tree-optimization/63841
>>         * tree-ssa-strlen.c (strlen_optimize_stmt): Ignore clobbers.
>>
>> 2014-11-13  Teresa Johnson  <tejohnson@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  <tejohnson@google.com>

gcc:
        PR tree-optimization/63841
        * tree-ssa-strlen.c (strlen_optimize_stmt): Ignore clobbers.

2014-11-13  Teresa Johnson  <tejohnson@google.com>

gcc/testsuite:
        PR tree-optimization/63841
        * testsuite/g++.dg/tree-ssa/pr63841.C: New test.


>
>> +
>> +  return good != bad;
>> +}
>
>         Jakub

Comments

Jakub Jelinek Nov. 13, 2014, 9:45 p.m. UTC | #1 
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  <tejohnson@google.com>
> 
> gcc:
>         PR tree-optimization/63841
>         * tree-ssa-strlen.c (strlen_optimize_stmt): Ignore clobbers.
> 
> 2014-11-13  Teresa Johnson  <tejohnson@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 Nov. 13, 2014, 9:58 p.m. UTC | #2 
On Thu, Nov 13, 2014 at 1:45 PM, Jakub Jelinek <jakub@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  <tejohnson@google.com>
>>
>> gcc:
>>         PR tree-optimization/63841
>>         * tree-ssa-strlen.c (strlen_optimize_stmt): Ignore clobbers.
>>
>> 2014-11-13  Teresa Johnson  <tejohnson@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
diff mbox

Patch

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 ();
+}