Re: [PATCH] Fix ms_struct/-mms-bitfields structure layout (PR target/52991)

[JonY]

On 02/28/2018 12:26 AM, Jakub Jelinek wrote:
> Hi!
> 
> The following patch fixes the reported ms_struct/-mms-bitfields structure
> layout issues from PR52991.
> 
> There are multiple issues, two of them introduced by the
> https://gcc.gnu.org/ml/gcc-patches/2006-04/msg01064.html -mms-bitfields
> revamp from Eric and follow-up fix r114552, the rest has been introduced
> later when the known_align < desired_align case has been enabled for the ms
> bitfield layout.
> 
> The first 2 hunks fix alignment of packed non-bitfield fields, we can't
> ignore all the alignment updates for them, just should use only
> desired_align which takes DECL_PACKED into account, rather than
> MAX (type_align, desired_align).  Similarly, the last hunk in stor-layout.c
> makes sure that for DECL_PACKED fields we use BITS_PER_UNIT alignment rather
> than the type alignment.
> 
> The rest attempts to unbreak r184409 which enabled known_align < desired_align
> case; doing that if rli->prev_field and ms layout is wrong, we first need to
> deal with the bitfield packing and if we are within a bitfield word, we
> shouldn't do any realignment, only in between them.
> 
> The patch reverts changes to bf-ms-layout{,-2}.c tests done in 2012, which
> were done just to match the r184409 changes, and adds 2 new tests.  All of
> these 4 I've tested (slightly tweaked, so that it compiles with VC) with
> the online VC compiler http://rextester.com/l/c_online_compiler_visual .
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> 

No objections.





signature.asc
Description: OpenPGP digital signature

Re: [PATCH] Fix ms_struct/-mms-bitfields structure layout (PR target/52991)

[Kai Tietz]

Hello Jakub,

I can't approve this patch, but I can confirm that changes are
sensible.  I verified the structure layout - as far as possible - also
with MS' compiler, and can confirm the adjustments.

Cheers,
Ka

2018-02-28 1:26 GMT+01:00 Jakub Jelinek :
> Hi!
>
> The following patch fixes the reported ms_struct/-mms-bitfields structure
> layout issues from PR52991.
>
> There are multiple issues, two of them introduced by the
> https://gcc.gnu.org/ml/gcc-patches/2006-04/msg01064.html -mms-bitfields
> revamp from Eric and follow-up fix r114552, the rest has been introduced
> later when the known_align < desired_align case has been enabled for the ms
> bitfield layout.
>
> The first 2 hunks fix alignment of packed non-bitfield fields, we can't
> ignore all the alignment updates for them, just should use only
> desired_align which takes DECL_PACKED into account, rather than
> MAX (type_align, desired_align).  Similarly, the last hunk in stor-layout.c
> makes sure that for DECL_PACKED fields we use BITS_PER_UNIT alignment rather
> than the type alignment.
>
> The rest attempts to unbreak r184409 which enabled known_align < desired_align
> case; doing that if rli->prev_field and ms layout is wrong, we first need to
> deal with the bitfield packing and if we are within a bitfield word, we
> shouldn't do any realignment, only in between them.
>
> The patch reverts changes to bf-ms-layout{,-2}.c tests done in 2012, which
> were done just to match the r184409 changes, and adds 2 new tests.  All of
> these 4 I've tested (slightly tweaked, so that it compiles with VC) with
> the online VC compiler http://rextester.com/l/c_online_compiler_visual .
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> 2018-02-27  Jakub Jelinek  
>
> PR target/52991
> * stor-layout.c (update_alignment_for_field): For
> targetm.ms_bitfield_layout_p (rli->t), if !is_bitfield
> && !DECL_PACKED (field), do the alignment update, just use
> only desired_align instead of MAX (type_align, desired_align)
> as the alignment.
> (place_field): Don't do known_align < desired_align handling
> early if targetm.ms_bitfield_layout_p (rli->t) and rli->prev_field
> is non-NULL, instead do it after rli->prev_field handling and
> only if not within a bitfield word.  For DECL_PACKED (field)
> use type_align of BITS_PER_UNIT.
>
> * gcc.dg/bf-ms-layout.c: Revert 2012-04-26 changes.
> * gcc.dg/bf-ms-layout-2.c: Revert 2012-02-23 changes.
> * gcc.dg/bf-ms-layout-4.c: New test.
> * gcc.dg/bf-ms-layout-5.c: New test.
>
> --- gcc/stor-layout.c.jj2018-02-22 14:35:33.135216198 +0100
> +++ gcc/stor-layout.c   2018-02-27 18:56:26.906494801 +0100
> @@ -1038,7 +1038,7 @@ update_alignment_for_field (record_layou
>  the type, except that for zero-size bitfields this only
>  applies if there was an immediately prior, nonzero-size
>  bitfield.  (That's the way it is, experimentally.) */
> -  if ((!is_bitfield && !DECL_PACKED (field))
> +  if (!is_bitfield
>   || ((DECL_SIZE (field) == NULL_TREE
>|| !integer_zerop (DECL_SIZE (field)))
>   ? !DECL_PACKED (field)
> @@ -1047,7 +1047,10 @@ update_alignment_for_field (record_layou
>  && ! integer_zerop (DECL_SIZE (rli->prev_field)
> {
>   unsigned int type_align = TYPE_ALIGN (type);
> - type_align = MAX (type_align, desired_align);
> + if (!is_bitfield && DECL_PACKED (field))
> +   type_align = desired_align;
> + else
> +   type_align = MAX (type_align, desired_align);
>   if (maximum_field_alignment != 0)
> type_align = MIN (type_align, maximum_field_alignment);
>   rli->record_align = MAX (rli->record_align, type_align);
> @@ -1303,7 +1306,9 @@ place_field (record_layout_info rli, tre
>
>/* Does this field automatically have alignment it needs by virtue
>   of the fields that precede it and the record's own alignment?  */
> -  if (known_align < desired_align)
> +  if (known_align < desired_align
> +  && (! targetm.ms_bitfield_layout_p (rli->t)
> + || rli->prev_field == NULL))
>  {
>/* No, we need to skip space before this field.
>  Bump the cumulative size to multiple of field alignment.  */
> @@ -1331,8 +1336,6 @@ place_field (record_layout_info rli, tre
>
>if (! TREE_CONSTANT (rli->offset))
> rli->offset_align = desired_align;
> -  if (targetm.ms_bitfield_layout_p (rli->t))
> -   rli->prev_field = NULL;
>  }
>
>/* Handle compatibility with PCC.  Note that if the record has any
> @@ -1448,6 +1451,8 @@ place_field (record_layout_info rli, tre
>/* This is a bitfield if it exists.  */
>if (rli->prev_field)
> {
> + bool realign_p = known_align < desired_align;
> +
>   

[PATCH] Fix ms_struct/-mms-bitfields structure layout (PR target/52991)

[Jakub Jelinek]

Hi!

The following patch fixes the reported ms_struct/-mms-bitfields structure
layout issues from PR52991.

There are multiple issues, two of them introduced by the
https://gcc.gnu.org/ml/gcc-patches/2006-04/msg01064.html -mms-bitfields
revamp from Eric and follow-up fix r114552, the rest has been introduced
later when the known_align < desired_align case has been enabled for the ms
bitfield layout.

The first 2 hunks fix alignment of packed non-bitfield fields, we can't
ignore all the alignment updates for them, just should use only
desired_align which takes DECL_PACKED into account, rather than
MAX (type_align, desired_align).  Similarly, the last hunk in stor-layout.c
makes sure that for DECL_PACKED fields we use BITS_PER_UNIT alignment rather
than the type alignment.

The rest attempts to unbreak r184409 which enabled known_align < desired_align
case; doing that if rli->prev_field and ms layout is wrong, we first need to
deal with the bitfield packing and if we are within a bitfield word, we
shouldn't do any realignment, only in between them.

The patch reverts changes to bf-ms-layout{,-2}.c tests done in 2012, which
were done just to match the r184409 changes, and adds 2 new tests.  All of
these 4 I've tested (slightly tweaked, so that it compiles with VC) with
the online VC compiler http://rextester.com/l/c_online_compiler_visual .

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2018-02-27  Jakub Jelinek  

PR target/52991
* stor-layout.c (update_alignment_for_field): For
targetm.ms_bitfield_layout_p (rli->t), if !is_bitfield
&& !DECL_PACKED (field), do the alignment update, just use
only desired_align instead of MAX (type_align, desired_align)
as the alignment.
(place_field): Don't do known_align < desired_align handling
early if targetm.ms_bitfield_layout_p (rli->t) and rli->prev_field
is non-NULL, instead do it after rli->prev_field handling and
only if not within a bitfield word.  For DECL_PACKED (field)
use type_align of BITS_PER_UNIT.

* gcc.dg/bf-ms-layout.c: Revert 2012-04-26 changes.
* gcc.dg/bf-ms-layout-2.c: Revert 2012-02-23 changes.
* gcc.dg/bf-ms-layout-4.c: New test.
* gcc.dg/bf-ms-layout-5.c: New test.

--- gcc/stor-layout.c.jj2018-02-22 14:35:33.135216198 +0100
+++ gcc/stor-layout.c   2018-02-27 18:56:26.906494801 +0100
@@ -1038,7 +1038,7 @@ update_alignment_for_field (record_layou
 the type, except that for zero-size bitfields this only
 applies if there was an immediately prior, nonzero-size
 bitfield.  (That's the way it is, experimentally.) */
-  if ((!is_bitfield && !DECL_PACKED (field))
+  if (!is_bitfield
  || ((DECL_SIZE (field) == NULL_TREE
   || !integer_zerop (DECL_SIZE (field)))
  ? !DECL_PACKED (field)
@@ -1047,7 +1047,10 @@ update_alignment_for_field (record_layou
 && ! integer_zerop (DECL_SIZE (rli->prev_field)
{
  unsigned int type_align = TYPE_ALIGN (type);
- type_align = MAX (type_align, desired_align);
+ if (!is_bitfield && DECL_PACKED (field))
+   type_align = desired_align;
+ else
+   type_align = MAX (type_align, desired_align);
  if (maximum_field_alignment != 0)
type_align = MIN (type_align, maximum_field_alignment);
  rli->record_align = MAX (rli->record_align, type_align);
@@ -1303,7 +1306,9 @@ place_field (record_layout_info rli, tre
 
   /* Does this field automatically have alignment it needs by virtue
  of the fields that precede it and the record's own alignment?  */
-  if (known_align < desired_align)
+  if (known_align < desired_align
+  && (! targetm.ms_bitfield_layout_p (rli->t)
+ || rli->prev_field == NULL))
 {
   /* No, we need to skip space before this field.
 Bump the cumulative size to multiple of field alignment.  */
@@ -1331,8 +1336,6 @@ place_field (record_layout_info rli, tre
 
   if (! TREE_CONSTANT (rli->offset))
rli->offset_align = desired_align;
-  if (targetm.ms_bitfield_layout_p (rli->t))
-   rli->prev_field = NULL;
 }
 
   /* Handle compatibility with PCC.  Note that if the record has any
@@ -1448,6 +1451,8 @@ place_field (record_layout_info rli, tre
   /* This is a bitfield if it exists.  */
   if (rli->prev_field)
{
+ bool realign_p = known_align < desired_align;
+
  /* If both are bitfields, nonzero, and the same size, this is
 the middle of a run.  Zero declared size fields are special
 and handled as "end of run". (Note: it's nonzero declared
@@ -1481,7 +1486,10 @@ place_field (record_layout_info rli, tre
rli->remaining_in_alignment = typesize - bitsize;
}
  else
-   rli->remaining_in_alignment -= bitsize;
+   {
+