[Bug middle-end/22275] [3.4/4.0/4.1 Regression] bitfield layout change (regression?)

[Bug middle-end/22275] [3.4/4.0/4.1 Regression] bitfield layout change (regression?)

[steven at gcc dot gnu dot org]

------- Comment #4 from steven at gcc dot gnu dot org  2005-10-17 15:09 -------
If you add a line to the test case:

  printf ("%d\n", offsetof (CABINETSTATE, fMenuEnumFilter));

you get 2 for GCC 4.1, and 4 for GCC 3.3.

So this is definitely an ABI change.


-- 

steven at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
     Ever Confirmed|0                           |1
   Last reconfirmed|0000-00-00 00:00:00         |2005-10-17 15:09:33
               date|                            |


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22275

[Bug middle-end/22275] [3.4/4.0/4.1 Regression] bitfield layout change (regression?)

[mmitchel at gcc dot gnu dot org]

------- Comment #5 from mmitchel at gcc dot gnu dot org  2005-10-31 03:57 -------
This is a showstopper; we need to at least understand why this changed and
whether or not we should change it back.


-- 

mmitchel at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P2                          |P1


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22275

[Bug middle-end/22275] [3.4/4.0/4.1 Regression] bitfield layout change (regression?)

[steven at gcc dot gnu dot org]

------- Comment #6 from steven at gcc dot gnu dot org  2005-10-31 07:21 -------
This was not a show stopper for GCC 3.4 and GCC 4.0.  Why is it a show stopper
now for GCC 4.1?

And we can't unconditionally change it back now.  We already have GCC 3.4 and
4.0 based compilers in production environments.


-- 

steven at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |mark at codesourcery dot com


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22275

[Bug middle-end/22275] [3.4/4.0/4.1 Regression] bitfield layout change (regression?)

[mark at codesourcery dot com]

------- Comment #7 from mark at codesourcery dot com  2005-10-31 07:44 -------
Subject: Re:  [3.4/4.0/4.1 Regression] bitfield layout
 change (regression?)

steven at gcc dot gnu dot org wrote:
> ------- Comment #6 from steven at gcc dot gnu dot org  2005-10-31 07:21 -------
> This was not a show stopper for GCC 3.4 and GCC 4.0.  Why is it a show stopper
> now for GCC 4.1?
> 
> And we can't unconditionally change it back now.  We already have GCC 3.4 and
> 4.0 based compilers in production environments.

I suspect you didn't realize that the PR wasn't opened until July; I had
no knowledge of it when 3.4 or 4.0 were released.  If I had, I would
have declared it a showstopper then.

Now, it's a showstopper to figure out what happened and make an informed
decision.  If we conform to a published ABI where we did not before,
maybe we do nothing; this is a bug fix.  If we now fail to conform to a
published ABI, maybe we add a switch.  But, we certainly have to
evaluate it before the release.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22275

[Bug middle-end/22275] [3.4/4.0/4.1 Regression] bitfield layout change (regression?)

[steven at gcc dot gnu dot org]

------- Comment #8 from steven at gcc dot gnu dot org  2005-10-31 17:31 -------
Right, I didn't know this wasn't opened until July of this year, sorry.
I should have looked.

I still am not sure whether this does break a documented ABI.  Relevant
texts in C99 are 6.2.6.1 sub 4, and 6.7.2.1 sub 3.

The test case satisfies 6.7.2.1 sub 3, as far as I can tell:
"The expression that specifies the width of a bit-field shall be an
integer constant expression that has nonnegative value that shall not
exceed the number of bits in an object of the type that is specified
if the colon and expression are omitted. If the value is zero, the
declaration shall have no declarator."

Then, 6.2.6.1 sub 4 says:
"Values stored in bit-fields consist of m bits, where m is the size
specified for the bit-field.  The object representation is the set
of m bits the bit-field comprises in the addressable storage unit
holding it."

In our case, we have m==0.  As far as I can tell (but I'm not exactly
a C language lawyer ;-) the standard doesn't say anything about signed
or unsigned bit-fields.

So how should we interpret this...


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22275

[Bug middle-end/22275] [3.4/4.0/4.1 Regression] bitfield layout change (regression?)

[pinskia at gcc dot gnu dot org]

------- Comment #9 from pinskia at gcc dot gnu dot org  2005-10-31 23:02 -------
Here is a compile time testcase for easier regression hunting:
typedef int BOOL;
typedef unsigned int UINT;

#pragma pack(1)

typedef struct {
    BOOL fFullPathTitle:1;
    BOOL fSaveLocalView:1;
    BOOL fNotShell:1;
    BOOL fSimpleDefault:1;
    BOOL fDontShowDescBar:1;
    BOOL fNewWindowMode:1;
    BOOL fShowCompColor:1;
    BOOL fDontPrettyNames:1;
    BOOL fAdminsCreateCommonGroups:1;
    UINT fUnusedFlags:7;
    UINT :0;
    UINT fMenuEnumFilter;
} CABINETSTATE;

int f[sizeof(CABINETSTATE) == 8? 1 : -1];


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22275

[Bug middle-end/22275] [3.4/4.0/4.1 Regression] bitfield layout change (regression?)

[steven at gcc dot gnu dot org]

------- Comment #10 from steven at gcc dot gnu dot org  2005-10-31 23:14 -------
I have asked Janis to reghunt this one.  The bug also affects ppc.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22275

[Bug middle-end/22275] [3.4/4.0/4.1 Regression] bitfield layout change (regression?)

[janis at gcc dot gnu dot org]

------- Comment #11 from janis at gcc dot gnu dot org  2005-11-01 16:42 -------
A regression hunt identified the following patch:

r65207 | jason | 2003-04-03 18:23:04 +0000 (Thu, 03 Apr 2003) | 17 lines

        * stor-layout.c (do_type_align): New fn, split out from...
        (layout_decl): ...here.  Do all alignment calculations for
        FIELD_DECLs here.
        (update_alignment_for_field): Not here.
        (start_record_layout, debug_rli): Remove unpadded_align.
        * tree.h (struct record_layout_info_s): Remove unpadded_align.
        * c-decl.c (finish_enum): Don't set DECL_SIZE, DECL_ALIGN
        or DECL_MODE on the CONST_DECLs.
        (finish_struct): Don't mess with DECL_ALIGN.
        * cp/class.c (build_vtable): Set DECL_ALIGN here.
        (get_vtable_decl): Not here.
        (layout_vtable_decl): Or here.
        (create_vtable_ptr): Or here.
        (layout_class_type): Or here.
        (check_bitfield_decl): Don't mess with field alignment.
        * ada/misc.c (gnat_adjust_rli): #if 0.


-- 

janis at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jason at gcc dot gnu dot org


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22275

[Bug middle-end/22275] [3.4/4.0/4.1 Regression] bitfield layout change (regression?)

[steven at gcc dot gnu dot org]

------- Comment #12 from steven at gcc dot gnu dot org  2005-11-01 16:44 -------
The mail to gcc-patches for the patch identified in comment #11 is
http://gcc.gnu.org/ml/gcc-patches/2003-04/msg00209.html.


-- 

steven at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |steven at gcc dot gnu dot
                   |                            |org


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22275

[Bug middle-end/22275] [3.4/4.0/4.1 Regression] bitfield layout change (regression?)

[pinskia at gcc dot gnu dot org]

------- Comment #13 from pinskia at gcc dot gnu dot org  2005-11-01 16:57 -------
Related to PR 10145


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22275

[Bug middle-end/22275] [3.4/4.0/4.1 Regression] bitfield layout change (regression?)

[pinskia at gcc dot gnu dot org]

-- 

pinskia at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Severity|normal                      |critical


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22275

[Bug middle-end/22275] [3.4/4.0/4.1 Regression] bitfield layout change (regression?)

[steven at gcc dot gnu dot org]

------- Comment #14 from steven at gcc dot gnu dot org  2005-11-07 17:19 -------
As an interesting data point: Intel's ICC 8.0 does what GCC 3.3 and earlier do,
but it seems that ICC 9.0 follow GCC 3.4 and later...


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22275

[Bug middle-end/22275] [3.4/4.0/4.1/4.2 Regression] bitfield layout change (regression?)

[mmitchel at gcc dot gnu dot org]

------- Comment #15 from mmitchel at gcc dot gnu dot org  2005-12-20 02:34 -------
In this message:

http://gcc.gnu.org/ml/gcc/2005-12/msg00558.html

I suggested that we switch GCC 4.1 back to the pre-3.4 behavior and asked if
there were objections.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22275

[Bug middle-end/22275] [3.4/4.0/4.1/4.2 Regression] bitfield layout change (regression?)

[steven at gcc dot gnu dot org]

------- Comment #16 from steven at gcc dot gnu dot org  2006-01-14 13:56 -------
With this patch we force the alignment, but I have to admit I have no idea if
this approach is correct.  The only other place in stor-layout.c where we look
at EMPTY_FIELD_BOUNDARY is in layout_decl, but that code is never reached for
targets that define PCC_BITFIELD_TYPE_MATTERS.

Index: stor-layout.c
===================================================================
--- stor-layout.c       (revision 109701)
+++ stor-layout.c       (working copy)
@@ -1235,6 +1235,20 @@ place_field (record_layout_info rli, tre
       rli->bitpos = bitsize_zero_node;
       rli->offset_align = MIN (rli->offset_align, desired_align);
     }
+#ifdef EMPTY_FIELD_BOUNDARY
+  else if (integer_zerop (DECL_SIZE (field))
+          && ! (*targetm.ms_bitfield_layout_p) (DECL_FIELD_CONTEXT (field)))
+    {
+      /* If FIELD is a zero-bit bitfield, force alignment on the next
+        EMPTY_FIELD_BOUNDARY.  */
+      tree x = size_binop (MINUS_EXPR,
+                          build_int_cst (TREE_TYPE (DECL_SIZE (field)),
+                                         EMPTY_FIELD_BOUNDARY),
+                          rli->bitpos);
+      rli->bitpos = size_binop (PLUS_EXPR, rli->bitpos, x);
+      normalize_rli (rli);
+    }
+#endif
   else
     {
       rli->bitpos = size_binop (PLUS_EXPR, rli->bitpos, DECL_SIZE (field));


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22275

[Bug middle-end/22275] [3.4/4.0/4.1/4.2 Regression] bitfield layout change (regression?)

[steven at gcc dot gnu dot org]

------- Comment #17 from steven at gcc dot gnu dot org  2006-01-14 13:57 -------
If the approach is good, we should at least be checking (DECL_BIT_FIELD
(field)) also before forcing things...


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22275

[Bug middle-end/22275] [3.4/4.0/4.1/4.2 Regression] bitfield layout change (regression?)

[steven at gcc dot gnu dot org]

------- Comment #18 from steven at gcc dot gnu dot org  2006-01-14 14:48 -------
This code in layout_decl looks suspicous:

      /* Should this be controlled by DECL_USER_ALIGN, too?  */
      if (maximum_field_alignment != 0)
        DECL_ALIGN (decl) = MIN (DECL_ALIGN (decl), maximum_field_alignment);

For this test case, maximum_field_alignment==8, so every bitfield is at most
byte aligned by these two lines.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22275

[Bug middle-end/22275] [3.4/4.0/4.1/4.2 Regression] bitfield layout change (regression?)

[steven at gcc dot gnu dot org]

------- Comment #19 from steven at gcc dot gnu dot org  2006-01-14 15:17 -------
On AMD64, EMPTY_FIELD_BOUNDARY is 64 bits, so if we honnor that for the
zero-length bitfield, we get a size of 12 for the struct in the test case in
comment #9 (where we apparently expect 8, which is what you get with
EMPTY_FIELD_BOUNDARY==32 even on AMD64).


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22275

[Bug middle-end/22275] [3.4/4.0/4.1/4.2 Regression] bitfield layout change (regression?)

[steven at gcc dot gnu dot org]

------- Comment #20 from steven at gcc dot gnu dot org  2006-01-14 15:35 -------
Indeed, in GCC 3.2 ("GNU C version 3.2.3 (x86_64-unknown-linux-gnu)") initially
we have a DECL_ALIGN of 64 bits for the zero-length bitfield when we enter
field_decl:

Breakpoint 2, place_field (rli=0xa15b30, field=0x2aaaaab34820) at
stor-layout.c:727
727       tree type = TREE_TYPE (field);
1: debug_tree (field) =  <field_decl 0x2aaaaab34820
    type <integer_type 0x2aaaaaae7750 unsigned int unsigned SI
        size <integer_cst 0x2aaaaaae58d0 constant 32>
        unit size <integer_cst 0x2aaaaaae5a20 constant 4>
        align 32 symtab 0 alias set -1 precision 32 min <integer_cst
0x2aaaaaae5ab0 0> max <integer_cst 0x2aaaaaae5ae0 4294967295>>
    bit-field nonaddressable decl_4 VOID file t.c line 6
    size <integer_cst 0x2aaaaaaee870 type <integer_type 0x2aaaaaaf09c0
bit_size_type> constant 0>
    align 64 offset_align 1 context <record_type 0x2aaaaab349c0> chain
<field_decl 0x2aaaaab348f0 fMenuEnumFilter>>
void
(gdb) undisp 1
(gdb) p field->decl.u1.a.align
$1 = 64
(gdb)    

We go on to layout_decl then with the following parameters:

775       layout_decl (field, known_align);
(gdb) p known_align
$2 = 16
(gdb) p desired_align
$3 = 64
(gdb) p user_align
$4 = 0
(gdb) next
776       if (! DECL_PACKED (field))
(gdb) p field->decl.u1.a.align
$5 = 8

So layout_decl has overruled the desired alignment of the field from 64 bits to
8 bits.  Then we later on change the desired alignment again:

799       if ((* targetm.ms_bitfield_layout_p) (rli->t)
(gdb)
817       if (PCC_BITFIELD_TYPE_MATTERS && type != error_mark_node
(gdb)
826           if (! integer_zerop (DECL_SIZE (field)))
(gdb)
828           else if (! DECL_PACKED (field))
(gdb)
829             desired_align = TYPE_ALIGN (type);
(gdb)
833           if (DECL_NAME (field) != 0)
(gdb) p desired_align
$8 = 32

Then we change the rli->bitpos to match the latest desire_align on lines
880-908 and we lay out the next field on the 32th bit in the record.

In other words, it looks like effectively the pre-3.4 ABI for AMD64
accidentally updated the alignment to 32 bits (instead of 64 as requested).  To
return to the pre-3.4 ABI, we also need target changes to recognize zero-length
bit fields and make EMPTY_FIELD_BOUNDARY return 32 bits for them.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22275

[Bug middle-end/22275] [3.4/4.0/4.1/4.2 Regression] bitfield layout change (regression?)

[steven at gcc dot gnu dot org]

------- Comment #21 from steven at gcc dot gnu dot org  2006-01-14 15:55 -------
Created an attachment (id=10641)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=10641&action=view)
Fix x86_field_alignment for AMD64 zero-length bitfields; try to revert to
pre-jason ABI

So I'll just admit I have no idea how to fix this.  I've attached my latest
effort, but this layout issue is complicated enough that someone _very_
familiar with stor-layout should look at this bug if we're going to have a safe
fix for GCC 4.1.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22275

[Bug middle-end/22275] [3.4/4.0/4.1/4.2 Regression] bitfield layout change (regression?)

[rguenth at gcc dot gnu dot org]

------- Comment #22 from rguenth at gcc dot gnu dot org  2006-01-16 10:22 -------
Honza or Micha - can you take over from here?


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22275

[Bug middle-end/22275] [3.4/4.0/4.1/4.2 Regression] bitfield layout change (regression?)

[matz at suse dot de]

------- Comment #23 from matz at suse dot de  2006-01-16 15:14 -------
The x86-64 ABI itself doesn't talk about zero-sized bitfields.  So both
behaviours are correct regarding the ABI.  It talks about unnamed bitfields
(which zero-sized ones must be) not influencing the overall alignment
of structures or unions, but the problem here is different.

Having said that I agree with Marks mail on gcc@ that the pre-3.4 behaviour
made more sense.  Unfortunately I'm also no stor-layout.c expert, so
can't really comment on how the best approach is to implement it.  I assume
Jason would be the best to comment here, as he changed that behaviour.
Stevens latest patch changes the evaluation of EMPTY_FIELD_BOUNDARY vs.
PCC_BITFIELD_TYPE_MATTERS, so someone needs to make sure that this is okay.

Additionally I don't know how stor-layout tracks alignment, i.e. if
desired_align contains the alignment for the _current_ field, or for the
_next_ field.  A zero-sized bitfield should influence alignment of the
next field (although due to the size of zero this shouldn't make a
difference).


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22275

[Bug middle-end/22275] [3.4/4.0/4.1/4.2 Regression] bitfield layout change (regression?)

[steven at gcc dot gnu dot org]

------- Comment #24 from steven at gcc dot gnu dot org  2006-01-16 23:31 -------
Realistically, I don't think it's safe to still fix this for GCC 4.1.  The
whole thing looks so complicated (to me at least) that the risk of introducing
new bugs just before the release is significant.

It is unfortunate that Jason has completely ignored this regression, even
though he caused it.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22275

[Bug middle-end/22275] [3.4/4.0/4.1/4.2 Regression] bitfield layout change (regression?)

[steven at gcc dot gnu dot org]

------- Comment #25 from steven at gcc dot gnu dot org  2006-01-16 23:33 -------
Re. comment #23, iiuc the old behavior was to align the zero-length bitfield,
not the next field.  My patch (one of them anyway) switches the order of
EMPTY_FIELD_BOUNDARY and PCC_BITFIELD_TYPE_MATTERS back to how it was before
Jason's change, but that change alone is not enough to fix the bug.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22275

[Bug middle-end/22275] [3.4/4.0/4.1/4.2 Regression] bitfield layout change (regression?)

[hubicka at gcc dot gnu dot org]

------- Comment #26 from hubicka at gcc dot gnu dot org  2006-01-17 21:07 -------
Hi,
I've looked into it for some time, so here is my POV of this ugly issue.
It seems to me that from documentation of EMPTY_FIELD_BOUNDARY in gccint it is
clear that it
should be ignored when PPC_BITFIELD_TYPE_MATTERS is set.  This seems pretty
correct in Jason's version of code to me.

Corcerning the value of EMPTY_FIELD_BOUNDARY in i386.h, I actually introduced
it back in 2001 while doing the 64bit changes.  I am pretty sure I didn't much
worried about actual meaning of the value,
I just looked for "32" and was trying to decide whether the value means "32" or
size of word.  The alignment seemed to be more word related.
The behaviour in 3.0.4 is to set the alignment according to type of bitfield
(is PPC_BITFIELD_TYPE_MATTERS).  So struct a {char a; short :0;} has size 2. 
This seems to be handled by the last hunk overwriting EMPTY_FIELD_BOUDNARY
Steven quotes in his analysis that has no equivalent in Jason's code.
I think it should be added into the conditional PPC_BITFIELD_TYPE_MATTERS in
stor layout at the place PPC_BITFIELD_TYPE_MATTERS is processed now, so I will
try to test the patch tomorrow.
I would also suggest removing the ignored EMPTY_FIELD_BOUNDARY from i386.h.

Does something from the above seem to make sense? (this is really slipperly)
Honza


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22275

[Bug middle-end/22275] [3.4/4.0/4.1/4.2 Regression] bitfield layout change (regression?)

[matz at suse dot de]

------- Comment #27 from matz at suse dot de  2006-01-17 22:12 -------
Funnily I've also looked at stor-layout.c a bit, and basically came to
a similar conclusion and patch like Steven.  I agree that as per documentation
PCC_BITFIELD_TYPE_MATTERS overrides EMPTY_FIELD_BOUNDARY.  But that was
also a change by Jasons patch.  Formerly it just "influenced" how empty
fields are handled.  Clearly it influenced it in a different way than simple
overriding.

The problem, like Steven already analyzed, is twofold:
  1) maximum_field_alignment now affects also empty bitfields,
     which it didn't before, because before Jason maximum_field_alignment
     was evaluated before other things were taken into account, and now
     it's the last thing done.  That's why earlier something larger than
     alignment 8 bit was possible with #pragma pack(1) at all.
  2) A bug or feature in pre-3.4 lead to the ignoring of EMPTY_FIELD_ALIGNMENT
     when larger than 32 in our case.  Namely because the initial alignment
     of 64 (as required by EMPTY_FIELD_ALIGNMENT) was overriden by the
     alignment of the type of the bitfield (int here, i.e. 32 bit).

I've come up with this simple patch for the problem, which fixes the testcase
for i386 and x86-64 (in the sense of being compatible with <= 3.3) .
The idea is to simply ignore the max field alignment
for empty bitfield (hence falling back to either PCC_BITFIELD_TYPE_MATTERS
or EMPTY_FIELD_ALIGNMENT as the target requested).

This needs to be tested also with struct where the packed property is not
due to a #pragma pack(1) but rather a packed attribute, or similar.

Index: stor-layout.c
===================================================================
--- stor-layout.c       (revision 107699)
+++ stor-layout.c       (working copy)
@@ -337,6 +337,7 @@
     /* For fields, it's a bit more complicated...  */
     {
       bool old_user_align = DECL_USER_ALIGN (decl);
+      bool zero_bitfield = false;

       if (DECL_BIT_FIELD (decl))
        {
@@ -348,6 +349,7 @@
              && ! DECL_PACKED (decl)
              && ! targetm.ms_bitfield_layout_p (DECL_FIELD_CONTEXT (decl)))
            {
+              zero_bitfield = true;
 #ifdef PCC_BITFIELD_TYPE_MATTERS
              if (PCC_BITFIELD_TYPE_MATTERS)
                do_type_align (type, decl);
@@ -428,7 +430,7 @@
        }

       /* Should this be controlled by DECL_USER_ALIGN, too?  */
-      if (maximum_field_alignment != 0)
+      if (maximum_field_alignment != 0 && ! zero_bitfield)
        DECL_ALIGN (decl) = MIN (DECL_ALIGN (decl), maximum_field_alignment);
     }


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22275

[Bug middle-end/22275] [3.4/4.0/4.1/4.2 Regression] bitfield layout change (regression?)

[matz at suse dot de]

------- Comment #28 from matz at suse dot de  2006-01-17 22:31 -------
And indeed with this testcase:
------------------------------------------------
typedef int BOOL;
typedef unsigned int UINT;

typedef struct {
    BOOL fFullPathTitle:1;
    BOOL fSaveLocalView:1;
    BOOL fNotShell:1;
    BOOL fSimpleDefault:1;
    BOOL fDontShowDescBar:1;
    BOOL fNewWindowMode:1;
    BOOL fShowCompColor:1;
    BOOL fDontPrettyNames:1;

    BOOL fAdminsCreateCommonGroups:1;
    UINT fUnusedFlags:7;

    UINT :0;
    UINT fMenuEnumFilter;
} __attribute__((packed)) CABINETSTATE;

int f[sizeof(CABINETSTATE) == 8? 1 : -1];
---------------------------------------------------

it's still broken with the patch.  gcc 3.3 has size 8 on i686 and size 12
on x86-64.  With some other fix to my patch (below) I get 8 and 8 (without
that fix it's 6 and 6).  This
is consistent with the old #pragma pack(1) behaviour, so arguably this was
an inconsistency in 3.3, worth to be fixed, but it's still a change
in behaviour.  It would be interesting to know what earlier compilers had.

The mentioned fix is ignoring zero sized bitfields also when DECL_PACKED
is set, like so:

Index: stor-layout.c
===================================================================
--- stor-layout.c       (revision 107699)
+++ stor-layout.c       (working copy)
@@ -337,6 +337,7 @@
     /* For fields, it's a bit more complicated...  */
     {
       bool old_user_align = DECL_USER_ALIGN (decl);
+      bool zero_bitfield = false;

       if (DECL_BIT_FIELD (decl))
        {
@@ -345,9 +346,9 @@
          /* A zero-length bit-field affects the alignment of the next
             field.  */
          if (integer_zerop (DECL_SIZE (decl))
-             && ! DECL_PACKED (decl)
              && ! targetm.ms_bitfield_layout_p (DECL_FIELD_CONTEXT (decl)))
            {
+              zero_bitfield = true;
 #ifdef PCC_BITFIELD_TYPE_MATTERS
              if (PCC_BITFIELD_TYPE_MATTERS)
                do_type_align (type, decl);
@@ -408,6 +409,7 @@
         check old_user_align instead.  */
       if (DECL_PACKED (decl)
          && !old_user_align
+         && !zero_bitfield
          && (DECL_NONADDRESSABLE_P (decl)
              || DECL_SIZE_UNIT (decl) == 0
              || TREE_CODE (DECL_SIZE_UNIT (decl)) == INTEGER_CST))
@@ -428,7 +430,7 @@
        }

       /* Should this be controlled by DECL_USER_ALIGN, too?  */
-      if (maximum_field_alignment != 0)
+      if (maximum_field_alignment != 0 && ! zero_bitfield)
        DECL_ALIGN (decl) = MIN (DECL_ALIGN (decl), maximum_field_alignment);
     }


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22275

[Bug middle-end/22275] [3.4/4.0/4.1/4.2 Regression] bitfield layout change (regression?)

[mark at codesourcery dot com]

------- Comment #29 from mark at codesourcery dot com  2006-01-18 23:00 -------
Subject: Re:  [3.4/4.0/4.1/4.2 Regression] bitfield
 layout change (regression?)

I think that we should do as follows.

Preserve the original value of maximum_field_alignment when doing
#pragma pack.  Then, for zero-width bitfields, we should align to the
minimum of the original maximum_field_alignment and the otherwise
natural alignment.

The difference between this and the last proposed patch is that I don't
think we should entirely ignore maximum_field_alignment for zero-width
bitfields; if "long long" as a field will only have (say) 2-byte
alignment on some embedded target where structure-packing is the
default, then a "long long : 0;" bitfield should only force 4-byte
alignment.

However, that's an abstract argument; I'm not actually sure what
existing practice was with older versions of GCC.

Again, in the abstract, I think the example in Comment #12 ought to have
size 8 on both IA32 and AMD64 architectures.  I can't see any good
justification for size 12, with a PCC_BITFIELD_TYPES_MATTER ABI.  And, I
think that the size of the structure with #pragma pack(1) ought to be
the same as with __attribute__((packed)).

So, my concern with the patch in Comment #12 is that it would ignore the
pre-set maximum_field_alignment on targets with default structure
packing; other than that, I think it looks fine.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22275

[Bug middle-end/22275] [3.4/4.0/4.1/4.2 Regression] bitfield layout change (regression?)

[steven at gcc dot gnu dot org]

------- Comment #30 from steven at gcc dot gnu dot org  2006-01-18 23:08 -------
We should at least avoid introducing a third variant of how we lay out these
nasty zero-sized friends.  People actually use them.  For example Wine uses
them to enforce compatible alignment and data layout with MS Windows
datastructure.  Wine has a bunch of #ifdefs spread around in its sources to
make it work with the pre- and post-GCC 3.4 layout.  Adding a third would drive
those poor people nuts.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22275

[Bug middle-end/22275] [3.4/4.0/4.1/4.2 Regression] bitfield layout change (regression?)

[mark at codesourcery dot com]

------- Comment #31 from mark at codesourcery dot com  2006-01-18 23:28 -------
Subject: Re:  [3.4/4.0/4.1/4.2 Regression] bitfield
 layout change (regression?)

steven at gcc dot gnu dot org wrote:
> ------- Comment #30 from steven at gcc dot gnu dot org  2006-01-18 23:08 -------
> We should at least avoid introducing a third variant of how we lay out these
> nasty zero-sized friends.  People actually use them.  For example Wine uses
> them to enforce compatible alignment and data layout with MS Windows
> datastructure.  Wine has a bunch of #ifdefs spread around in its sources to
> make it work with the pre- and post-GCC 3.4 layout.  Adding a third would drive
> those poor people nuts.

I agree -- but I don't think we're talking about a third variant.
Michael's patch looks to me like it will restore the pre-3.4 behavior,
which everyone agrees makes more sense.  My issue with respect to
maximum_field_alignment doesn't really apply to pre-4.0 toolchains,
since they didn't have default structure packing for targets, AFAICT.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22275

[Bug middle-end/22275] [3.4/4.0/4.1/4.2 Regression] bitfield layout change (regression?)

[matz at suse dot de]

------- Comment #32 from matz at suse dot de  2006-01-19 14:44 -------
Mark, I agree that it's saner when both structures (with #pragma pack and
attribute packed) have the same length of 8 on i686 and x86_64 (because
the bitfield was declared 'int' in difference to 'long' for instance).

Then I have a question to clarify if I understood correctly: by remembering
the original maximum_field_alignment and using that for zero-sized bitfields
you want to use the absolute first, default, m_f_a, or the one last set
before the innermost #pragma pack?  Consider an example like so, and lets
assume the initial max field alignment was 4:

mfa == 4
#pragma pack(2)         // 1  mfa == 2
#pragma pack(1)         // 2  mfa == 1
#pragma pack()          // 3  mfa == 4
#pragma pack (push,2)   // 4  mfa == 2
#pragma pack (push,1)   // 5  mfa == 1
#pragma pop             // 6  mfa == 2
#pragma pop             // 7  mfa == 4

With what would you constrain the alignment of a zero sized bitfield
at each of the seven points?  What if the initial mfa is 0 (i.e. not set)?
Should -fpack-struct=... (which influences the initial mfa) influence
that constraint too, or not?

My opinion is, that at each of the seven points above we should constrain
with the initial mfa (i.e. 4 in the example above), as adjusted by
the -fpack-struct command line option.

That would have the effect of aligning zero sized bitfield at max to the
architecture default (possibly adjusted globally by the cmdline option),
while effectively ignoring all #pragma packs in effect.  I think that is
what we want the semantics of a zero-sized bitfield to be.  Agreed?

Another point: If we make the structure with attribute packed on both x86 and
x86-64 be eight long (to agree with the behaviour of using pragma), then we
do add another variant unfortunately.  In pre 3.4 that structure was 12 on
x86-64 (which I think was an actual error).  Wine itself uses only #pragma
pack AFAIK, so it wouldn't be affected by this change.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22275

[Bug middle-end/22275] [3.4/4.0/4.1/4.2 Regression] bitfield layout change (regression?)

[mark at codesourcery dot com]

------- Comment #33 from mark at codesourcery dot com  2006-01-19 16:59 -------
Subject: Re:  [3.4/4.0/4.1/4.2 Regression] bitfield
 layout change (regression?)

matz at suse dot de wrote:
> ------- Comment #32 from matz at suse dot de  2006-01-19 14:44 -------
> Mark, I agree that it's saner when both structures (with #pragma pack and
> attribute packed) have the same length of 8 on i686 and x86_64 (because
> the bitfield was declared 'int' in difference to 'long' for instance).
> 
> Then I have a question to clarify if I understood correctly: by remembering
> the original maximum_field_alignment and using that for zero-sized bitfields
> you want to use the absolute first, default, m_f_a, or the one last set
> before the innermost #pragma pack?  Consider an example like so, and lets
> assume the initial max field alignment was 4:
> 
> mfa == 4
> #pragma pack(2)         // 1  mfa == 2
> #pragma pack(1)         // 2  mfa == 1
> #pragma pack()          // 3  mfa == 4
> #pragma pack (push,2)   // 4  mfa == 2
> #pragma pack (push,1)   // 5  mfa == 1
> #pragma pop             // 6  mfa == 2
> #pragma pop             // 7  mfa == 4
> 
> With what would you constrain the alignment of a zero sized bitfield
> at each of the seven points?  

I'm suggesting that a zero-width bitfield have the same alignment
influence at all 7 points.  In your example, mfa would be effectively
four at all 7 points.

> What if the initial mfa is 0 (i.e. not set)?

Then, it should be zero at all seven points, meaning that a zero-width
bitfield of type T would force alignment to whatever alignment a field
of type T would have.

> Should -fpack-struct=... (which influences the initial mfa) influence
> that constraint too, or not?

That's harder, but I think it should.  The point is basically that,
independent of #pragma pack, a zero-width bitfield should give you the
alignment it would give you oustide of the #pragma packs.

> My opinion is, that at each of the seven points above we should constrain
> with the initial mfa (i.e. 4 in the example above), as adjusted by
> the -fpack-struct command line option.

Good, we agree!

> Another point: If we make the structure with attribute packed on both x86 and
> x86-64 be eight long (to agree with the behaviour of using pragma), then we
> do add another variant unfortunately.  In pre 3.4 that structure was 12 on
> x86-64 (which I think was an actual error).

That's unfortunate, but given that things are moving around, I think we
should go for maximum sanity.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22275

[Bug middle-end/22275] [3.4/4.0/4.1/4.2 Regression] bitfield layout change (regression?)

[steven at gcc dot gnu dot org]

------- Comment #34 from steven at gcc dot gnu dot org  2006-01-19 17:40 -------
I looked up a few links to see how people use zero-length bit-fields and what
semantics they're expecting.  I mostly found links to compiler documentation
about how other compilers interpret these bit-fields. Perhaps we can verify
that GCC 4.1 with a patch for this bug satisfies those expectations.  If they
don't, perhaps those deviations should be mentioned explicitly in the release
notes and the manual.


Intel ICC behavior was already noted in comment #14.


IBM (XLC):
http://publib.boulder.ibm.com/infocenter/pseries/v5r3/index.jsp?topic=/com.ibm.vacpp7a.doc/proguide/ref/calgnbit.htm
"A zero-length bit field pads to the next alignment boundary of its base
declared type. This causes the next member to begin on a 4-byte boundary for
all types except long in 64-bit mode and long long in both 32-bit and 64-bit
mode, which will move the next member to the next 8-byte boundary. Padding does
not occur if the previous member's memory layout ended on the appropriate
boundary."

And the rules from "Rules for bit-packed alignment" in the "XL C/C++ Advanced
Edition for LinuxProgramming Guide Version 7.0" state for bit-packed structs:
"If a zero-length bit field is the first member of an aggregate, it has no
effect on boundary. If the zero-length bit field is already at a byte boundary,
the next member starts at this boundary. A non-bit field member that follows a
bit field is aligned on the next bit boundary."  I don't know if bit-packed is
the same as just "packed" in GCC.


Microsoft:
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/vccelng/htm/class_34.asp
Has an example saying "unsigned " 0;    // Force alignment to next boundary.",
which is not very specific (but better than nothing I guess).


HP, HP C manual for VAX and other older HP systems IIUC
(http://docs.hp.com/en/B3901-90012/ch02s01.html):
"zero-length bit-fields force the following member to the next natural
boundary."

And from HP's "Itanium Software Conventions and Runtime Architecture"
(http://www.desktalk.com/drc/resources/ia64rt-12-gen.pdf):
"Zero-length bit fields force the alignment of the following member of a
structure to the next alignment boundary corresponding to the type of the
bit field. An unnamed zero-length bit field, however, will not force the
external alignment of the structure to that boundary."

And last but not least, HP's OpenVMS C compiler
(http://h71000.www7.hp.com/commercial/c/docs/6180p012.html#index_x_361):
"If the bit field is assigned a width of 0, it indicates that no further bit
fields should be placed in the alignment unit[.] (...) As a special case, an
unnamed [bit-]field of width 0 causes the next member (normally another field)
to be aligned on at least a byte boundary; that is, a bit-field structure
member with zero width indicates that no further bit field should be packed
into an alignment unit. (...) [T]he second is zero bits wide, which forces the
next member to be aligned on a natural or byte boundary."

So in summary:
- Intel 9.0 is just as buggy as GCC 3.4 and GCC 4.0 because it apparently
  tries to even mimick GCC bugs now.  I think ICC/GCC compatibility is
  relevant but we can't stop fixing our bugs because Intel adopts them too.
  (Besides, we don't know what ICC does on x86-64 and for e.g. long long).
- Older HP compilers and MS compilers use zero-length bit-fields to force
  the following member of a struct to the next natural (is that GCC's 
  BITS_PER_WORD?) boundary.
- XLC and HP on IA-64 force alignment corresponding to the type of the bit
  field, of which IBM says it is essentially the same, but this is not true
  for GCC with e.g. long long on ix86.


I also found this post from Mark M. about PCC_BITFIELD_TYPE_MATTERS which may
be of interest: http://gcc.gnu.org/ml/gcc/2002-12/msg01324.html.  This is the
first time I've seen the purpose of PCC_BITFIELD_TYPE_MATTERS in a way that I
can understand ;-)


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22275

[Bug middle-end/22275] [3.4/4.0/4.1/4.2 Regression] bitfield layout change (regression?)

[mark at codesourcery dot com]

------- Comment #35 from mark at codesourcery dot com  2006-01-19 19:14 -------
Subject: Re:  [3.4/4.0/4.1/4.2 Regression] bitfield
 layout change (regression?)

steven at gcc dot gnu dot org wrote:

> - Older HP compilers and MS compilers use zero-length bit-fields to force
>   the following member of a struct to the next natural (is that GCC's 
>   BITS_PER_WORD?) boundary.
> - XLC and HP on IA-64 force alignment corresponding to the type of the bit
>   field, of which IBM says it is essentially the same, but this is not true
>   for GCC with e.g. long long on ix86.

That's essentially the PCC_BITFIELD_TYPE_MATTERS distinction; the HP/MS
compilers in the first bullet point are not using the pcc-style bitfields.

Your research is interesting.  However, I'm not too worried about our
handling of zero-width bitfields by themselves; the various psABIs
specify that behavior, so we have a clear guideline.  It's when we throw
in various GCC extensions (#pragma pack, __attribute__((aligned)), etc.)
that it gets complicated and hard.

I'm hoping that since Michael and I independently came to the same
conclusion about how things should work, and since there were no
objections to my initial proposal to change things back on the mailing
list, we're actually pretty close to a patch.  Michael, do you plan to
revise your patch to implement the semantics on which we've agreed?

Thanks,


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22275

[Bug middle-end/22275] [3.4/4.0/4.1/4.2 Regression] bitfield layout change (regression?)

[matz at suse dot de]

------- Comment #36 from matz at suse dot de  2006-01-20 14:01 -------
Yes.  Should be done shortly.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22275

[Bug middle-end/22275] [3.4/4.0/4.1/4.2 Regression] bitfield layout change (regression?)

[matz at suse dot de]

------- Comment #37 from matz at suse dot de  2006-01-20 16:36 -------
Hmpf.  One more difficulty.  x86 uses the ADJUST_FIELD_ALIGN macro
to further fiddle with alignments of fields.  On x86 this is used to
adjust the alignment of long long to 4 (instead of the natural 8).
This is used only when the field is not DECL_PACKED (makes sense).
This has the funny side-effect that a struct containing a long long zero-width
bitfield aligns to 4 for unpacked and to 8 for packed structs, i.e. the
packed struct actually is _larger_ than the unpacked struct.  E.g. the
running example with UINT being long long:

typedef int BOOL;
typedef unsigned long long UINT;
#pragma pack(1)
typedef struct {
    BOOL fFullPathTitle:1;
    BOOL fSaveLocalView:1;
    BOOL fNotShell:1;
    BOOL fSimpleDefault:1;
    BOOL fDontShowDescBar:1;
    BOOL fNewWindowMode:1;
    BOOL fShowCompColor:1;
    BOOL fDontPrettyNames:1;

    BOOL fAdminsCreateCommonGroups:1;
    UINT fUnusedFlags:7;

    UINT :0;
    UINT fMenuEnumFilter;
} CABINETSTATE;

This struct being unpacked (only influenced by #pragma) has size 12 after
my patch on i686, and size 16 (!) when being packed via attribute.

What's even more ugly is that pre-3.4 GCC had a size of 16 for both
cases (pragma or attribute packed) on i686 :-(

So, how would we like to handle this?  Doing as pre 3.4 did is probably
possible but not trivially done.  Basically the code doing this is:

      if (! DECL_USER_ALIGN (decl) && ! DECL_PACKED (decl))
        {
#ifdef ADJUST_FIELD_ALIGN
          DECL_ALIGN (decl) = ADJUST_FIELD_ALIGN (decl, DECL_ALIGN (decl));
#endif
        }

Now, I could just ignore DECL_PACKED for zero-width bitfields, then the
adjustment would be done for both cases and we had a size of 12 with
attribute or pragma, i.e. not the same as pre 3.4 in both.  I also
could never adjust zero-width bitfields, so that they would get their
natural alignment even when the target wanted something else.  Then both
cases would have size 16, being the same as pre 3.4.

I'm leaning towards not doing field adjustments for zero-width bitfields
at all, having the effect that a zero-width bitfield has a user-alignment
set explicitely (of it's base type).  I think if one understands zero-width
bitfields purely as alignment constraints than this implicit DECL_USER_ALIGN
behaviour seems sensible.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22275

[Bug middle-end/22275] [3.4/4.0/4.1/4.2 Regression] bitfield layout change (regression?)

[mark at codesourcery dot com]

------- Comment #38 from mark at codesourcery dot com  2006-01-20 18:02 -------
Subject: Re:  [3.4/4.0/4.1/4.2 Regression] bitfield
 layout change (regression?)

matz at suse dot de wrote:
> ------- Comment #37 from matz at suse dot de  2006-01-20 16:36 -------
> Hmpf.  One more difficulty.  x86 uses the ADJUST_FIELD_ALIGN macro
> to further fiddle with alignments of fields.  On x86 this is used to
> adjust the alignment of long long to 4 (instead of the natural 8).
> This is used only when the field is not DECL_PACKED (makes sense).
> This has the funny side-effect that a struct containing a long long zero-width
> bitfield aligns to 4 for unpacked and to 8 for packed structs

Ugh.  I think we can all agree that these issues had not been well
thought through previously. :-)

>       if (! DECL_USER_ALIGN (decl) && ! DECL_PACKED (decl))
>         {
> #ifdef ADJUST_FIELD_ALIGN
>           DECL_ALIGN (decl) = ADJUST_FIELD_ALIGN (decl, DECL_ALIGN (decl));
> #endif
>         }
>
> Now, I could just ignore DECL_PACKED for zero-width bitfields, then the
> adjustment would be done for both cases and we had a size of 12 with
> attribute or pragma, i.e. not the same as pre 3.4 in both.  

I don't think a zero-width bitfield should ever be DECL_PACKED.  (In
this case, it's DECL_PACKED because the structure is in the scope of
#pragma pack(1).)

In other words, I think a zero-width bitfield should always be subject
to ADJUST_FIELD_ALIGN; the meaning of a zero-width bitfield of type T
(for PCC_BITFIELD_TYPE_MATTERS) should be "align the next field as you
would a field of type T".

> I'm leaning towards not doing field adjustments for zero-width bitfields
> at all, having the effect that a zero-width bitfield has a user-alignment
> set explicitely (of it's base type).

Careful!  That would be an ABI change even in the non-packed case, which
we don't want to do.

I think the best change would be just not to mark zero-width bitfields
as DECL_PACKED in the first place; a second choice, if easier, would be
to disregard the DECL_PACKED setting in stor-layout.c.  Please add a
test case for this new oddity you discovered, if you would.

Thanks,


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22275

[Bug middle-end/22275] [3.4/4.0/4.1/4.2 Regression] bitfield layout change (regression?)

[matz at suse dot de]

------- Comment #39 from matz at suse dot de  2006-01-23 10:32 -------
Gnah!  It's even worse.  I spoke too soon, and actually pre-3.4 (3.3 in fact)
has the behaviour _swapped_.  I.e. using the example struct I have these
sizes (on i686):
3.3:        normal 16, pragma 16, packed 12
4.1+patch:  normal 12, pragma 12, packed 16

The 3.3 case is nice in the sense that the packed struct actually
is smaller than the unpacked struct, but in a way it also means that 3.3
did somehow adjust the field offsets for the packed struct.

What do we do?


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22275

[Bug middle-end/22275] [3.4/4.0/4.1/4.2 Regression] bitfield layout change (regression?)

[matz at suse dot de]

------- Comment #40 from matz at suse dot de  2006-01-23 11:21 -------
Mark, re your comment #38:  (my comment #39 actually came before, but I forgot
to press "Commit" :-/ ) the #pragma pack(1) does not influence DECL_PACKED.
It is only set by attribute(packed).  That's why the difference of behaviour
between a struct under #pragma pack(1) vs. a struct with an attribute packed
occurs.

I agree that it conceptually makes sense to implicitely have a zero-width
bit-field never be DECL_PACKED (though this would deviate from pre-3.4).

The ugly thing is, that current code in GCC seems to handle exactly this case
differently.  For instance in place_field(), we have this code:
  if (PCC_BITFIELD_TYPE_MATTERS
      && ! targetm.ms_bitfield_layout_p (rli->t)
      && TREE_CODE (field) == FIELD_DECL
      && type != error_mark_node
      && DECL_BIT_FIELD (field)
      && ! DECL_PACKED (field)
      && maximum_field_alignment == 0
      && ! integer_zerop (DECL_SIZE (field))
    ....

the body contains a call to ADJUST_FIELD_ALIGN then.  So if this is a
zero-sized bitfield, then this ADJUST won't be done no matter what DECL_PACKED
is, and it seems that this is wanted here (in difference to the other place
in layout_decl, where zero-sized bitfield simply weren't handled).  The
comment above this code says that it's purpose is compatibility with
PCC, so perhaps struct with zero-sized bitfields weren't handled at all
by PCC, and this is a non-issue.  I don't know.

Otherwise it might be, that both places need to be handled the same way
to not risk inconsistencies.  It currently looks as if it also isn't
handled consistently right now (another call to ADJUST_... misses to
test DECL_PACKED at all for instance).  Double-sigh.

Anyway, I'll attach my current patch which implements the suggested
behaviour, including zero-bitfield == !DECL_PACKED in layout_decl.

And also a small testprogram showing information about different struct
under different settings.  It shows inconsistencies in 3.3, and with
the patch 4.1 is more consistent (plus the case in wine, namely of using
#pragma pack(1) still does the same as pre-3.4).


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22275

[Bug middle-end/22275] [3.4/4.0/4.1/4.2 Regression] bitfield layout change (regression?)

[matz at suse dot de]

------- Comment #41 from matz at suse dot de  2006-01-23 11:23 -------
Created an attachment (id=10710)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=10710&action=view)
candidate patch

This patch contains some commented out test code I had in for playing around.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22275

[Bug middle-end/22275] [3.4/4.0/4.1/4.2 Regression] bitfield layout change (regression?)

[matz at suse dot de]

------- Comment #42 from matz at suse dot de  2006-01-23 11:28 -------
Created an attachment (id=10711)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=10711&action=view)
Testprogram

This program generates the following output for 3.3-hammer-branch on x86-64:
          S_normal_i size 8 align 4 ofs 4
          S_pragma_i size 8 align 1 ofs 4
          S_packed_i size 12 align 1 ofs 8
   S_pragma_packed_i size 12 align 1 ofs 8
         S_normal_ll size 16 align 8 ofs 8
         S_pragma_ll size 16 align 1 ofs 8
         S_packed_ll size 16 align 1 ofs 8
  S_pragma_packed_ll size 16 align 1 ofs 8

With -m32 it's:
          S_normal_i size 8 align 4 ofs 4
          S_pragma_i size 8 align 1 ofs 4
          S_packed_i size 8 align 1 ofs 4
   S_pragma_packed_i size 8 align 1 ofs 4
         S_normal_ll size 16 align 4 ofs 8
         S_pragma_ll size 16 align 1 ofs 8
         S_packed_ll size 12 align 1 ofs 4
  S_pragma_packed_ll size 12 align 1 ofs 4

Note how 3.3 handled packed structs (in difference to those under #pragma)
really strange.

With 4.1 plus patch on x86-64:
          S_normal_i size 8 align 4 ofs 4
          S_pragma_i size 8 align 1 ofs 4
          S_packed_i size 8 align 1 ofs 4
   S_pragma_packed_i size 8 align 1 ofs 4
         S_normal_ll size 16 align 8 ofs 8
         S_pragma_ll size 16 align 1 ofs 8
         S_packed_ll size 16 align 1 ofs 8
  S_pragma_packed_ll size 16 align 1 ofs 8

With -m32:
          S_normal_i size 8 align 4 ofs 4
          S_pragma_i size 8 align 1 ofs 4
          S_packed_i size 8 align 1 ofs 4
   S_pragma_packed_i size 8 align 1 ofs 4
         S_normal_ll size 12 align 4 ofs 4
         S_pragma_ll size 12 align 1 ofs 4
         S_packed_ll size 12 align 1 ofs 4
  S_pragma_packed_ll size 12 align 1 ofs 4

So, it's at least consistent, and maybe even senseful


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22275

[Bug middle-end/22275] [3.4/4.0/4.1/4.2 Regression] bitfield layout change (regression?)

[rguenth at gcc dot gnu dot org]

------- Comment #43 from rguenth at gcc dot gnu dot org  2006-01-27 23:25 -------
Can we have an update on this or post the patch to get review please?


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22275

[Bug middle-end/22275] [3.4/4.0/4.1/4.2 Regression] bitfield layout change

[steven at gcc dot gnu dot org]

------- Comment #44 from steven at gcc dot gnu dot org  2006-02-09 23:02 -------
Ping! :-)
Micha, are you going to take this bug?


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22275

[Bug middle-end/22275] [3.4/4.0/4.1/4.2 Regression] bitfield layout change

[david dot moore at intel dot com]

------- Comment #45 from david dot moore at intel dot com  2006-02-10 22:34 -------
The (C99) standard says:

6.7.2.1 (10) An implementation may allocate any addressable storage unit large
enough to hold a bitfield.

and

6.7.2.1 (11) As a special case, a bit-field structure member with a width of 0
indicates that no further bit-field is to be packed into the unit in which the
previous bit-field, if any, was placed. 

NB -Not into a unit required by the given type (on the <type> :0 line)

So looks like 6 is the more plausible answer and the old behavior seems to be
bizarre at best. It means, in effect, that the compiler has to go back and
change its mind about the size of unit in which the previous bitfield was to be
allocated. (You can no doubt get around the use of the past tense in the
standard by suitable invocation of an oracle, or two-pass compilation - you
could also always allocate eveything in UINTS - yetch)

A more compelling argument for not changing this back is that gcc has done a
good job of being (C++) ABI compatible since 3.4. Changing this back to pre-3.4
behavior now will create an ABI break between 3.4 and 4.0.3. Even delaying the
change till 4.1 would be bad enough. 

So I strongly represent that this change should not be made or, if it is made,
it should only happen under control of a flag that is off by default. 

BTW, you may think I am arguing this because it would avoid having to change
the Intel compiler but actually it looks like when we changed our behavior we
did not make sure that our gcc 3.2 compatible behavior did not change, so it
looks like we have work to do anyway, and the impact of this change on our work
is perhaps an extra leg in a conditional. 

So I can claim altruism here. Actually the real reason is I have been telling
people that the ABI has been very stable since 3.4 and I would rather not have
egg on my face. 


-- 

david dot moore at intel dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |david dot moore at intel dot
                   |                            |com


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22275

[Bug middle-end/22275] [3.4/4.0/4.1/4.2 Regression] bitfield layout change

[david dot moore at intel dot com]

------- Comment #46 from david dot moore at intel dot com  2006-02-11 00:14 -------
(Note - I had not realized the importance of pragma pack to this problem. The
fact that without it the behavior has not changed weakens my case, although it
probably weakens the case that it should be restored to gcc 3.2 behavior by an
equal amount)


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22275

[Bug middle-end/22275] [3.4/4.0/4.1/4.2 Regression] bitfield layout change

[matz at suse dot de]

------- Comment #47 from matz at suse dot de  2006-02-12 03:59 -------
What do you mean with 6 (as making more sense)?  The size of the struct?
Anyway, even ignoring that we talk about structs which are packed in various
ways (as you rightly noticed) even the old (IMHO more sensible behaviour)
fullfills the C standard you quoted.  By aligning it to <type> it automatically
makes a following bitfield not come to lie in the same unit (a byte usually),
though that's obviously not the most strict interpretation of this rule.
So it's not that the old behabiour would violate C99.

What strengenths the case to go back actually are programs relieing on
that behaviour, _and_ that it's more expressive.  With the new behaviour
there's no difference between
  char :0;
  short :0;
  int :0;

If the user really only want to close the current unit he can write 'char :0'.
But if he wants more alignment in a otherwise packed struct he has to play
games currently, whereas with the pre-3.4 sematic he could have written 'int:0'
(if "int" was his desired alignment for the next field).

So, I still stand by my opinion that we should want to go back.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22275

[Bug middle-end/22275] [3.4/4.0/4.1/4.2 Regression] bitfield layout change

[mmitchel at gcc dot gnu dot org]

------- Comment #48 from mmitchel at gcc dot gnu dot org  2006-02-13 22:42 -------
I'm going to comment on the behavior issue raised in the comments from David
Moore, and then review the patch itself.

First, the C standard has nothing to say about this issue; even were it not for
the fact that the change only occurs within the scope of #pragma pack, both
behaviors are conformant.  Second, the ABI standards have nothing to say about
this issue, because (so far as I am aware) none of them discuss #pragma pack. 
If the current behavior (i.e., GCC 4.1 as of today) had been the behavior of
GCC for all time, I would not be in favor of making a change -- even though I
think the current behavior is peculiar.  However, the current behavior is
itself a change; we need to resolve the situation once and for all, and we
might as well do what seems best.  As Michael and I have agreed, it seems to
make sense that the zero-width bitfield directives have meaning based on their
types; that makes the GNU-extended language (including #pragma pack) more
expressive.

I understand the user perception/marketing issue around ABI stability, and
that's very important to me as well.  However, this is very much a corner case
(given the fact that #pragma pack is involved), and I think there are sound
technical arguments for the change.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22275

[Bug middle-end/22275] [3.4/4.0/4.1/4.2 Regression] bitfield layout change

[mmitchel at gcc dot gnu dot org]

------- Comment #49 from mmitchel at gcc dot gnu dot org  2006-02-13 22:59 -------
I've approved this for 4.1 and mainline.

http://gcc.gnu.org/ml/gcc-patches/2006-02/msg01031.html

Please do not apply to 4.0 or 3.4, as an ABI change along a release branch is
even worse than an ordinary ABI change.  Therefore, once applied to 4.1 and
mainline, this PR can be closed.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22275

[Bug middle-end/22275] [3.4/4.0/4.1/4.2 Regression] bitfield layout change

[matz at gcc dot gnu dot org]

------- Comment #50 from matz at gcc dot gnu dot org  2006-02-14 16:13 -------
Subject: Bug 22275

Author: matz
Date: Tue Feb 14 16:12:56 2006
New Revision: 110982

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=110982
Log:
        PR middle-end/22275

        * stor-layout.c (layout_decl): Zero-width bitfields aren't
        influenced by maximum_field_alignment or DECL_PACKED.
        (update_alignment_for_field): Ditto.
        (place_field): Ditto.
        * doc/extend.texi (<#pragma pack>, <Type Attributes>): Document
        this behaviour.

Modified:
    branches/gcc-4_1-branch/gcc/ChangeLog
    branches/gcc-4_1-branch/gcc/doc/extend.texi
    branches/gcc-4_1-branch/gcc/stor-layout.c


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22275

[Bug middle-end/22275] [3.4/4.0/4.2 Regression] bitfield layout change

[jakub at gcc dot gnu dot org]

------- Comment #51 from jakub at gcc dot gnu dot org  2006-02-14 17:39 -------
Fixed on gcc-4_1-branch, isn't going to be changed on gcc-3_4-branch
nor gcc-4_0-branch, as it is an ABI change.


-- 

jakub at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
      Known to fail|3.4.0 4.0.0 4.1.0           |3.4.0 4.0.0 4.2.0
      Known to work|3.0.4 3.2.3                 |3.0.4 3.2.3 4.1.0
            Summary|[3.4/4.0/4.1/4.2 Regression]|[3.4/4.0/4.2 Regression]
                   |bitfield layout change      |bitfield layout change
   Target Milestone|4.0.3                       |4.2.0


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22275

[Bug middle-end/22275] [3.4/4.0/4.2 Regression] bitfield layout change

[matz at gcc dot gnu dot org]

------- Comment #52 from matz at gcc dot gnu dot org  2006-02-15 12:19 -------
Subject: Bug 22275

Author: matz
Date: Wed Feb 15 12:19:49 2006
New Revision: 111109

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=111109
Log:
        PR middle-end/22275

        * stor-layout.c (layout_decl): Zero-width bitfields aren't
        influenced by maximum_field_alignment or DECL_PACKED.
        (update_alignment_for_field): Ditto.
        (place_field): Ditto.
        * doc/extend.texi (<#pragma pack>, <Type Attributes>): Document
        this behaviour.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/doc/extend.texi
    trunk/gcc/stor-layout.c


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22275

[Bug middle-end/22275] [3.4/4.0/4.2 Regression] bitfield layout change

[matz at suse dot de]

------- Comment #53 from matz at suse dot de  2006-02-15 12:24 -------
So, it's fixed.  I'm not able to actually change the state to FIXED, so
someone has to do this for me.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22275

[Bug middle-end/22275] [3.4/4.0 Regression] bitfield layout change

[rguenth at gcc dot gnu dot org]

------- Comment #54 from rguenth at gcc dot gnu dot org  2006-02-15 12:25 -------
Fixed.


-- 

rguenth at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
      Known to fail|3.4.0 4.0.0 4.2.0           |3.4.0 4.0.0
      Known to work|3.0.4 3.2.3 4.1.0           |3.0.4 3.2.3 4.1.0 4.2.0
         Resolution|                            |FIXED
            Summary|[3.4/4.0/4.2 Regression]    |[3.4/4.0 Regression]
                   |bitfield layout change      |bitfield layout change
   Target Milestone|4.2.0                       |4.1.0


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22275