I've switched form mingw gcc 4.6.2 to 4.7.0 and I've noticed that attribute packed does not work anymore as expected, simple testcase: #include <stdio.h> #include <stdlib.h> #include <inttypes.h> typedef struct st_tag { uint16_t head __attribute__ ((packed)); uint8_t type; uint16_t flags __attribute__ ((packed)); } st; int main(void) { st x; printf("Structure packing got %u offset, expected 3\n", (unsigned)((char*)&x.flags - (char*)&x)); return 0; } the generated asm is: movl $4, 4(%esp) movl $LC0, (%esp) call _printf the value is 3 in gcc 4.6.2 Using built-in specs. COLLECT_GCC=c:\mingw\bin\gcc-4.7.exe COLLECT_LTO_WRAPPER=c:/mingw/bin/../libexec/gcc/i686-pc-mingw32/4.7.0/lto-wrapper.exe Target: i686-pc-mingw32 Configured with: ../gcc-4.7.0/configure --prefix=/mingw --disable-bootstrap --enable-lto --with-pkgversion=Sherpya --with-bugurl=http://oss.netfarm.it/ --target=i686-pc-mingw32 --enable-targets=i686-pc-mingw32 --with-tune=generic --with-cpu=i686 --disable-win32-registry --disable-shared --program-suffix=-4.7 --enable-version-specific-runtime-libs --enable-languages=c,c++ --disable-werror --enable-threads=win32 --with-dwarf2 --disable-nls Thread model: win32 gcc version 4.7.0 (Sherpya) I don't think save-temps output may be relevant
I think I may have encountered this issue aswell. For example, given the following example program: #include <stdio.h> #include <wchar.h> struct A { short s; struct { int i; }; } __attribute__((__packed__)); struct B { short s; struct { int i; } __attribute__((__packed__)); } __attribute__((__packed__)); struct C { struct { int i; }; short s; } __attribute__((__packed__)); int main() { printf("sizeof(struct A) == %i\n", (int)sizeof(struct A)); printf("sizeof(struct B) == %i\n", (int)sizeof(struct B)); printf("sizeof(struct C) == %i\n", (int)sizeof(struct C)); return 0; } with TDM-GCC's mingw32 gcc 4.6.1, all sizes are 6, and for comparison, with Ubuntu's i686-linux-gnu gcc 4.6.3, all sizes are 6. However, with mingw.org's gcc 4.7.0, sizeof(A) == 8. B and C still are 6, as expected; A on the other hand apparently is not being packed, despite the __attribute__((__packed__)).
The -mms-bitfields option seems to trigger the wrong behaviour. As pointed out by http://gcc.gnu.org/gcc-4.7/changes.html it is enabled by default since 4.7.0, and using -mms-bitfields when compiling with TDM-GCC 4.6.1 also causes the wrong behaviour. In contrast, compiling with -mno-ms-bitfields makes the example programs work as expected under gcc 4.7.0.
I see the same breakage with a self-built gcc-4.7.1 for x86_64-w64-mingw32.
The problem started with the PR27942 fix in r114552: http://gcc.gnu.org/ml/gcc-patches/2006-06/msg00569.html http://gcc.gnu.org/ml/gcc-cvs/2006-06/msg00268.html and it affects gcc-4.2.0 up to current trunk. The problem isn't mingw-specific, it can be reproduced natively on e.g. x86_64-linux. Take this C test case, reduced from #c1: struct A { short s; struct { int i; } x; } __attribute__((__packed__)); struct C { struct { int i; } x; short s; } __attribute__((__packed__)); int foo(void) { return sizeof(struct A) == sizeof(struct C); } Compiling this with -mno-ms-bitfields makes foo() return 1, but compiling with -mms-bitfields makes foo() return 0 because sizeof(struct A) then is 8 while sizeof(struct C) still is 6.
The following variation of the test case was compiled with both gcc-4.7.1 and MS Visual Studio 2008: #include <stdio.h> #if defined(__GNUC__) struct A1 { short s; struct { int i; } x; } __attribute__((__packed__)); #endif #pragma pack(1) struct A2 { short s; struct { int i; } x; }; #pragma pack() int main(void) { #if defined(__GNUC__) printf("sizeof(struct A1) == %u\n", (unsigned)sizeof(struct A1)); #endif printf("sizeof(struct A2) == %u\n", (unsigned)sizeof(struct A2)); return 0; } Note that structs A1 and A2 are identical, except A1 is packed (when compiled by gcc) using the attribute syntax and A2 is packed using the #pragma syntax. Compiling this with gcc-4.7.1 (target x86_64-w64-mingw32) with -mms-bitfields (which is the default) gives: sizeof(struct A1) == 8 sizeof(struct A2) == 6 proving that attribute packed doesn't work. With -mno-ms-bitfields both structs are reported as being 6 bytes. Compiling with Visual Studio 2008 gives: sizeof(struct A2) == 6 as expected.
Using the ms_struct attribute instead of compiling with -mms-bitfields reproduces the packing issue, while using the gcc_struct attribute prevents the issue from showing up even under -mms-bitfields. These struct examples show the packing issue without nested structs: struct __attribute__((ms_struct, packed)) A { int a; short b; }; struct __attribute__((ms_struct, packed)) B { short a; int b; }; sizeof(struct A) == 6 as expected sizeof(struct B) == 8 unexpected, expected 6 instead offsetof(struct B, b) == 4 unexpected, expected 2 instead A message [1] from the mingw-w64-public mailing list indicates this behaviour: Only the outer struct is packed, causing padding bytes at its end to be removed as expected, but not the fields: padding bytes in front of a field according to the field's natural alignment are not removed, despite the packed attribute, which is unexpected. Explicitly packing the fields does not help: struct __attribute__((ms_struct)) C { int a __attribute__((packed, aligned(1))); short b __attribute__((packed, aligned(1))); }; struct __attribute__((ms_struct)) D { short a __attribute__((packed, aligned(1))); int b __attribute__((packed, aligned(1))); }; sizeof(struct C) == 6 as expected sizeof(struct D) == 8 unexpected, expected 6 instead offsetof(struct D, b) == 4 unexpected, expected 2 instead [1] http://sourceforge.net/mailarchive/message.php?msg_id=29650589
Let's add in CC target maintainers.
Yes, there is an issue about record/union attribute packed for ms_struct. The following patch is fixing that for me, I just do regression-testing. Index: stor-layout.c =================================================================== --- stor-layout.c (Revision 194356) +++ stor-layout.c (Arbeitskopie) @@ -756,7 +756,10 @@ start_record_layout (tree t) /* If the type has a minimum specified alignment (via an attribute declaration, for example) use it -- otherwise, start with a one-byte alignment. */ - rli->record_align = MAX (BITS_PER_UNIT, TYPE_ALIGN (t)); + if (TYPE_PACKED (t)) + rli->record_align = BITS_PER_UNIT; + else + rli->record_align = MAX (BITS_PER_UNIT, TYPE_ALIGN (t)); rli->unpacked_align = rli->record_align; rli->offset_align = MAX (rli->record_align, BIGGEST_ALIGNMENT); @@ -952,15 +955,20 @@ update_alignment_for_field (record_layout_info rli meaningless. */ if (targetm.ms_bitfield_layout_p (rli->t)) { + if (rli->t && TYPE_PACKED (rli->t) + && (is_bitfield || !DECL_PACKED (field) + || DECL_SIZE (field) == NULL_TREE + || !integer_zerop (DECL_SIZE (field)))) + desired_align = BITS_PER_UNIT; /* Here, the alignment of the underlying type of a bitfield can affect the alignment of a record; even a zero-sized field can do this. The alignment should be to the alignment of 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)) - || ((DECL_SIZE (field) == NULL_TREE - || !integer_zerop (DECL_SIZE (field))) + else if ((!is_bitfield && !DECL_PACKED (field)) + || ((DECL_SIZE (field) == NULL_TREE + || !integer_zerop (DECL_SIZE (field))) ? !DECL_PACKED (field) : (rli->prev_field && DECL_BIT_FIELD_TYPE (rli->prev_field) @@ -1414,7 +1422,13 @@ place_field (record_layout_info rli, tree field) } /* Now align (conventionally) for the new type. */ - type_align = TYPE_ALIGN (TREE_TYPE (field)); + if (!TYPE_PACKED (rli->t)) + { + type_align = TYPE_ALIGN (TREE_TYPE (field)); + if (DECL_PACKED (field)) + type_align = MIN (type_align, BITS_PER_UNIT); + + } if (maximum_field_alignment != 0) type_align = MIN (type_align, maximum_field_alignment);
It doesn't look to me that the first two hunks of the patch are needed. The last hunk doesn't look like it should be considering either TYPE_PACKED of the parent structure, or TYPE_ALIGN of the field type, but rather using DECL_ALIGN of the field. Which ought already have been assigned by layout_decl via update_alignment_for_field. The biggest problem to me appears to be the entire ms_bitfield_layout_p block of code there, whose block comments talk about adjusting bitfields, and yet is firing for structures that contain no bitfields at all. That tells me that the logic higher in the block (or elsewere wrt bitfield_layout_p) is in error.
(In reply to comment #9) > It doesn't look to me that the first two hunks of the patch are needed. Yes, that's right. > The last hunk doesn't look like it should be considering either TYPE_PACKED > of the parent structure, or TYPE_ALIGN of the field type, but rather > using DECL_ALIGN of the field. Which ought already have been assigned > by layout_decl via update_alignment_for_field. Well, nothing considers TYPE_PACKED of the parent structure for ms_struct. The field's type-alignment needs to be considered for checking, if type has user-specified alignment AFAICS. > The biggest problem to me appears to be the entire ms_bitfield_layout_p > block of code there, whose block comments talk about adjusting bitfields, > and yet is firing for structures that contain no bitfields at all. That > tells me that the logic higher in the block (or elsewere wrt bitfield_layout_p) > is in error. That isn't the fully story for ms_struct here, the ms_biffield_layout_p block has as internal packing of same-types - that's not related to aligned or packed attributes at all - and needs to be done dependent to its offset, when type's offset is known. So there is no logical-error in this code, beside. Nevertheless I admit that the ms_struct code-path and the gnu_struct code-path could win by separating them, and for both some documention about used schema in source would be great. In general it would be great to have somebody with a VC, who would be willing to compile some test-cases by this VC. The documention about structure-layout on msdn it pretty poor and most stuff needs to be collected or done via best-guess. User-specified alignment within structures (not via #pragma pack) seems to be introduced in recent VC versions, so tester should have a modern version.
Created attachment 29119 [details] Testcase for bitfield and type-packing This first testcase checking pack-pragma for x64/x86 MS-struct layout. I got this testcase verified by a VC user. It was verified with VC10, VC11, and (Nov2012_CTP). (Btw no regression with my suggested patch applied).
Created attachment 29121 [details] Testcase for bitfield and structure-aligning via attribute This testcase use aligned/align attribute instead of #pragma pack. With suggested patch applied no regressions to VC results.
Created attachment 29122 [details] Testcase for bitfield and structure field-aligning by attribute By this testcase we validate that structure's member-alignment set by attribute is applied. With applied patch this test shows no regressions to VC compiled variant.
Ping -- there's been a patch for this bug for almost 10 months now...
So is the problem here that -mms-bitfields became the default, which caused difficulties, or that -mms-bitfields is broken? (if the latter, does using gcc_struct everywhere cause the right behavior?) Sorry I'm a bit clueless here. Thanks!
ms-bitfield is broken regarding pack-attribute and align-attribute. Later is the cause why suggested patch is just half of the story.
I'm affected by this as well. I was hit when cross-compiling Rockbox simulators (www.rockbox.org) for win32 using i686-w64-mingw32-gcc, version 4.8.2
*** Bug 260998 has been marked as a duplicate of this bug. *** Seen from the domain http://volichat.com Marked for reference. Resolved as fixed @bugzilla.
This bug report is now over three years old. Any progress? I'm still seeing it in 5.1.0. Should the summary also be updated to reflect that this is a regression? And should the default be reverted until the code can be fixed?
I just hit this bug on dfu-util 0.8 (i686-w64-mingw32 5.3.0) The patch has been available for 3 years now, but if you don't want to apply that patch please apply at least the workaround (reverting to -mno-ms-bitfields as default)
Hi! I'm still seeing this problem on recent release 6.3.1, and it seems to be enabled by default on at least some builds (msys2 for example). Can I help somehow to get this into trunk sooner? Thanks!
(In reply to Ladislav Láska from comment #21) > Hi! > > I'm still seeing this problem on recent release 6.3.1, and it seems to be > enabled by default on at least some builds (msys2 for example). > > Can I help somehow to get this into trunk sooner? > > Thanks! everyone uses -mno-ms-bitfields nowadays
Not everyone, ms bitfields are enabled by default on MINGW architectures, making the "packed" attribute broken by default and without a warning. If everyone really uses -mno-ms-bitfields, it should become the default and possibly removed, or the documentation fixed to warn everyone that using it will break stuff.
This is a regression even if the bug was latent with -mms-bitfields.
GCC 5 branch is being closed
I try to work on this bug, by curiosity... And I did found 3 bug in the current layout of structure when ms_bitfield_layout_p = 1: ************************** 1) Basic packing (the failing test): -------------------------- PACK(struct test_sp1 { int a; short b; int c; char d; }); assert_cc(sizeof(struct test_sp1) == 11); assert_cc(offsetof(struct test_sp1, a) == 0); assert_cc(offsetof(struct test_sp1, b) == 4); assert_cc(offsetof(struct test_sp1, c) == 6); assert_cc(offsetof(struct test_sp1, d) == 10); -------------------------- Associated Patch : -------------------------- @@ -1460,7 +1475,8 @@ place_field (record_layout_info rli, tree field) } /* 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); -------------------------- ************************** 2) Struct packing that contains a field with an aligned attribute (the failing test): -------------------------- PACK(struct test_sp3 { int a; ALIGN(short b, 8); int c; char d; }); assert_cc(sizeof(struct test_sp3) == 16); assert_cc(offsetof(struct test_sp3, a) == 0); assert_cc(offsetof(struct test_sp3, b) == 8); assert_cc(offsetof(struct test_sp3, c) == 10); assert_cc(offsetof(struct test_sp3, d) == 14); -------------------------- Associated Patch : -------------------------- @@ -1018,8 +1018,13 @@ update_alignment_for_field (record_layout_info rli, tree field, if (maximum_field_alignment != 0) type_align = MIN (type_align, maximum_field_alignment); rli->record_align = MAX (rli->record_align, type_align); - rli->unpacked_align = MAX (rli->unpacked_align, TYPE_ALIGN (type)); - } + } + else + { + rli->record_align = MAX (rli->record_align, desired_align); + } + + rli->unpacked_align = MAX (rli->unpacked_align, TYPE_ALIGN (type)); } else if (is_bitfield && PCC_BITFIELD_TYPE_MATTERS) { -------------------------- ************************** 3) Struct with a bit-field followed by a char (the failing test): -------------------------- struct test_s4 { int a; short b; int c:15; char d; }; #ifdef _WIN32 assert_cc(sizeof(struct test_s4) == 16); assert_cc(offsetof(struct test_s4, a) == 0); assert_cc(offsetof(struct test_s4, b) == 4); assert_cc(offsetof(struct test_s4, d) == 12); #else assert_cc(sizeof(struct test_s4) == 12); assert_cc(offsetof(struct test_s4, a) == 0); assert_cc(offsetof(struct test_s4, b) == 4); assert_cc(offsetof(struct test_s4, d) == 8); #endif -------------------------- Associated Patch : -------------------------- @@ -1218,9 +1223,21 @@ place_field (record_layout_info rli, tree field) /* No, we need to skip space before this field. Bump the cumulative size to multiple of field alignment. */ - if (!targetm.ms_bitfield_layout_p (rli->t) - && DECL_SOURCE_LOCATION (field) != BUILTINS_LOCATION) - warning (OPT_Wpadded, "padding struct to align %q+D", field); + if (targetm.ms_bitfield_layout_p (rli->t)) + { + if (rli->prev_field != NULL && + !integer_zerop (DECL_SIZE (rli->prev_field)) ) + { + rli->bitpos = size_binop (PLUS_EXPR, rli->bitpos, + bitsize_int (rli->remaining_in_alignment)); + } + rli->prev_field = NULL; + } + else + { + if (DECL_SOURCE_LOCATION (field) != BUILTINS_LOCATION) + warning (OPT_Wpadded, "padding struct to align %q+D", field); + } /* If the alignment is still within offset_align, just align the bit position. */ -------------------------- ************************ *) Macro used in the code above -------------------------- #ifdef _WIN32 # ifdef _MSC_VER # define PACK(typeDec) __pragma( pack(push, 1) ) typeDec __pragma( pack(pop) ) # else # define PACK(typeDec) typeDec __attribute__((__packed__,ms_struct)) # endif #else # define PACK(typeDec) typeDec __attribute__((__packed__)) #endif #ifdef _MSC_VER # define ALIGN(typeDec, n) __declspec(align(n)) typeDec #else # define ALIGN(typeDec, n) typeDec __attribute__((aligned(n))) #endif --------------------------
The #c26 tests in a single testcase: #ifdef _WIN32 # ifdef _MSC_VER # define PACK(typeDec) __pragma( pack(push, 1) ) typeDec __pragma( pack(pop) ) # else # define PACK(typeDec) typeDec __attribute__((__packed__,ms_struct)) # endif #else # define PACK(typeDec) typeDec __attribute__((__packed__)) #endif #ifdef _MSC_VER # define ALIGN(typeDec, n) __declspec(align(n)) typeDec #else # define ALIGN(typeDec, n) typeDec __attribute__((aligned(n))) #endif #define assert_cc(expr) extern char c[(expr) ? 1 : -1] #define offsetof(x, y) __builtin_offsetof (x, y) PACK(struct test_sp1 { int a; short b; int c; char d; }); assert_cc(sizeof(struct test_sp1) == 11); assert_cc(offsetof(struct test_sp1, a) == 0); assert_cc(offsetof(struct test_sp1, b) == 4); assert_cc(offsetof(struct test_sp1, c) == 6); assert_cc(offsetof(struct test_sp1, d) == 10); PACK(struct test_sp3 { int a; ALIGN(short b, 8); int c; char d; }); assert_cc(sizeof(struct test_sp3) == 16); assert_cc(offsetof(struct test_sp3, a) == 0); assert_cc(offsetof(struct test_sp3, b) == 8); assert_cc(offsetof(struct test_sp3, c) == 10); assert_cc(offsetof(struct test_sp3, d) == 14); struct test_s4 { int a; short b; int c:15; char d; }; #ifdef _WIN32 assert_cc(sizeof(struct test_s4) == 16); assert_cc(offsetof(struct test_s4, a) == 0); assert_cc(offsetof(struct test_s4, b) == 4); assert_cc(offsetof(struct test_s4, d) == 12); #else assert_cc(sizeof(struct test_s4) == 12); assert_cc(offsetof(struct test_s4, a) == 0); assert_cc(offsetof(struct test_s4, b) == 4); assert_cc(offsetof(struct test_s4, d) == 8); #endif but rather with r114364 which revamped the ms bitfield layout completely. See https://gcc.gnu.org/ml/gcc-patches/2006-04/msg01064.html for more info. The above testcase indeed passes with -D_WIN32 -mms-bitfields when built with GCC 4.1 or earlier. Ignoring the type_align bump in place_field for DECL_PACKED fields sounds reasonable, just the formatting is wrong (needs a space in between DECL_PACKED and (field)). In the second hunk, the formatting is also wrong (e.g. a single statement body of else shouldn't be wrapped with {}s around it), but more importantly it is unclear why that would be the right thing, e.g. why is ignoring maximum_field_alignment the right thing etc. So, for the first two hunks in #c26 I'd go with: --- gcc/stor-layout.c.jj 2018-02-22 14:35:33.135216198 +0100 +++ gcc/stor-layout.c 2018-02-27 15:14:03.271387867 +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); @@ -1555,7 +1558,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); instead. The #c26 third hunk is I presume instead something that was probably broken only in GCC 4.6 with r184409 aka PR52238 fix. That if (targetm.ms_bitfield_layout_p (rli->t)) rli->prev_field = NULL; in that patch looks wrong to me, you lose all the special handling of rli->prev_field that way. Before trying to deal with that, I think it is important to figure out what VC does for the case when a bitfield is followed by another bitfield with the same underlying type size, but higher alignment. E.g. how is: struct S { int a : 2; __declspec(align(8)) int b : 2; int c : 28; __declspec(align(16)) int d : 2; int e : 30; } s; int a = sizeof (struct S); void f1 (int x) { s.a = x; } void f2 (int x) { s.b = x; } void f3 (int x) { s.c = x; } void f4 (int x) { s.d = x; } void f5 (int x) { s.e = x; } compiled by VC? Godbolt.org seems to be down right now, checking some clang version with -fms-extensions -mms-bitfields shows that the differences in type alignments don't break the bitfield packs, a, b, c are all packed into a single 32-bit word at offset 0, then d and e are packed into another 32-bit word at offset 16 (so the alignment is honored for the first bitfield in the pack). If that is what VC does, then one way to deal with this would be to replace: if (known_align < desired_align) with if (known_align < desired_align && (!targetm.ms_bitfield_layout_p (rli->t) || rli->prev_field == NULL)) drop the if (targetm.ms_bitfield_layout_p (rli->t)) rli->prev_field = NULL; and deal with the further alignment (basically duplicate the if (known_align < desired_align body)) later.
Trying: --- gcc/stor-layout.c.jj 2018-02-22 14:35:33.135216198 +0100 +++ gcc/stor-layout.c 2018-02-27 17:32:17.934820133 +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 @@ -1505,6 +1508,31 @@ place_field (record_layout_info rli, tre as if the prior field was not a bitfield. */ prev_saved = NULL; + /* 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 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; + } + /* Cause a new bitfield to be captured, either this time (if currently a bitfield) or next time we see one. */ if (!DECL_BIT_FIELD_TYPE (field) @@ -1530,7 +1558,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 +1583,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); fixes this testcase, but breaks bf-ms-layout.c and bf-ms-layout-2.c. The tests were adjusted in r186880 and r184519, but perhaps those were just bogus changes. I guess what matters more is whether the layout in those tests and in the above tests match what VC does.
Created attachment 43522 [details] gcc8-pr52991.patch Full untested patch (except for make check-gcc RUNTESTFLAGS='--target_board=unix\{-m32,-m64\} dg.exp=attr-ms*' on x86_64-linux), though this really needs to be double-checked against what VC does, on all the changed testcases.
The test cases bf-ms-layout.c and bf-ms-layout-2.c are valid. You can test it with an online compiler, for example: http://rextester.com/l/c_online_compiler_visual
Thanks for the hint. So: #include <stdio.h> struct S { int a : 2; __declspec(align(8)) int b : 2; int c : 28; __declspec(align(16)) int d : 2; int e : 30; } s; int a = sizeof (struct S); void f1 (int x) { s.a = x; } void f2 (int x) { s.b = x; } void f3 (int x) { s.c = x; } void f4 (int x) { s.d = x; } void f5 (int x) { s.e = x; } void printme (void) { int i; for (i = 0; i < sizeof (s); ++i) printf ("%02x, ", ((unsigned char *) &s)[i]); printf ("\n"); } int main () { printf ("%d\n", (int) sizeof (s)); s.a = -1; printme (); s.a = 0; s.b = -1; printme (); s.b = 0; s.c = -1; printme (); s.c = 0; s.d = -1; printme (); s.d = 0; s.e = -1; printme (); return 0; } matches what GCC with the patch emits (that is the bf-ms-layout-5.c test), and so does: #include <stddef.h> #ifdef _WIN32 # ifdef _MSC_VER # define PACK(typeDec) __pragma( pack(push, 1) ) typeDec __pragma( pack(pop) ) # else # define PACK(typeDec) typeDec __attribute__((__packed__,ms_struct)) # endif #else # define PACK(typeDec) typeDec __attribute__((__packed__)) #endif #ifdef _MSC_VER # define ALIGN(typeDec, n) __declspec(align(n)) typeDec #else # define ALIGN(typeDec, n) typeDec __attribute__((aligned(n))) #endif #define assert_cc(expr) extern char c[(expr) ? 1 : -1] PACK(struct test_sp1 { int a; short b; int c; char d; }); assert_cc(sizeof(struct test_sp1) == 11); assert_cc(offsetof(struct test_sp1, a) == 0); assert_cc(offsetof(struct test_sp1, b) == 4); assert_cc(offsetof(struct test_sp1, c) == 6); assert_cc(offsetof(struct test_sp1, d) == 10); PACK(struct test_sp3 { int a; ALIGN(short b, 8); int c; char d; }); assert_cc(sizeof(struct test_sp3) == 16); assert_cc(offsetof(struct test_sp3, a) == 0); assert_cc(offsetof(struct test_sp3, b) == 8); assert_cc(offsetof(struct test_sp3, c) == 10); assert_cc(offsetof(struct test_sp3, d) == 14); struct test_s4 { int a; short b; int c:15; char d; }; #ifdef _WIN32 assert_cc(sizeof(struct test_s4) == 16); assert_cc(offsetof(struct test_s4, a) == 0); assert_cc(offsetof(struct test_s4, b) == 4); assert_cc(offsetof(struct test_s4, d) == 12); #else assert_cc(sizeof(struct test_s4) == 12); assert_cc(offsetof(struct test_s4, a) == 0); assert_cc(offsetof(struct test_s4, b) == 4); assert_cc(offsetof(struct test_s4, d) == 8); #endif int main () { return 0; } (bf-ms-layout-4.c). So does: #define _TEST_MS_LAYOUT #include <stddef.h> #include <string.h> extern void abort(); #pragma pack(8) struct one { int d; unsigned char a; unsigned short b:7; char c; } ; struct two { int d; unsigned char a; unsigned int b:7; char c; } ; struct three { short d; unsigned short a:3; unsigned short b:9; unsigned char c:7; } ; /* Bitfields of size 0 have some truly odd behaviors. */ struct four { unsigned short a:3; unsigned short b:9; unsigned int :0; /* forces struct alignment to int */ unsigned char c:7; } ; struct five { char a; int :0; /* ignored; prior field is not a bitfield. */ char b; char c; } ; struct six { char a :8; int :0; /* not ignored; prior field IS a bitfield, causes struct alignment as well. */ char b; char c; } ; struct seven { char a:8; char :0; int :0; /* Ignored; prior field is zero size bitfield. */ char b; char c; } ; struct eight { /* ms size 4 */ short b:3; char c; } ; #ifdef _MSC_VER #define LONGLONG __int64 #else #define LONGLONG long long #endif union nine { /* ms size 8 */ LONGLONG a:3; char c; } ; struct ten { /* ms size 16 */ LONGLONG a:3; LONGLONG b:3; char c; } ; #define val(s,f) (s.f) #define check_struct(_X) \ { \ if (sizeof (struct _X) != exp_sizeof_##_X ) \ abort(); \ memcpy(&test_##_X, filler, sizeof(test_##_X));\ if (val(test_##_X,c) != exp_##_X##_c) \ abort(); \ } #define check_union(_X) \ { \ if (sizeof (union _X) != exp_sizeof_##_X ) \ abort(); \ memcpy(&test_##_X, filler, sizeof(test_##_X));\ if (val(test_##_X,c) != exp_##_X##_c) \ abort(); \ } #define check_struct_size(_X) \ { \ if (sizeof (struct _X) != exp_sizeof_##_X ) \ abort(); \ } #define check_struct_off(_X) \ { \ memcpy(&test_##_X, filler, sizeof(test_##_X));\ if (val(test_##_X,c) != exp_##_X##_c) \ abort(); \ } #define check_union_size(_X) \ { \ if (sizeof (union _X) != exp_sizeof_##_X ) \ abort(); \ } #define check_union_off(_X) \ { \ memcpy(&test_##_X, filler, sizeof(test_##_X));\ if (val(test_##_X,c) != exp_##_X##_c) \ abort(); \ } int main(){ unsigned char filler[16]; struct one test_one; struct two test_two; struct three test_three; struct four test_four; struct five test_five; struct six test_six; struct seven test_seven; struct eight test_eight; union nine test_nine; struct ten test_ten; #if defined (_TEST_MS_LAYOUT) || defined (_MSC_VER) 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 = 4; size_t exp_sizeof_nine = 8; size_t exp_sizeof_ten = 16; 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 = 2; char exp_nine_c = 0; char exp_ten_c = 8; #else /* testing -mno-ms-bitfields */ size_t exp_sizeof_one = 8; size_t exp_sizeof_two = 8; size_t exp_sizeof_three = 6; size_t exp_sizeof_four = 6; size_t exp_sizeof_five = 6; size_t exp_sizeof_six = 6; size_t exp_sizeof_seven = 6; size_t exp_sizeof_eight = 2; size_t exp_sizeof_nine = 8; size_t exp_sizeof_ten = 8; unsigned short exp_one_c = 6; unsigned int exp_two_c = 6; unsigned char exp_three_c = 64; unsigned char exp_four_c = 4; char exp_five_c = 5; char exp_six_c = 5; char exp_seven_c = 5; char exp_eight_c = 1; char exp_nine_c = 0; char exp_ten_c = 1; #endif unsigned char i; for ( i = 0; i < 16; i++ ) filler[i] = i; check_struct_off (one); check_struct_off (two); check_struct_off (three); check_struct_off (four); check_struct_off (five); check_struct_off (six); check_struct_off (seven); check_struct_off (eight); check_union_off (nine); check_struct_off (ten); check_struct_size (one); check_struct_size (two); check_struct_size (three); check_struct_size (four); check_struct_size (five); check_struct_size (six); check_struct_size (seven); check_struct_size (eight); check_union_size (nine); check_struct_size (ten); return 0; } which is the patched version of bf-ms-layout.c, and so does: #define _TEST_MS_LAYOUT #include <stddef.h> #include <string.h> extern void abort(); #pragma pack(8) #ifdef __GNUC__ #define ATTR __attribute__ ((ms_struct)) #else #define ATTR #endif struct one { int d; unsigned char a; unsigned short b:7; char c; } ATTR; struct two { int d; unsigned char a; unsigned int b:7; char c; } ATTR; struct three { short d; unsigned short a:3; unsigned short b:9; unsigned char c:7; } ATTR; /* Bitfields of size 0 have some truly odd behaviors. */ struct four { unsigned short a:3; unsigned short b:9; unsigned int :0; /* forces struct alignment to int */ unsigned char c:7; } ATTR; struct five { char a; int :0; /* ignored; prior field is not a bitfield. */ char b; char c; } ATTR; struct six { char a :8; int :0; /* not ignored; prior field IS a bitfield, causes struct alignment as well. */ char b; char c; } ATTR; struct seven { char a:8; char :0; int :0; /* Ignored; prior field is zero size bitfield. */ char b; char c; } ATTR; struct eight { /* ms size 4 */ short b:3; char c; } ATTR; #ifdef _MSC_VER #define LONGLONG __int64 #else #define LONGLONG long long #endif union nine { /* ms size 8 */ LONGLONG a:3; char c; } ATTR; struct ten { /* ms size 16 */ LONGLONG a:3; LONGLONG b:3; char c; } ATTR; #define val(s,f) (s.f) #define check_struct(_X) \ { \ if (sizeof (struct _X) != exp_sizeof_##_X ) \ abort(); \ memcpy(&test_##_X, filler, sizeof(test_##_X));\ if (val(test_##_X,c) != exp_##_X##_c) \ abort(); \ } #define check_union(_X) \ { \ if (sizeof (union _X) != exp_sizeof_##_X ) \ abort(); \ memcpy(&test_##_X, filler, sizeof(test_##_X));\ if (val(test_##_X,c) != exp_##_X##_c) \ abort(); \ } #define check_struct_size(_X) \ { \ if (sizeof (struct _X) != exp_sizeof_##_X ) \ abort(); \ } #define check_struct_off(_X) \ { \ memcpy(&test_##_X, filler, sizeof(test_##_X));\ if (val(test_##_X,c) != exp_##_X##_c) \ abort(); \ } #define check_union_size(_X) \ { \ if (sizeof (union _X) != exp_sizeof_##_X ) \ abort(); \ } #define check_union_off(_X) \ { \ memcpy(&test_##_X, filler, sizeof(test_##_X));\ if (val(test_##_X,c) != exp_##_X##_c) \ abort(); \ } int main(){ unsigned char filler[16]; struct one test_one; struct two test_two; struct three test_three; struct four test_four; struct five test_five; struct six test_six; struct seven test_seven; struct eight test_eight; union nine test_nine; struct ten test_ten; #if defined (_TEST_MS_LAYOUT) || defined (_MSC_VER) 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 = 4; size_t exp_sizeof_nine = 8; size_t exp_sizeof_ten = 16; 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 = 2; char exp_nine_c = 0; char exp_ten_c = 8; #else /* testing -mno-ms-bitfields */ size_t exp_sizeof_one = 8; size_t exp_sizeof_two = 8; size_t exp_sizeof_three = 6; size_t exp_sizeof_four = 6; size_t exp_sizeof_five = 6; size_t exp_sizeof_six = 6; size_t exp_sizeof_seven = 6; size_t exp_sizeof_eight = 2; size_t exp_sizeof_nine = 8; size_t exp_sizeof_ten = 8; unsigned short exp_one_c = 6; unsigned int exp_two_c = 6; unsigned char exp_three_c = 64; unsigned char exp_four_c = 4; char exp_five_c = 5; char exp_six_c = 5; char exp_seven_c = 5; char exp_eight_c = 1; char exp_nine_c = 0; char exp_ten_c = 1; #endif unsigned char i; for ( i = 0; i < 16; i++ ) filler[i] = i; check_struct_off (one); check_struct_off (two); check_struct_off (three); check_struct_off (four); check_struct_off (five); check_struct_off (six); check_struct_off (seven); check_struct_off (eight); check_union_off (nine); check_struct_off (ten); check_struct_size (one); check_struct_size (two); check_struct_size (three); check_struct_size (four); check_struct_size (five); check_struct_size (six); check_struct_size (seven); check_struct_size (eight); check_union_size (nine); check_struct_size (ten); return 0; }; which is the patched version of bf-ms-layout-2.c (with ATTR defined to nothing for non-__GNUC__), again the unpatched version fails.
Author: jakub Date: Wed Feb 28 17:17:29 2018 New Revision: 258075 URL: https://gcc.gnu.org/viewcvs?rev=258075&root=gcc&view=rev Log: 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. Added: trunk/gcc/testsuite/gcc.dg/bf-ms-layout-4.c trunk/gcc/testsuite/gcc.dg/bf-ms-layout-5.c Modified: trunk/gcc/ChangeLog trunk/gcc/stor-layout.c trunk/gcc/testsuite/ChangeLog trunk/gcc/testsuite/gcc.dg/bf-ms-layout-2.c trunk/gcc/testsuite/gcc.dg/bf-ms-layout.c
Hopefully fixed in GCC 8. It is an ABI change, so not going to backport it.
Thank you a lot for the fix. I have no idea what I did yesterday when I did test bf-ms-layout-2.c (Yes the test was wrong, and by default cannot compile under Visual Studio VC) The test can be slightly improved by replacing: #ifdef __GNUC__ #define ATTR __attribute__ ((ms_struct)) #endif By: #ifdef __GNUC__ # ifdef _TEST_MS_LAYOUT # define ATTR __attribute__ ((ms_struct)) # else # define ATTR __attribute__ ((gcc_struct)) # endif #else # define ATTR #endif Otherwise the else branch of "#if defined (_TEST_MS_LAYOUT) || defined (_MSC_VER)" cannot be tested easily. And even more important, it can be directly compiled with Visual Studio VC (VC does not known what is ATTR)
*** Bug 87344 has been marked as a duplicate of this bug. ***
*** Bug 61813 has been marked as a duplicate of this bug. ***