Bug 52991 - attribute packed broken on mingw32?
attribute packed broken on mingw32?
Status: NEW
Product: gcc
Classification: Unclassified
Component: target
4.7.0
: P3 major
: ---
Assigned To: Not yet assigned to anyone
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-15 04:21 UTC by Gianluigi Tiesi
Modified: 2014-03-26 08:10 UTC (History)
12 users (show)

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


Attachments
Testcase for bitfield and type-packing (2.32 KB, text/plain)
2013-01-09 12:30 UTC, Kai Tietz
Details
Testcase for bitfield and structure-aligning via attribute (2.73 KB, text/plain)
2013-01-09 13:34 UTC, Kai Tietz
Details
Testcase for bitfield and structure field-aligning by attribute (1.55 KB, text/plain)
2013-01-09 13:37 UTC, Kai Tietz
Details

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
*** 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.