[Bug middle-end/28161] New: Wrong bit field layout with -mms-bitfields

[Bug middle-end/28161] New: Wrong bit field layout with -mms-bitfields

[kkojima at gcc dot gnu dot org]

Several g++ tests in tmpdir-g++.dg-struct-layout-1 fail with
the execution error.  A reduced testcase is

extern "C" void abort (void);

struct S
{
  long long d:23;
  int e:32;
  int f:32;
} a;

int main (void)
{
  a.e = -3;
  a.f = 1;
  if (a.e != -3)
    abort ();
  return 0;
}

which shows that the current compiler allocates a.e and a.f in
an overlapped manner with -mms-bitfields.  With that option,
the 4.1 compiler allocates a field in the first 8-byte for a.d,
the next 4-byte for a.e and the another 4-byte for a.f, but 4.2
allocates 23-bit for a.d, the next 32-bit for a.e and the last
32-bit for a.f in the same 8-byte.


-- 
           Summary: Wrong bit field layout with -mms-bitfields
           Product: gcc
           Version: 4.2.0
            Status: UNCONFIRMED
          Keywords: wrong-code
          Severity: normal
          Priority: P3
         Component: middle-end
        AssignedTo: unassigned at gcc dot gnu dot org
        ReportedBy: kkojima at gcc dot gnu dot org
 GCC build triplet: i686-pc-linux-gnu
  GCC host triplet: i686-pc-linux-gnu
GCC target triplet: i686-pc-linux-gnu


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

[Bug middle-end/28161] Wrong bit field layout with -mms-bitfields

[seongbae dot park at gmail dot com]

------- Comment #1 from seongbae dot park at gmail dot com  2006-06-26 20:47 -------
I belive this is a bug in stor-layout.c:place_field() around line 10503
bitpos is calculated as bit_offset of rli->prev_field + type size.
However, the prev_field is not really the immediately previous field but the
first field of the consecutive same-sized fields.
Hence, in this case:

struct S
{
  long long d:23;
  int e:32;
  int f:32;
} a;

rli->prev_field is "d" when "field" is f. 
The correct fix should calculate the bitpos as 
rli->bitpos + type size.


-- 

seongbae dot park at gmail dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |seongbae dot park at gmail
                   |                            |dot com


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

[Bug middle-end/28161] Wrong bit field layout with -mms-bitfields

[seongbae dot park at gmail dot com]

------- Comment #2 from seongbae dot park at gmail dot com  2006-06-26 21:10 -------
More specifically:

   1048               if (rli->remaining_in_alignment < bitsize)
   1049                 {
   1050                   /* out of bits; bump up to next 'word'.  */
   1051                   rli->offset = DECL_FIELD_OFFSET (rli->prev_field);
   1052                   rli->bitpos
   1053                     = size_binop (PLUS_EXPR, TYPE_SIZE (type),
   1054                                   DECL_FIELD_BIT_OFFSET
(rli->prev_field));
   1055                   rli->prev_field = field;
   1056                   rli->remaining_in_alignment
   1057                     = tree_low_cst (TYPE_SIZE (type), 1);
   1058                 }

The third operand of size_binop() of size_binop() at line 1053-1054 
should be rli->bitpos.


-- 


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

[Bug middle-end/28161] Wrong bit field layout with -mms-bitfields

[kkojima at gcc dot gnu dot org]

------- Comment #3 from kkojima at gcc dot gnu dot org  2006-06-26 23:10 -------
GDB says that TREE_TYPE of a.d is the 32-bit integer and its
DECL_BIT_FIELD_TYPE is 64-bit when place_field processes a.e.
place_field uses the size of TREE_TYPE of the previous bit filed
when deciding whether a bit field to be packed with the previous
one or not.  It seems that this makes the ms-bitfiled code
confused.  The appended patch is to make place_field use
DECL_BIT_FIELD_TYPE of the previous bit field instead of
TREE_TYPE when comparing the size of consecutive bit fields.
It works for the above testcase, though the thorough tests are
needed.

--- ORIG/trunk/gcc/stor-layout.c        2006-06-13 09:06:36.000000000 +0900
+++ LOCAL/trunk/gcc/stor-layout.c       2006-06-26 16:44:31.000000000 +0900
@@ -1022,6 +1022,7 @@ place_field (record_layout_info rli, tre
   if (targetm.ms_bitfield_layout_p (rli->t))
     {
       tree prev_saved = rli->prev_field;
+      tree prev_type = prev_saved ? DECL_BIT_FIELD_TYPE (prev_saved) : NULL;

       /* This is a bitfield if it exists.  */
       if (rli->prev_field)
@@ -1037,8 +1038,7 @@ place_field (record_layout_info rli, tre
              && !integer_zerop (DECL_SIZE (rli->prev_field))
              && host_integerp (DECL_SIZE (rli->prev_field), 0)
              && host_integerp (TYPE_SIZE (type), 0)
-             && simple_cst_equal (TYPE_SIZE (type),
-                                  TYPE_SIZE (TREE_TYPE (rli->prev_field))))
+             && simple_cst_equal (TYPE_SIZE (type), TYPE_SIZE (prev_type)))
            {
              /* We're in the middle of a run of equal type size fields; make
                 sure we realign if we run out of bits.  (Not decl size,
@@ -1105,8 +1108,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 (TREE_TYPE (prev_saved)))
+             ? !simple_cst_equal (TYPE_SIZE (type), TYPE_SIZE (prev_type))
              : !integer_zerop (DECL_SIZE (field)) ))
        {
          /* Never smaller than a byte for compatibility.  */


-- 


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

[Bug middle-end/28161] Wrong bit field layout with -mms-bitfields

[seongbae dot park at gmail dot com]

------- Comment #4 from seongbae dot park at gmail dot com  2006-06-26 23:41 -------
You're correct -I've overlooked the type mismatch. 
One question though is the local variable "type" of place_field() is 
TREE_TYPE (field), not DECL_BIT_FIELD_TYPE (field).
I'm not familiar with gcc type system to know which is correct
but it doesn't look consistent as it is.


-- 


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

[Bug middle-end/28161] Wrong bit field layout with -mms-bitfields

[kkojima at gcc dot gnu dot org]

------- Comment #5 from kkojima at gcc dot gnu dot org  2006-06-27 00:28 -------
Although I'm also not familiar with these materials, when
place_field is called for a.d, gdb says that

(gdb) call debug_tree (field)
 <field_decl 0xb7f494ac d
    type <integer_type 0xb7ea63f4 long long int DI
        size <integer_cst 0xb7e91510 constant invariant 64>
        unit size <integer_cst 0xb7e91528 constant invariant 8>
        align 64 symtab 0 alias set -1 precision 64 min <integer_cst 0xb7e914c8
0x8000000000000000> max <integer_cst 0xb7e914e0 0x7fffffffffffffff>>
    external nonlocal bit-field nonaddressable decl_3 decl_4 VOID file tiny.cc
line 3
    size <integer_cst 0xb7f4b6a8 type <integer_type 0xb7ea605c bit_size_type>
constant invariant 23>
    user align 64 offset_align 1 context <record_type 0xb7f49450 S> attributes
<tree_list 0xb7f4b690> chain <field_decl 0xb7f49508 e>>

but when it's called for a.e

(gdb) call debug_tree (rli->prev_field)
 <field_decl 0xb7f494ac d
    type <integer_type 0xb7f495c0 SI
        size <integer_cst 0xb7e913d8 constant invariant 32>
        unit size <integer_cst 0xb7e91168 constant invariant 4>
        align 32 symtab 0 alias set -1 precision 23 min <integer_cst 0xb7f168b8
-4194304> max <integer_cst 0xb7f4b6d8 4194303>>
    external nonlocal bit-field nonaddressable decl_3 decl_4 DI file tiny.cc
line 3
    size <integer_cst 0xb7f4b6a8 type <integer_type 0xb7ea605c bit_size_type>
constant invariant 23>
    unit size <integer_cst 0xb7e91348 type <integer_type 0xb7ea6000 unsigned
int> constant invariant 3>
    user align 64 offset_align 128
    offset <integer_cst 0xb7e91180 type <integer_type 0xb7ea6000 unsigned int>
constant invariant 0>
    bit offset <integer_cst 0xb7e91930 type <integer_type 0xb7ea605c
bit_size_type> constant invariant 0>
    bit_field_type <integer_type 0xb7ea63f4 long long int DI
        size <integer_cst 0xb7e91510 constant invariant 64>
        unit size <integer_cst 0xb7e91528 constant invariant 8>
        align 64 symtab 0 alias set -1 precision 64 min <integer_cst 0xb7e914c8
0x8000000000000000> max <integer_cst 0xb7e914e0 0x7fffffffffffffff>> context
<record_type 0xb7f49450 S> attributes <tree_list 0xb7f4b690> chain <field_decl
0xb7f49508 e>>

i.e. DECL_BIT_FIELD_TYPE is added in processing.  I guess that
this is the reason for the use of TREE_TYPE (field) for not yet
processed fields and non ms-bitfield case.  gcc allocates 32-bit
for a.d without -mms-bitfield.  OTOH, ms-bitfield code see
the previous and already processed field and should allocate
64-bit for a.d.


-- 


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

[Bug middle-end/28161] Wrong bit field layout with -mms-bitfields

[patchapp at dberlin dot org]

------- Comment #6 from patchapp at dberlin dot org  2006-06-29 21:45 -------
Subject: Bug number PR middle-end/28161

A patch for this bug has been added to the patch tracker.
The mailing list url for the patch is
http://gcc.gnu.org/ml/gcc-patches/2006-06/msg01473.html


-- 


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

[Bug middle-end/28161] Wrong bit field layout with -mms-bitfields

[kkojima at gcc dot gnu dot org]

------- Comment #7 from kkojima at gcc dot gnu dot org  2006-07-15 06:59 -------
Subject: Bug 28161

Author: kkojima
Date: Sat Jul 15 06:58:57 2006
New Revision: 115464

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=115464
Log:
        PR middle-end/28160
        * stor-layout.c (place_field): Take the bit field with
        an excessive size into account in the ms-bitfiled case.

        PR middle-end/28161
        * stor-layout.c (place_field): Use DECL_BIT_FIELD_TYPE of
        the previous bit field.


Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/stor-layout.c


-- 


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

[Bug middle-end/28161] Wrong bit field layout with -mms-bitfields

[kkojima at gcc dot gnu dot org]

------- Comment #8 from kkojima at gcc dot gnu dot org  2006-07-15 23:13 -------
Fixed.


-- 

kkojima at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|                            |FIXED


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