Bug 43798 - [9/10/11/12 Regression] attribute((aligned(x))) not honored for array element types?
Summary: [9/10/11/12 Regression] attribute((aligned(x))) not honored for array element...
Status: NEW
Alias: None
Product: gcc
Classification: Unclassified
Component: c (show other bugs)
Version: 4.6.0
: P2 normal
Target Milestone: 9.5
Assignee: Not yet assigned to anyone
URL:
Keywords: documentation, wrong-code
: 89769 (view as bug list)
Depends on:
Blocks:
 
Reported: 2010-04-19 12:59 UTC by Richard Biener
Modified: 2021-06-01 08:04 UTC (History)
6 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2019-03-20 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Richard Biener 2010-04-19 12:59:14 UTC
in libbid we have

typedef __attribute__((aligned(16)))
struct {
    unsigned long long w[3];
} UINT192;

UINT192 bid_Kx192[32];

thus we request 16-byte alignment for UINT192 (whose elements add
up to a size of 24).  Now the array ends up with elements of size 24
and thus the elements are _not_ aligned to a 16-byte boundary, still
the element type is not adjusted to reflect that leading to inconsistencies
when one for example tries to set operand 3 of an ARRAY_REF
(which is specified in units of the alignment of the element).

This causes PR43783.

This is also at least a documentation bug as I can't find anything
that documents the above behavior.
Comment 1 Richard Biener 2010-04-19 13:05:03 UTC
Namely the bid_Kx192 decl looks like

 <var_decl 0x7ffff5af8000 bid_Kx192
    type <array_type 0x7ffff5add7e0
        type <record_type 0x7ffff5add690 UINT192 type_0 BLK
            size <integer_cst 0x7ffff7ef70c8 constant 192>
            unit size <integer_cst 0x7ffff7ef7078 constant 24>
            user align 128 symtab 0 alias set -1 canonical type 0x7ffff5add498 fields <field_decl 0x7ffff7fb34c0 w> context <translation_unit_decl 0x7ffff5afe000 D.1631>
            pointer_to_this <pointer_type 0x7ffff5add888>>
        BLK
        size <integer_cst 0x7ffff7ef7258 constant 6144>
        unit size <integer_cst 0x7ffff7fcfaa0 constant 768>
        user align 128 symtab 0 alias set -1 canonical type 0x7ffff5add9d8
        domain <integer_type 0x7ffff5add738>
        pointer_to_this <pointer_type 0x7ffff5afc000>>
    addressable used public static common BLK file t.c line 6 col 9 size <integer_cst 0x7ffff7ef7258 6144> unit size <integer_cst 0x7ffff7fcfaa0 768>
    align 256 chain <function_decl 0x7ffff5adcb00 main>>

where the TYPE_ALIGN of the element type only applies to the first
array element.  If you look at expr.c:array_ref_element_size then you
can see that there doesn't exist a valid TREE_OPERAND (array-ref, 3)
for indexing the above array as its TYPE_ALIGN_UNIT is bigger than
the aligned-size.

IMHO the C frontend ought to generate a variant type for the element
type that has its alignment adjusted (or it shall follow the users
request and add padding between the elements?).  The current situation
is unfortunate for the middle-end.
Comment 2 Andreas Schwab 2010-04-19 13:28:32 UTC
An array cannot have internal padding, so the padding needs to be added to the element type.
The attempt to define such an array should probably be rejected.
Comment 3 Jakub Jelinek 2010-04-19 13:44:06 UTC
At least with pointers alignment greater than size of the pointed to type (or not divisible by it) is often used to say that the start of the array is aligned some way.
Comment 4 rguenther@suse.de 2010-04-19 13:57:09 UTC
Subject: Re:  attribute((aligned(x))) not honored for array
 element types?

On Mon, 19 Apr 2010, jakub at gcc dot gnu dot org wrote:

> ------- Comment #3 from jakub at gcc dot gnu dot org  2010-04-19 13:44 -------
> At least with pointers alignment greater than size of the pointed to type (or
> not divisible by it) is often used to say that the start of the array is
> aligned some way.

Yes, we correctly copy the (over-)alignment of the element type to
the array type.  But the element type alignment then stays "wrong".

Richard.
Comment 5 Richard Biener 2019-03-20 08:09:57 UTC
*** Bug 89769 has been marked as a duplicate of this bug. ***
Comment 6 Richard Biener 2019-03-20 08:11:55 UTC
Manifests itself as regression now (see duplicate).  If C doesn't allow padding between elements of an array then the C frontend has to adjust the element type when building the array type.  At least we cannot leave the bogus alignment in place.
Comment 7 Richard Biener 2019-03-20 08:13:38 UTC
Joseph, can we reject the testcase?

OTOH a 16 aligned struct UINT192 should have size 32 (but appearantly it does not when using a typedef because of sharing of TYPE_FIELDs and friends?)
Comment 8 Jakub Jelinek 2019-03-20 08:39:03 UTC
Note, since r105095 - https://gcc.gnu.org/ml/gcc-patches/2005-10/msg00252.html we reject a subset of this (when the element size is smaller than the element alignment).
So, either we extend that to also when the element size is not divisible by the element size, or silently replace in layout_type? the element type with a variant 
thereof that has smaller alignment.
Comment 9 rguenther@suse.de 2019-03-20 08:42:57 UTC
On Wed, 20 Mar 2019, jakub at gcc dot gnu.org wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=43798
> 
> Jakub Jelinek <jakub at gcc dot gnu.org> changed:
> 
>            What    |Removed                     |Added
> ----------------------------------------------------------------------------
>                  CC|                            |jakub at gcc dot gnu.org
> 
> --- Comment #8 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
> Note, since r105095 - https://gcc.gnu.org/ml/gcc-patches/2005-10/msg00252.html
> we reject a subset of this (when the element size is smaller than the element
> alignment).
> So, either we extend that to also when the element size is not divisible by the
> element size, or silently replace in layout_type? the element type with a
> variant 
> thereof that has smaller alignment.

I think we should reject since if the user writes it that way he
doesn't expect that

UINT192 *ptr = &bid_Kx192[i];

converts between pointed-to types of different alignment.

That is, if we fix up we IMHO need to pad.  And I do wonder why
sizeof (UINT192) is 24 and not 32 (is it?).
Comment 10 Jakub Jelinek 2019-03-20 09:02:59 UTC
We can't pad, that is against the ABI.
I think we've discussed this many times in the past, the fact is that people do use this kind of mess when they want to say that the whole array has certain alignment or when they are just careless, I think it is too late now for GCC9 to start rejecting it without an analysis how much code would be affected, not to mention that it is not backportable.

The following patch does the adjustment and fixes the testcase:
--- gcc/stor-layout.c.jj	2019-01-01 12:37:17.296972670 +0100
+++ gcc/stor-layout.c	2019-03-20 09:56:15.576407411 +0100
@@ -2550,10 +2550,27 @@ layout_type (tree type)
 	    /* If TYPE_SIZE_UNIT overflowed, then it is certainly larger than
 	       TYPE_ALIGN_UNIT.  */
 	    && !TREE_OVERFLOW (TYPE_SIZE_UNIT (element))
-	    && !integer_zerop (TYPE_SIZE_UNIT (element))
-	    && compare_tree_int (TYPE_SIZE_UNIT (element),
-			  	 TYPE_ALIGN_UNIT (element)) < 0)
-	  error ("alignment of array elements is greater than element size");
+	    && !integer_zerop (TYPE_SIZE_UNIT (element)))
+	  {
+	    if (compare_tree_int (TYPE_SIZE_UNIT (element),
+				  TYPE_ALIGN_UNIT (element)) < 0)
+	      error ("alignment of array elements is greater than "
+		     "element size");
+	    if (TYPE_ALIGN_UNIT (element))
+	      {
+		unsigned align = (TREE_INT_CST_LOW (TYPE_SIZE_UNIT (element))
+				  & (TYPE_ALIGN_UNIT (element) - 1));
+		align = least_bit_hwi (align);
+		if (align && align != TYPE_ALIGN_UNIT (element))
+		  {
+		    element = build_variant_type_copy (element);
+		    SET_TYPE_ALIGN (element, align * BITS_PER_UNIT);
+		    TYPE_USER_ALIGN (element) = 1;
+		    TREE_TYPE (type) = element;
+		    TYPE_USER_ALIGN (type) = 1;
+		  }
+	      }
+	  }
 	break;
       }
 
but I haven't done any testing on it beyond that.
Perhaps for GCC 9 we could warn when we do this and depending on the amount of warnings consider rejecting it later on if it doesn't affect significant amount of code?
Comment 11 Jakub Jelinek 2019-03-20 09:05:14 UTC
Maybe the TREE_OVERFLOW check should be done only for the error, even with TREE_OVERFLOW we should be able to check the low bits of the size.
Comment 12 rguenther@suse.de 2019-03-20 09:34:15 UTC
On Wed, 20 Mar 2019, jakub at gcc dot gnu.org wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=43798
> 
> --- Comment #10 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
> We can't pad, that is against the ABI.
> I think we've discussed this many times in the past, the fact is that people do
> use this kind of mess when they want to say that the whole array has certain
> alignment or when they are just careless, I think it is too late now for GCC9
> to start rejecting it without an analysis how much code would be affected, not
> to mention that it is not backportable.
> 
> The following patch does the adjustment and fixes the testcase:
> --- gcc/stor-layout.c.jj        2019-01-01 12:37:17.296972670 +0100
> +++ gcc/stor-layout.c   2019-03-20 09:56:15.576407411 +0100
> @@ -2550,10 +2550,27 @@ layout_type (tree type)
>             /* If TYPE_SIZE_UNIT overflowed, then it is certainly larger than
>                TYPE_ALIGN_UNIT.  */
>             && !TREE_OVERFLOW (TYPE_SIZE_UNIT (element))
> -           && !integer_zerop (TYPE_SIZE_UNIT (element))
> -           && compare_tree_int (TYPE_SIZE_UNIT (element),
> -                                TYPE_ALIGN_UNIT (element)) < 0)

I believe this check was just bogus since size < align catches just
very few cases of small objects.  Thus the case in question now isn't
really different from the intent of the above.  Shouldn't we just
change this to

             && !multiple_of_p (sizetype, TYPE_SIZE_UNIT (element),
				TYPE_ALIGN_UNIT (element))

and adjust the error message accordingly?

> -         error ("alignment of array elements is greater than element size");
> +           && !integer_zerop (TYPE_SIZE_UNIT (element)))
> +         {
> +           if (compare_tree_int (TYPE_SIZE_UNIT (element),
> +                                 TYPE_ALIGN_UNIT (element)) < 0)
> +             error ("alignment of array elements is greater than "
> +                    "element size");
> +           if (TYPE_ALIGN_UNIT (element))
> +             {
> +               unsigned align = (TREE_INT_CST_LOW (TYPE_SIZE_UNIT (element))
> +                                 & (TYPE_ALIGN_UNIT (element) - 1));
> +               align = least_bit_hwi (align);
> +               if (align && align != TYPE_ALIGN_UNIT (element))
> +                 {
> +                   element = build_variant_type_copy (element);
> +                   SET_TYPE_ALIGN (element, align * BITS_PER_UNIT);
> +                   TYPE_USER_ALIGN (element) = 1;
> +                   TREE_TYPE (type) = element;
> +                   TYPE_USER_ALIGN (type) = 1;
> +                 }
> +             }
> +         }
>         break;
>        }
> 
> but I haven't done any testing on it beyond that.
> Perhaps for GCC 9 we could warn when we do this and depending on the amount of
> warnings consider rejecting it later on if it doesn't affect significant amount
> of code?
> 
>
Comment 13 Jakub Jelinek 2019-03-20 10:06:19 UTC
See above, I'm afraid that would break a lot of code in the wild.  We've been accepting it for way too long and handling the way those users expect (i.e. that the whole array is 16 byte aligned), clang accepts it too that way (without any warnings), so does icc.
Comment 14 Jakub Jelinek 2019-03-20 10:17:57 UTC
Acjusted patch that uses build_aligned_type instead of doing it on its own.
--- gcc/stor-layout.c.jj	2019-01-01 12:37:17.296972670 +0100
+++ gcc/stor-layout.c	2019-03-20 11:15:12.166935041 +0100
@@ -2547,13 +2547,32 @@ layout_type (tree type)
 	   large as the element alignment.  */
 	if (TYPE_SIZE_UNIT (element)
 	    && TREE_CODE (TYPE_SIZE_UNIT (element)) == INTEGER_CST
+	    && !integer_zerop (TYPE_SIZE_UNIT (element)))
+	  {
 	    /* If TYPE_SIZE_UNIT overflowed, then it is certainly larger than
 	       TYPE_ALIGN_UNIT.  */
-	    && !TREE_OVERFLOW (TYPE_SIZE_UNIT (element))
-	    && !integer_zerop (TYPE_SIZE_UNIT (element))
-	    && compare_tree_int (TYPE_SIZE_UNIT (element),
-			  	 TYPE_ALIGN_UNIT (element)) < 0)
-	  error ("alignment of array elements is greater than element size");
+	    if (!TREE_OVERFLOW (TYPE_SIZE_UNIT (element))
+	        && compare_tree_int (TYPE_SIZE_UNIT (element),
+				     TYPE_ALIGN_UNIT (element)) < 0)
+	      error ("alignment of array elements is greater than "
+		     "element size");
+	    if (TYPE_ALIGN_UNIT (element))
+	      {
+		unsigned align = (TREE_INT_CST_LOW (TYPE_SIZE_UNIT (element))
+				  & (TYPE_ALIGN_UNIT (element) - 1));
+		align = least_bit_hwi (align);
+		if (align && align != TYPE_ALIGN_UNIT (element))
+		  {
+		    /* If the size of the element is not a multiple of
+		       alignment, lower the alignment of the elements of the
+		       array, but keep the whole array alignment.  */
+		    element
+		      = build_aligned_type (element, align * BITS_PER_UNIT);
+		    TREE_TYPE (type) = element;
+		    TYPE_USER_ALIGN (type) = 1;
+		  }
+	      }
+	  }
 	break;
       }
Comment 15 Alexander Monakov 2019-03-20 14:56:11 UTC
Not 100% sure if it's known, the comments don't seem to mention this, so I'll note for completeness sake:

placement of the attribute makes a difference, here it looks like the attribute on the type introduced via a typedef leads to weird semantics, while the attribute on the struct itself leads to logical 32/16 size/align:

typedef struct __attribute__((aligned(16))) {
    unsigned long long w[3];
} UINT192;

int a1 = __alignof(UINT192);
int s1 = sizeof(UINT192);

a1:
        .long   16
s1:
        .long   32


Also one cannot create a 64-bit aligned/128-bit sized typedef to 'long double' on 32-bit:

typedef long double T __attribute__((aligned(8)));

int a1 = __alignof(T);
int s1 = sizeof(T);

a1:
        .long   8
s1:
        .long   12
Comment 16 Richard Biener 2019-03-20 15:02:31 UTC
IMHO if we silently lower alignment of UINT192 as array elements we
should at least warn for

typedef __attribute__((aligned(16)))
struct {
    unsigned long long w[3];
} UINT192;

UINT192 bid_Kx192[32];


like

  UINT192 bid_Kx192[32];
  ^^^^^^^
warning: alignment of array element type lowered to 8
note: accesses via UINT192 * might generate wrong code

which IMHO is a reason to reject this.  Can the bogus situation be reproduced
with C _Alignas?
Comment 17 Alexander Monakov 2019-03-20 15:15:53 UTC
> Can the bogus situation be reproduced with C _Alignas?

C11 does not allow _Alignas on typedefs, so don't see how; likewise for alignas in C++11.
Comment 18 Zdenek Sojka 2019-04-30 05:56:55 UTC
From my (of course limited) experience, such behavior is unexpected by the user. The user really wants:

typedef
struct  __attribute__((aligned(16))) {
    unsigned long long w[3];
} UINT192;

OR

typedef
struct {
    unsigned long long w[3];
} __attribute__((aligned(16))) UINT192;

BUT if he writes

typedef __attribute__((aligned(16)))
struct {
    unsigned long long w[3];
} UINT192;

OR

typedef
struct {
    unsigned long long w[3];
} UINT192 __attribute__((aligned(16)));

he then gets this broken code (array elements not aligned to 16).



Currently, you get an error for:

typedef __attribute__((__aligned__(64)))
struct {
    int a;
} SMALL ;

SMALL s[2];

$ gcc tst.c
tst.c:64:1: error: alignment of array elements is greater than element size
 SMALL s[2];
 ^~~~~

BUT not for:

typedef __attribute__((__aligned__(64)))
struct {
    int a[100];
} BIG;

BIG b[2];

even though this is leads to the same kind bugs as using SMALL.
(b[1] is not aligned to 64)
_Alignof(BIG)=64, sizeof(BIG)=400
_Alignof(b)=64, sizeof(b)=832 (why?)

I think a warning for declaration of both SMALL and BIG, or at least when declaring "BIG b[2]", would be reasonable.
Comment 19 Jakub Jelinek 2020-03-04 09:44:11 UTC
GCC 8.4.0 has been released, adjusting target milestone.
Comment 20 Jakub Jelinek 2021-05-14 09:46:11 UTC
GCC 8 branch is being closed.
Comment 21 Richard Biener 2021-06-01 08:04:53 UTC
GCC 9.4 is being released, retargeting bugs to GCC 9.5.