Bug 52991 - [6/7/8 Regression] attribute packed broken on mingw32?
Summary: [6/7/8 Regression] attribute packed broken on mingw32?
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: target (show other bugs)
Version: 4.7.0
: P2 major
Target Milestone: 6.5
Assignee: Not yet assigned to anyone
URL:
Keywords: ABI, wrong-code
Depends on:
Blocks:
 
Reported: 2012-04-15 04:21 UTC by Gianluigi Tiesi
Modified: 2018-02-28 20:32 UTC (History)
21 users (show)

See Also:
Host: i686-pc-mingw32
Target: i686-pc-mingw32
Build: i686-pc-mingw32
Known to work: 4.6.2
Known to fail: 4.7.0
Last reconfirmed: 2012-12-10 00:00:00


Attachments
Testcase for bitfield and type-packing (640 bytes, text/plain)
2013-01-09 12:30 UTC, Kai Tietz
Details
Testcase for bitfield and structure-aligning via attribute (706 bytes, text/plain)
2013-01-09 13:34 UTC, Kai Tietz
Details
Testcase for bitfield and structure field-aligning by attribute (566 bytes, text/plain)
2013-01-09 13:37 UTC, Kai Tietz
Details
gcc8-pr52991.patch (2.46 KB, patch)
2018-02-27 18:03 UTC, Jakub Jelinek
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gianluigi Tiesi 2012-04-15 04:21:09 UTC
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
Comment 1 daniel.c.klauer 2012-08-18 03:37:01 UTC
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__)).
Comment 2 daniel.c.klauer 2012-08-18 04:00:51 UTC
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.
Comment 3 Mikael Pettersson 2012-08-18 10:13:20 UTC
I see the same breakage with a self-built gcc-4.7.1 for x86_64-w64-mingw32.
Comment 4 Mikael Pettersson 2012-08-19 20:56:19 UTC
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.
Comment 5 Mikael Pettersson 2012-08-22 08:34:46 UTC
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.
Comment 6 daniel.c.klauer 2012-10-06 23:02:22 UTC
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
Comment 7 Paolo Carlini 2012-10-06 23:08:12 UTC
Let's add in CC target maintainers.
Comment 8 Kai Tietz 2012-12-10 15:13:37 UTC
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);
Comment 9 Richard Henderson 2012-12-18 17:14:37 UTC
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.
Comment 10 Kai Tietz 2012-12-18 21:35:27 UTC
(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.
Comment 11 Kai Tietz 2013-01-09 12:30:50 UTC
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).
Comment 12 Kai Tietz 2013-01-09 13:34:07 UTC
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.
Comment 13 Kai Tietz 2013-01-09 13:37:13 UTC
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.
Comment 14 Mikael Pettersson 2013-10-05 14:26:02 UTC
Ping -- there's been a patch for this bug for almost 10 months now...
Comment 15 roger pack 2013-12-12 15:32:56 UTC
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!
Comment 16 Kai Tietz 2013-12-12 15:48:55 UTC
ms-bitfield is broken regarding pack-attribute and align-attribute.  Later is the cause why suggested patch is just half of the story.
Comment 17 Thomas Martitz 2014-01-17 10:09:00 UTC
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
Comment 18 Jackie Rosen 2014-02-16 10:02:55 UTC Comment hidden (spam)
Comment 19 Pierre Ossman 2015-07-14 09:08:45 UTC
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?
Comment 20 m.facchin 2016-02-12 10:21:03 UTC
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)
Comment 21 Ladislav Láska 2017-05-19 12:51:23 UTC
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!
Comment 22 Gianluigi Tiesi 2017-05-20 22:08:51 UTC
(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
Comment 23 Ladislav Láska 2017-06-13 07:14:29 UTC
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.
Comment 24 Richard Biener 2017-06-13 07:28:43 UTC
This is a regression even if the bug was latent with -mms-bitfields.
Comment 25 Jakub Jelinek 2017-10-10 13:25:55 UTC
GCC 5 branch is being closed
Comment 26 Benjamin Robin 2017-12-29 20:47:32 UTC
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
--------------------------
Comment 27 Jakub Jelinek 2018-02-27 16:16:16 UTC
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.
Comment 28 Jakub Jelinek 2018-02-27 16:55:44 UTC
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.
Comment 29 Jakub Jelinek 2018-02-27 18:03:27 UTC
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.
Comment 30 Benjamin Robin 2018-02-27 18:51:25 UTC
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
Comment 31 Jakub Jelinek 2018-02-27 21:42:45 UTC
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.
Comment 32 Jakub Jelinek 2018-02-28 17:18:01 UTC
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
Comment 33 Jakub Jelinek 2018-02-28 17:25:04 UTC
Hopefully fixed in GCC 8.  It is an ABI change, so not going to backport it.
Comment 34 Benjamin Robin 2018-02-28 20:32:05 UTC
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)