[Bug middle-end/97943] New: [11 Regression] ICE with __builtin_clear_padding on flexible array member

[Bug middle-end/97943] New: [11 Regression] ICE with __builtin_clear_padding on flexible array member

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97943

            Bug ID: 97943
           Summary: [11 Regression] ICE with __builtin_clear_padding on
                    flexible array member
           Product: gcc
           Version: 11.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: middle-end
          Assignee: unassigned at gcc dot gnu.org
          Reporter: jakub at gcc dot gnu.org
  Target Milestone: ---

struct S { int a; char b[] __attribute__((aligned (2 * sizeof (int)))); };

void
foo (struct S *u)
{
  __builtin_clear_padding (u);
}

ICEs starting with the __builtin_clear_padding implementation
r11-5196-g1bea0d0aa5936cb36b6f86f721ca03c1a1bb601d

Avoiding the ICE is easy, e.g.
--- gcc/gimple-fold.c.jj        2020-11-20 12:28:10.102680956 +0100
+++ gcc/gimple-fold.c   2020-11-22 21:54:39.527918551 +0100
@@ -4439,6 +4439,12 @@ clear_padding_type (clear_padding_struct
                      }
                  }
              }
+           else if (DECL_SIZE_UNIT (field) == NULL_TREE)
+             {
+               gcc_assert (TREE_CODE (TREE_TYPE (field)) == ARRAY_TYPE
+                           && !COMPLETE_TYPE_P (TREE_TYPE (field)));
+               /* Flexible array member.  Consider it all padding.  */
+             }
            else
              {
                HOST_WIDE_INT pos = int_byte_position (field);
the important question is what behavior we want in that case.
The above patch will make all the bits in the tail padding starting with the
address of the flexible array member till the end of the type in question
padding bits (plus the padding before the flexible array member but that is not
questionable).
The problem with that is that if the object does contain any flexible array
members, __builtin_clear_padding could then clear even the value representation
of those array elements.

Another approach would be to consider that the flexible array member contains
as many elements as can (even partially) fit into the type bounds.
So e.g. if we have:
struct S { char a; short b; char c; };
struct T { char a; struct S b __attribute__((aligned (32))); S c[]; };
the padding mask (0 for padding, 1 for unmodified value) for struct S is 0xff,
0, 0xff, 0xff, 0xff, 0
and for T it would be 0xff, 31x 0, (now is b field) 0xff, 0, 0xff, 0xff, 0xff,
0,
(now starts the flex array member and we 32-6 bytes in the tail padding), so we
repeat 0xff, 0, 0xff, 0xff, 0xff, 0 there 4 times (that fits in full) and then
0xff, 0 (first two bytes of the element that would cross the size of the T type
we were passed pointer to). 

Thoughts on this?

If we go the latter route, there are complications on what to do if a structure
with flexible array member is embedded into other structures or unions.
And in those it can go either in places where it is followed by other fields,
or no real fields after it.
So given the above struct S and T, if we have:
struct U { char a __attribute__((aligned (128))); struct T b; };
U's size is 256 bytes, would the padding be 0xff, 127x 0, then for T
0xff, 31x 0, then 16x ( 0xff, 0, 0xff, 0xff, 0xff, 0 ) , i.e. as if c[] was
actually c[15] in this case?
If there are fields after it, instead consider only what is the largest number
that would fit in before that next field without partial elements?

For the implementation, we'd probably need to pass down the size of the padding
before the next element (if any), or end of the type and for the flexible array
members compute from there how many elements and whether full or partial one
can fit.  If there following fields are bitfields, I have no idea, though I'd
say the elements shouldn't cross even unnamed bitfields.
struct V { struct T a; int : 30; int c : 2; };

[Bug middle-end/97943] [11 Regression] ICE with __builtin_clear_padding on flexible array member

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97943

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |NEW
   Target Milestone|---                         |11.0
                 CC|                            |redi at gcc dot gnu.org,
                   |                            |rodgertq at gcc dot gnu.org
   Last reconfirmed|                            |2020-11-23

[Bug middle-end/97943] [11 Regression] ICE with __builtin_clear_padding on flexible array member

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97943

--- Comment #1 from Richard Biener <rguenth at gcc dot gnu.org> ---
I think the only viable route is to _not_ clear the "padding" covered by
flexarray members.  We may never ever clear actual values and they, as you say,
could be actual values.

But ... can we even do a atomic operation on such "incomplete" type?  So,
isn't there a complete type w/o flexarray always available?

So ... sorry () here and reject an attempt to clear padding in an incomplete
type?

Which raises the question what to do for the real-world case which won't
use char b[] since that's not C++ but would use char b[1] ...

[Bug middle-end/97943] [11 Regression] ICE with __builtin_clear_padding on flexible array member

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97943

--- Comment #2 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #1)
> I think the only viable route is to _not_ clear the "padding" covered by
> flexarray members.  We may never ever clear actual values and they, as you
> say,
> could be actual values.
> 
> But ... can we even do a atomic operation on such "incomplete" type?  So,
> isn't there a complete type w/o flexarray always available?
> 
> So ... sorry () here and reject an attempt to clear padding in an incomplete
> type?
> 
> Which raises the question what to do for the real-world case which won't
> use char b[] since that's not C++ but would use char b[1] ...

To answer my own question, array_at_struct_end_p () covered padding must not be
cleared.

[Bug middle-end/97943] [11 Regression] ICE with __builtin_clear_padding on flexible array member

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97943

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jason at gcc dot gnu.org

--- Comment #3 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #2)
> > Which raises the question what to do for the real-world case which won't
> > use char b[] since that's not C++ but would use char b[1] ...
> 
> To answer my own question, array_at_struct_end_p () covered padding must not
> be cleared.

If we do that, we'll violate C++20 I'd think.
For flexible array members we can choose what exactly we want to do, because we
are outside of the standard.  But when one uses:
#include <atomic>
struct S { char a; short b; };
struct T { char a; char b alignas (32); S c[1]; };
std::atomic<T> t;
then we can't pretend one can store meaningful values in c[3].b and expect it
to work.

[Bug middle-end/97943] [11 Regression] ICE with __builtin_clear_padding on flexible array member

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97943

--- Comment #4 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
BTW, for all the 3 cases __builtin_clear_padding will be called on, i.e. the
constructor of std::atomic for the type, the desired argument passed by value
and the expected argument, which while it is passed by value is also stored by
the implementation (copied from the atomic memory), one can't effectively have
any flexible (or poor man's flexible) array members except those that fit into
the tail padding of the structures.  So in many cases nothing at all.
So for the flexible array members, if we don't sorry on them, we could also
clear the tail padding spot for partial elements, because one really can't have
there half of an element.

[Bug middle-end/97943] [11 Regression] ICE with __builtin_clear_padding on flexible array member

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97943

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
(In reply to Jakub Jelinek from comment #4)
> value and the expected argument, which while it is passed by value is also

Sorry, meant passed by reference.

[Bug middle-end/97943] [11 Regression] ICE with __builtin_clear_padding on flexible array member

[jason at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97943

--- Comment #6 from Jason Merrill <jason at gcc dot gnu.org> ---
I think we should reject trying to clear the padding of a flexible/zero-length
array, with error rather than sorry.  And handle an array at the end of a
struct like any other array.  Nobody should be using this builtin with the
struct hack, it's impossible for it to do anything sensible.

[Bug middle-end/97943] [11 Regression] ICE with __builtin_clear_padding on flexible array member

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97943

--- Comment #7 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Jason Merrill from comment #6)
> I think we should reject trying to clear the padding of a
> flexible/zero-length array, with error rather than sorry.  And handle an
> array at the end of a struct like any other array.  Nobody should be using
> this builtin with the struct hack, it's impossible for it to do anything
> sensible.

I agree. In the unlikely scenario somebody comes up with a good use, we can
relax it later. If we allow it now somebody will find a way to depend on it
"working" and then it's harder to remove later.

[Bug middle-end/97943] [11 Regression] ICE with __builtin_clear_padding on flexible array member

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97943

--- Comment #8 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:a7285c8659689e9ade53fcb996b26d874455a4f3

commit r11-5329-ga7285c8659689e9ade53fcb996b26d874455a4f3
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Wed Nov 25 10:37:58 2020 +0100

    middle-end: Reject flexible array members in __builtin_clear_padding
[PR97943]

    As mentioned in the PR, we currently ICE on flexible array members in
    structs and unions during __builtin_clear_padding processing.

    Jason said in the PR he'd prefer an error in these cases over forcefully
    handling it as [0] arrays (everything is padding then) or consider the
    arrays to have as many whole elements as would fit into the tail padding.

    So, this patch implements that.

    2020-11-25  Jakub Jelinek  <jakub@redhat.com>

            PR middle-end/97943
            * gimple-fold.c (clear_padding_union, clear_padding_type): Error on
and
            ignore flexible array member fields.  Ignore fields with
            error_mark_node type.

            * c-c++-common/builtin-clear-padding-2.c: New test.
            * c-c++-common/builtin-clear-padding-3.c: New test.
            * g++.dg/ext/builtin-clear-padding-1.C: New test.
            * gcc.dg/builtin-clear-padding-2.c: New test.

[Bug middle-end/97943] [11 Regression] ICE with __builtin_clear_padding on flexible array member

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97943

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|NEW                         |RESOLVED

--- Comment #9 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Fixed.