On Wed, 17 Jul 2024, Jakub Jelinek wrote:
> Hi!
>
> The builtin-clear-padding-6.c testcase fails as clear_padding_type
> doesn't correctly recompute the buf->size and buf->off members after
> expanding clearing of an array using a runtime loop.
> buf->size should be in that case the offset after which it should continue
> with next members or padding before them modulo UNITS_PER_WORD and
> buf->off that offset minus buf->size. That is what the code was doing,
> but with off being the start of the loop cleared array, not its end.
> So, the last hunk in gimple-fold.cc fixes that.
> When adding the testcase, I've noticed that the
> c-c++-common/torture/builtin-clear-padding-* tests, although clearly
> written as runtime tests to test the builtins at runtime, didn't have
> { dg-do run } directive and were just compile tests because of that.
> When adding that to the tests, builtin-clear-padding-1.c was already
> failing without that clear_padding_type hunk too, but
> builtin-clear-padding-5.c was still failing even after the change.
> That is due to a bug in clear_padding_flush which the patch fixes as
> well - when clear_padding_flush is called with full=true (that happens
> at the end of the whole __builtin_clear_padding or on those array
> padding clears done by a runtime loop), it wants to flush all the pending
> padding clearings rather than just some. If it is at the end of the whole
> object, it decreases wordsize when needed to make sure the code never writes
> including RMW cycles to something outside of the object:
> if ((unsigned HOST_WIDE_INT) (buf->off + i + wordsize)
> > (unsigned HOST_WIDE_INT) buf->sz)
> {
> gcc_assert (wordsize > 1);
> wordsize /= 2;
> i -= wordsize;
> continue;
> }
> but if it is full==true flush in the middle, this doesn't happen, but we
> still process just the buffer bytes before the current end. If that end
> is not on a wordsize boundary, e.g. on the builtin-clear-padding-5.c test
> the last chunk is 2 bytes, '\0', '\xff', i is 16 and end is 18,
> nonzero_last might be equal to the end - i, i.e. 2 here, but still all_ones
> might be true, so in some spots we just didn't emit any clearing in that
> last chunk.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk
> and release branches? This affects even 11 branch, dunno if we want to
> change it even there or just ignore. It is a wrong-code issue though, where
> it can overwrite random non-padding bits or bytes instead of the padding
> one.
OK. It's a bit late for the 11 branch without some soaking on trunk -
when do we use __builtin_clear_padding? IIRC for C++ atomics?
Thanks,
Richard.
> 2024-07-17 Jakub Jelinek <ja...@redhat.com>
>
> PR middle-end/115527
> * gimple-fold.cc (clear_padding_flush): Introduce endsize
> variable and use it instead of wordsize when comparing it against
> nonzero_last.
> (clear_padding_type): Increment off by sz.
>
> * c-c++-common/torture/builtin-clear-padding-1.c: Add dg-do run
> directive.
> * c-c++-common/torture/builtin-clear-padding-2.c: Likewise.
> * c-c++-common/torture/builtin-clear-padding-3.c: Likewise.
> * c-c++-common/torture/builtin-clear-padding-4.c: Likewise.
> * c-c++-common/torture/builtin-clear-padding-5.c: Likewise.
> * c-c++-common/torture/builtin-clear-padding-6.c: New test.
>
> --- gcc/gimple-fold.cc.jj 2024-07-16 13:36:36.000000000 +0200
> +++ gcc/gimple-fold.cc 2024-07-16 19:27:06.776641459 +0200
> @@ -4242,7 +4242,8 @@ clear_padding_flush (clear_padding_struc
> i -= wordsize;
> continue;
> }
> - for (size_t j = i; j < i + wordsize && j < end; j++)
> + size_t endsize = end - i > wordsize ? wordsize : end - i;
> + for (size_t j = i; j < i + endsize; j++)
> {
> if (buf->buf[j])
> {
> @@ -4271,12 +4272,12 @@ clear_padding_flush (clear_padding_struc
> if (padding_bytes)
> {
> if (nonzero_first == 0
> - && nonzero_last == wordsize
> + && nonzero_last == endsize
> && all_ones)
> {
> /* All bits are padding and we had some padding
> before too. Just extend it. */
> - padding_bytes += wordsize;
> + padding_bytes += endsize;
> continue;
> }
> if (all_ones && nonzero_first == 0)
> @@ -4316,7 +4317,7 @@ clear_padding_flush (clear_padding_struc
> if (nonzero_first == wordsize)
> /* All bits in a word are 0, there are no padding bits. */
> continue;
> - if (all_ones && nonzero_last == wordsize)
> + if (all_ones && nonzero_last == endsize)
> {
> /* All bits between nonzero_first and end of word are padding
> bits, start counting padding_bytes. */
> @@ -4358,7 +4359,7 @@ clear_padding_flush (clear_padding_struc
> j = k;
> }
> }
> - if (nonzero_last == wordsize)
> + if (nonzero_last == endsize)
> padding_bytes = nonzero_last - zero_last;
> continue;
> }
> @@ -4832,6 +4833,7 @@ clear_padding_type (clear_padding_struct
> buf->off = 0;
> buf->size = 0;
> clear_padding_emit_loop (buf, elttype, end, for_auto_init);
> + off += sz;
> buf->base = base;
> buf->sz = prev_sz;
> buf->align = prev_align;
> --- gcc/testsuite/c-c++-common/torture/builtin-clear-padding-1.c.jj
> 2020-11-20 12:28:10.105680922 +0100
> +++ gcc/testsuite/c-c++-common/torture/builtin-clear-padding-1.c
> 2024-07-16 17:05:39.950485611 +0200
> @@ -1,4 +1,5 @@
> /* PR libstdc++/88101 */
> +/* { dg-do run } */
>
> int i1, i2;
> long double l1, l2;
> --- gcc/testsuite/c-c++-common/torture/builtin-clear-padding-2.c.jj
> 2020-11-20 12:28:10.105680922 +0100
> +++ gcc/testsuite/c-c++-common/torture/builtin-clear-padding-2.c
> 2024-07-16 17:05:52.710323728 +0200
> @@ -1,4 +1,5 @@
> /* PR libstdc++/88101 */
> +/* { dg-do run } */
>
> typedef int T __attribute__((aligned (16384)));
> struct S { char a; short b; long double c; T d; T e; long long f; };
> --- gcc/testsuite/c-c++-common/torture/builtin-clear-padding-3.c.jj
> 2020-11-20 12:28:10.105680922 +0100
> +++ gcc/testsuite/c-c++-common/torture/builtin-clear-padding-3.c
> 2024-07-16 17:06:05.366163163 +0200
> @@ -1,4 +1,5 @@
> /* PR libstdc++/88101 */
> +/* { dg-do run } */
>
> union V { char a; signed char b; unsigned char c; };
> struct T { char a; int b; union V c; };
> --- gcc/testsuite/c-c++-common/torture/builtin-clear-padding-4.c.jj
> 2022-02-11 11:20:24.285392974 +0100
> +++ gcc/testsuite/c-c++-common/torture/builtin-clear-padding-4.c
> 2024-07-16 17:06:37.671753308 +0200
> @@ -1,6 +1,6 @@
> -/* { dg-require-effective-target alloca } */
> -
> /* PR libstdc++/88101 */
> +/* { dg-do run } */
> +/* { dg-require-effective-target alloca } */
>
> struct S { char a; short b; char c; };
>
> --- gcc/testsuite/c-c++-common/torture/builtin-clear-padding-5.c.jj
> 2020-11-20 12:28:10.106680911 +0100
> +++ gcc/testsuite/c-c++-common/torture/builtin-clear-padding-5.c
> 2024-07-16 17:06:49.216606833 +0200
> @@ -1,4 +1,5 @@
> /* PR libstdc++/88101 */
> +/* { dg-do run } */
>
> struct S { char a; short b; char c; } s1[24], s2[24];
> struct T { char a; long long b; char c; struct S d[3]; long long e; char f;
> } t1, t2;
> --- gcc/testsuite/c-c++-common/torture/builtin-clear-padding-6.c.jj
> 2024-07-16 16:55:10.331460214 +0200
> +++ gcc/testsuite/c-c++-common/torture/builtin-clear-padding-6.c
> 2024-07-16 17:06:56.940508833 +0200
> @@ -0,0 +1,28 @@
> +/* PR middle-end/115527 */
> +/* { dg-do run } */
> +
> +struct T { struct S { double a; signed char b; long c; } d[3]; int e; } t1,
> t2;
> +
> +__attribute__((noipa)) void
> +foo (struct T *t)
> +{
> + for (int i = 0; i < 3; ++i)
> + {
> + t->d[i].a = 1. + 3 * i;
> + t->d[i].b = 2 + 3 * i;
> + t->d[i].c = 3 + 3 * i;
> + }
> + t->e = 10;
> +}
> +
> +int
> +main ()
> +{
> + __builtin_memset (&t2, -1, sizeof (t2));
> + foo (&t1);
> + foo (&t2);
> + __builtin_clear_padding (&t2);
> + if (__builtin_memcmp (&t1, &t2, sizeof (t1)))
> + __builtin_abort ();
> + return 0;
> +}
>
> Jakub
>
>
--
Richard Biener <rguent...@suse.de>
SUSE Software Solutions Germany GmbH,
Frankenstrasse 146, 90461 Nuernberg, Germany;
GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)
