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 <ja...@redhat.com>: > 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 <ja...@redhat.com> > > 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.jj 2018-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; > + { > + rli->remaining_in_alignment -= bitsize; > + realign_p = false; > + } > } > else > { > @@ -1512,6 +1520,31 @@ place_field (record_layout_info rli, tre > rli->prev_field = NULL; > } > > + /* Does this field automatically have alignment it needs by virtue > + of the fields that precede it and the record's own alignment? */ > + if (realign_p) > + { > + /* If the alignment is still within offset_align, just align > + the bit position. */ > + if (desired_align < rli->offset_align) > + rli->bitpos = round_up (rli->bitpos, desired_align); > + else > + { > + /* First adjust OFFSET by the partial bits, then align. */ > + tree d = size_binop (CEIL_DIV_EXPR, rli->bitpos, > + bitsize_unit_node); > + rli->offset = size_binop (PLUS_EXPR, rli->offset, > + fold_convert (sizetype, d)); > + rli->bitpos = bitsize_zero_node; > + > + rli->offset = round_up (rli->offset, > + desired_align / BITS_PER_UNIT); > + } > + > + if (! TREE_CONSTANT (rli->offset)) > + rli->offset_align = desired_align; > + } > + > normalize_rli (rli); > } > > @@ -1530,7 +1563,7 @@ place_field (record_layout_info rli, tre > if (!DECL_BIT_FIELD_TYPE (field) > || (prev_saved != NULL > ? !simple_cst_equal (TYPE_SIZE (type), TYPE_SIZE (prev_type)) > - : !integer_zerop (DECL_SIZE (field)) )) > + : !integer_zerop (DECL_SIZE (field)))) > { > /* Never smaller than a byte for compatibility. */ > unsigned int type_align = BITS_PER_UNIT; > @@ -1555,7 +1588,8 @@ place_field (record_layout_info rli, tre > } > > /* Now align (conventionally) for the new type. */ > - type_align = TYPE_ALIGN (TREE_TYPE (field)); > + if (! DECL_PACKED (field)) > + type_align = TYPE_ALIGN (TREE_TYPE (field)); > > if (maximum_field_alignment != 0) > type_align = MIN (type_align, maximum_field_alignment); > --- gcc/testsuite/gcc.dg/bf-ms-layout.c.jj 2017-06-20 21:38:01.634906200 > +0200 > +++ gcc/testsuite/gcc.dg/bf-ms-layout.c 2018-02-27 18:04:06.923851839 +0100 > @@ -153,27 +153,27 @@ int main(){ > struct ten test_ten; > > #if defined (_TEST_MS_LAYOUT) || defined (_MSC_VER) > - size_t exp_sizeof_one = 8; > - size_t exp_sizeof_two = 12; > + size_t exp_sizeof_one = 12; > + size_t exp_sizeof_two = 16; > size_t exp_sizeof_three =6; > size_t exp_sizeof_four = 8; > size_t exp_sizeof_five = 3; > size_t exp_sizeof_six = 8; > size_t exp_sizeof_seven = 3; > - size_t exp_sizeof_eight = 2; > + size_t exp_sizeof_eight = 4; > size_t exp_sizeof_nine = 8; > - size_t exp_sizeof_ten = 8; > + size_t exp_sizeof_ten = 16; > > - unsigned char exp_one_c = 7; > - unsigned char exp_two_c = 9; > + unsigned char exp_one_c = 8; > + unsigned char exp_two_c = 12; > unsigned char exp_three_c = 4; > unsigned char exp_four_c = 4; > char exp_five_c = 2; > char exp_six_c = 5; > char exp_seven_c = 2; > - char exp_eight_c = 1; > + char exp_eight_c = 2; > char exp_nine_c = 0; > - char exp_ten_c = 1; > + char exp_ten_c = 8; > > #else /* testing -mno-ms-bitfields */ > > --- gcc/testsuite/gcc.dg/bf-ms-layout-2.c.jj 2017-06-20 21:38:02.112900704 > +0200 > +++ gcc/testsuite/gcc.dg/bf-ms-layout-2.c 2018-02-27 18:04:06.923851839 > +0100 > @@ -158,27 +158,27 @@ int main(){ > struct ten test_ten; > > #if defined (_TEST_MS_LAYOUT) || defined (_MSC_VER) > - size_t exp_sizeof_one = 8; > - size_t exp_sizeof_two = 12; > + size_t exp_sizeof_one = 12; > + size_t exp_sizeof_two = 16; > size_t exp_sizeof_three =6; > size_t exp_sizeof_four = 8; > size_t exp_sizeof_five = 3; > size_t exp_sizeof_six = 8; > size_t exp_sizeof_seven = 3; > - size_t exp_sizeof_eight = 2; > + size_t exp_sizeof_eight = 4; > size_t exp_sizeof_nine = 8; > - size_t exp_sizeof_ten = 8; > + size_t exp_sizeof_ten = 16; > > - unsigned char exp_one_c = 7; > - unsigned char exp_two_c = 9; > + unsigned char exp_one_c = 8; > + unsigned char exp_two_c = 12; > unsigned char exp_three_c = 4; > unsigned char exp_four_c = 4; > char exp_five_c = 2; > char exp_six_c = 5; > char exp_seven_c = 2; > - char exp_eight_c = 1; > + char exp_eight_c = 2; > char exp_nine_c = 0; > - char exp_ten_c = 1; > + char exp_ten_c = 8; > > #else /* testing -mno-ms-bitfields */ > > --- gcc/testsuite/gcc.dg/bf-ms-layout-4.c.jj 2018-02-27 18:09:09.544421580 > +0100 > +++ gcc/testsuite/gcc.dg/bf-ms-layout-4.c 2018-02-27 18:18:00.845039925 > +0100 > @@ -0,0 +1,43 @@ > +/* PR target/52991 */ > +/* { dg-do compile { target i?86-*-* x86_64-*-* } } */ > + > +#define CHECK(expr) extern char c[(expr) ? 1 : -1] > +#define offsetof(x, y) __builtin_offsetof (x, y) > + > +struct test_sp1 { > + int a; > + short b; > + int c; > + char d; > +} __attribute__((packed,ms_struct)); > + > +CHECK (sizeof (struct test_sp1) == 11); > +CHECK (offsetof (struct test_sp1, a) == 0); > +CHECK (offsetof (struct test_sp1, b) == 4); > +CHECK (offsetof (struct test_sp1, c) == 6); > +CHECK (offsetof (struct test_sp1, d) == 10); > + > +struct test_sp3 { > + int a; > + short b __attribute__((aligned(8))); > + int c; > + char d; > +} __attribute__((packed,ms_struct)); > + > +CHECK (sizeof (struct test_sp3) == 16); > +CHECK (offsetof (struct test_sp3, a) == 0); > +CHECK (offsetof (struct test_sp3, b) == 8); > +CHECK (offsetof (struct test_sp3, c) == 10); > +CHECK (offsetof (struct test_sp3, d) == 14); > + > +struct test_s4 { > + int a; > + short b; > + int c:15; > + char d; > +} __attribute__((ms_struct)); > + > +CHECK (sizeof (struct test_s4) == 16); > +CHECK (offsetof (struct test_s4, a) == 0); > +CHECK (offsetof (struct test_s4, b) == 4); > +CHECK (offsetof (struct test_s4, d) == 12); > --- gcc/testsuite/gcc.dg/bf-ms-layout-5.c.jj 2018-02-27 18:31:24.043753173 > +0100 > +++ gcc/testsuite/gcc.dg/bf-ms-layout-5.c 2018-02-27 18:35:29.825676223 > +0100 > @@ -0,0 +1,45 @@ > +/* PR target/52991 */ > +/* { dg-do run { target i?86-*-* x86_64-*-* } } */ > + > +struct S { > + int a : 2; > + __attribute__((aligned (8))) int b : 2; > + int c : 28; > + __attribute__((aligned (16))) int d : 2; > + int e : 30; > +} __attribute__((ms_struct)); > + > +struct S s; > + > +int > +main () > +{ > + int i; > + if (sizeof (s) != 32) > + __builtin_abort (); > + s.a = -1; > + for (i = 0; i < 32; ++i) > + if (((char *) &s)[i] != (i ? 0 : 3)) > + __builtin_abort (); > + s.a = 0; > + s.b = -1; > + for (i = 0; i < 32; ++i) > + if (((char *) &s)[i] != (i ? 0 : 12)) > + __builtin_abort (); > + s.b = 0; > + s.c = -1; > + for (i = 0; i < 32; ++i) > + if (((signed char *) &s)[i] != (i > 3 ? 0 : (i ? -1 : -16))) > + __builtin_abort (); > + s.c = 0; > + s.d = -1; > + for (i = 0; i < 32; ++i) > + if (((signed char *) &s)[i] != (i == 16 ? 3 : 0)) > + __builtin_abort (); > + s.d = 0; > + s.e = -1; > + for (i = 0; i < 32; ++i) > + if (((signed char *) &s)[i] != ((i < 16 || i > 19) ? 0 : (i == 16 ? -4 : > -1))) > + __builtin_abort (); > + return 0; > +} > > Jakub
