[Bug c++/17112] New: default operator= for struct with packed bitfield is wrong

[Bug c++/17112] New: default operator= for struct with packed bitfield is wrong

[gene at digilicious dot com]

#include <iostream>
                                                                               
                                                              
using namespace std;
 
struct myint24 {
  int int24:24  __attribute__ ((packed)); 
};
 
myint24 x[3] = {
  0x123456,
  0x789abc,
  0xdef012
};
 
myint24 y[3];            // starts out as zeros
 
int main(int ac, char* av[])
{
  y[1] = x[1];           // should only copy 3 bytes
  for ( int i=0; i<3; ++i )
    if ( 0 != y[i].int24 )
      cout << i << endl; // should only find one non-zero element, and print "1"
  return 0;
}

// compile with default options

// the output should be "1" but it's not

// the bug exists in 3.4.1 and 3.5.0 cvs snapshot from 20040818
// also in 3.3 versions, but not in 3.2

// I don't expect you need the preprocessed sources in this case, since
// the example is so small.

-- 
           Summary: default operator= for struct with packed bitfield is
                    wrong
           Product: gcc
           Version: 3.4.1
            Status: UNCONFIRMED
          Severity: normal
          Priority: P2
         Component: c++
        AssignedTo: unassigned at gcc dot gnu dot org
        ReportedBy: gene at digilicious dot com
                CC: gcc-bugs at gcc dot gnu dot org,gene at digilicious dot
                    com
 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=17112

[Bug c++/17112] default operator= for struct with packed bitfield is wrong

[bangerth at dealii dot org]

------- Additional Comments From bangerth at dealii dot org  2004-08-20 12:45 -------
I don't know what the "right" behavior is (I would indeed have 
expected that only one element of the array 'y' is nonzero), but 
I get consistent results with icc8 as well as gcc 2.95, 3.2.3, 3.3.4, 
3.4.1 and mainline, namely that 1 and 2 is printed. Note that this 
really includes 3.2.3 as well, contrary to the claim in the original 
report. 
 
Someone with knowledge of the semantics of the packed attribute will 
have to decide what is right and wrong here. 
 
W. 

-- 


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

[Bug c++/17112] default operator= for struct with packed bitfield is wrong

[bangerth at dealii dot org]

------- Additional Comments From bangerth at dealii dot org  2004-08-20 12:50 -------
To see what really happens, here's a way to get a dump of the 
contents of the entire field: 
------------------ 
#include <iostream> 
                                                                                
                                                               
using namespace std; 
  
struct myint24 { 
  int int24:24  __attribute__ ((packed));  
}; 
  
myint24 x[3] = { 
  0x010203, 
  0x040506, 
  0x070809 
}; 
  
myint24 y[3];            // starts out as zeros 
  
int main(int ac, char* av[]) 
{ 
  y[1] = x[1]; 
  for ( int i=0; i<sizeof(y); ++i ) 
    cout << (int)*((char*)y + i) << endl; 
} 
-------------------------- 
 
This, for me, prints 
g/x> ./a.out  
0 
0 
0 
6 
5 
4 
9 
0 
0 
 
W. 

-- 


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

[Bug c++/17112] default operator= for struct with packed bitfield is wrong

[gene at digilicious dot com]

------- Additional Comments From gene at digilicious dot com  2004-08-20 20:23 -------
// If you add a user defined assignment operator, you can make it work!

#include <iostream>

using namespace std;

struct myint24 {
  int int24:24  __attribute__ ((packed));

  // If you add this function, it seems to work
    myint24& operator=(myint24& rhs) {
    int24 = rhs.int24;
    return *this;
  }

};

myint24 x[3] = {
  0x010203,
  0x040506,
  0x070809
};

myint24 y[3];

int main(int ac, char* av[])
{
  y[1] = x[1];			// should only copy 3 bytes

  y[1] = x[1]; 
  for ( int i=0; i<sizeof(y); ++i ) 
    cout << (int)*((char*)y + i) << endl; 

  return 0;
}


-- 


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

[Bug middle-end/17112] Copying of packed bitfields is wrong

[giovannibajo at libero dot it]

------- Additional Comments From giovannibajo at libero dot it  2004-09-23 02:48 -------
Confirmed, not a regression.

This behaviour is totally wrong to me, especially since sizeof(myint24) is 3 as 
expected. The same also applies if you compile the program with the C frontend.

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |giovannibajo at libero dot
                   |                            |it
             Status|UNCONFIRMED                 |NEW
          Component|c++                         |middle-end
     Ever Confirmed|                            |1
           Keywords|                            |wrong-code
      Known to fail|                            |2.95.3 3.0.4 3.2.3 3.3.3
                   |                            |3.4.0 4.0.0
   Last reconfirmed|0000-00-00 00:00:00         |2004-09-23 02:48:23
               date|                            |
            Summary|default operator= for struct|Copying of packed bitfields
                   |with packed bitfield is     |is wrong
                   |wrong                       |


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

[Bug middle-end/17112] Copying of packed bitfields is wrong

[giovannibajo at libero dot it]

------- Additional Comments From giovannibajo at libero dot it  2004-09-23 03:36 -------
int24 is given a SImode despite being 24bits, and then the SImode is given to 
myint24 too (because it's the only field). My understanding is that the struct 
was a BLKmode, everything would work as expected.

I think this bug needs someone more familiar with stor-layout.c than I am.

-- 


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

[Bug middle-end/17112] Copying of packed bitfields is wrong

[roger at eyesopen dot com]

------- Additional Comments From roger at eyesopen dot com  2004-09-23 13:03 -------
As far as the RTL expansion is concerned the problem seems to be that we're
wrapping a 32-bit component-ref around a 24-bit array-ref, such that bitsize
in store_bit_field is 32.  I've also confirmed that this behaviour is common
to both the C and C++ front-ends.

If the assignment is changed to y[1].int24 = x[1].int24 (in both C and C++)
everything works as expected (though the generated code looks fairly hideous,
but I know how to fix that :>).  This is why the explicit assignment operator works
in C++, it copies the field and not the structure.  Now to work out where the
component_ref with the incorrect tree type is coming from...


-- 


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

[Bug middle-end/17112] Copying of packed bitfields is wrong

[giovannibajo at libero dot it]

------- Additional Comments From giovannibajo at libero dot it  2004-09-23 13:16 -------
The problem seems to be that the bitfield is given SImode, and thus, being the 
only field of its containg struct, the struct is given SImode too. This is 
wrong: the struct should be BLKmode for the code to work as expected.

Even if I don't know how the code should actually behave. Should the minimum 
alignment proprty of the bitfield also make the structure packed?

-- 


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

[Bug middle-end/17112] Copying of packed bitfields is wrong

[roger at eyesopen dot com]

------- Additional Comments From roger at eyesopen dot com  2004-09-23 13:29 -------
Giovanni is probably right about BLKmode.  I believe the problem is in expr.c's
get_inner_reference which is given a type whose size field is explicitly
(const_int 24), but unless the mode is BLKmode, the code insists that the
bitsize is the GET_MODE_BITSIZE(mode).

expr.c:5348 onwards is:
      if (mode == BLKmode)
        size_tree = TYPE_SIZE (TREE_TYPE (exp));
      else
        *pbitsize = GET_MODE_BITSIZE (mode);

I believe that a possible cure would be

      if (mode != BLKmode)
        *pbitsize = GET_MODE_BITSIZE (mode);
      size_tree = TYPE_SIZE (TREE_TYPE (exp));


-- 


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

[Bug middle-end/17112] Copying of packed bitfields is wrong

[giovannibajo at libero dot it]

------- Additional Comments From giovannibajo at libero dot it  2004-09-23 14:50 -------
(In reply to comment #8)

> expr.c:5348 onwards is:
>       if (mode == BLKmode)
>         size_tree = TYPE_SIZE (TREE_TYPE (exp));
>       else
>         *pbitsize = GET_MODE_BITSIZE (mode);
> 
> I believe that a possible cure would be
> 
>       if (mode != BLKmode)
>         *pbitsize = GET_MODE_BITSIZE (mode);
>       size_tree = TYPE_SIZE (TREE_TYPE (exp));

I am not sure this is correct. size_tree is used to *only* to compute *pbitsize
later (a few lines below). With your proposed correction, it would be useless to
set *pbitsize, as it would get overwritten.

My understanding is that mode SHOULD be BLKmode at this point, which would allow
to extract the correct size from TYPE_SIZE. If it is not a BLKmode, this is
because of stor-layout.c, compute_record_mode.

  /* If we only have one real field; use its mode.  This only applies to
     RECORD_TYPE.  This does not apply to unions.  */
  if (TREE_CODE (type) == RECORD_TYPE && mode != VOIDmode)
    TYPE_MODE (type) = mode;
  else
    TYPE_MODE (type) = mode_for_size_tree (TYPE_SIZE (type), MODE_INT, 1);


The rationale for this is a few lines before, in a loop through the fields:

      /* If this field is the whole struct, remember its mode so
	 that, say, we can put a double in a class into a DF
	 register instead of forcing it to live in the stack.  */
      if (simple_cst_equal (TYPE_SIZE (type), DECL_SIZE (field)))
	mode = DECL_MODE (field);


The point is that the bitfield is given SImode (and I am told this is right
because it uses the underlying integer mode). But since the alignment of the
bitfield is relaxed to 1, its mode cannot be used for the struct. I believe the
solution is to prevent this to happen, by either checking DECL_PACKED, or
checking the alignment constraints (class alignment < field alignment). This
also requires understanding about whether the class alignement is modified by
having a single packed element (which appears it is, since sizeof() returns 3).

-- 


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

[Bug middle-end/17112] Copying of packed bitfields is wrong

[giovannibajo at libero dot it]

------- Additional Comments From giovannibajo at libero dot it  2004-09-24 03:03 -------
Jim, can you please have a look at this bug and comment on my analysys?

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |wilson at specifixinc dot
                   |                            |com


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

[Bug middle-end/17112] Copying of packed bitfields is wrong

[giovannibajo at libero dot it]

------- Additional Comments From giovannibajo at libero dot it  2004-09-25 09:45 -------
Patch posted by Roger:
http://gcc.gnu.org/ml/gcc-patches/2004-09/msg02635.html

Thanks Roger!

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |patch


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

[Bug middle-end/17112] Copying of packed bitfields is wrong

[cvs-commit at gcc dot gnu dot org]

------- Additional Comments From cvs-commit at gcc dot gnu dot org  2004-09-26 14:58 -------
Subject: Bug 17112

CVSROOT:	/cvs/gcc
Module name:	gcc
Changes by:	sayle@gcc.gnu.org	2004-09-26 14:58:34

Modified files:
	gcc            : ChangeLog stor-layout.c 
	gcc/testsuite  : ChangeLog 
Added files:
	gcc/testsuite/gcc.dg: pr17112-1.c 

Log message:
	PR middle-end/17112
	* stor-layout.c (compute_record_mode): For records with a single
	field, only use the field's mode if its size matches what we'd
	have choosen for the record ourselves.  This forces the use of
	BLKmode for packed records that don't completely fill a mode.
	
	* gcc.dg/pr17112-1.c: New test case.

Patches:
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/ChangeLog.diff?cvsroot=gcc&r1=2.5631&r2=2.5632
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/stor-layout.c.diff?cvsroot=gcc&r1=1.213&r2=1.214
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/ChangeLog.diff?cvsroot=gcc&r1=1.4351&r2=1.4352
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/gcc.dg/pr17112-1.c.diff?cvsroot=gcc&r1=NONE&r2=1.1



-- 


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

[Bug middle-end/17112] Copying of packed bitfields is wrong

[pinskia at gcc dot gnu dot org]

------- Additional Comments From pinskia at gcc dot gnu dot org  2004-09-26 15:16 -------
Fixed.

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|                            |FIXED
   Target Milestone|---                         |4.0.0


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

[Bug middle-end/17112] Copying of packed bitfields is wrong

[schwab at suse dot de]

------- Additional Comments From schwab at suse dot de  2004-09-27 10:06 -------
This breaks ia64. 
 
./xgcc -B./ -B/usr/local/ia64-suse-linux/bin/ 
-isystem /usr/local/ia64-suse-linux/include 
-isystem /usr/local/ia64-suse-linux/sys-include 
-L/tmp/cvs/gcc-20040927/Build/gcc/../ld -O2  -DIN_GCC    
-DUSE_LIBUNWIND_EXCEPTIONS -W -Wall -Wwrite-strings -Wstrict-prototypes 
-Wmissing-prototypes -Wold-style-definition  -isystem ./include  -fPIC 
-DUSE_GAS_SYMVER -g -DHAVE_GTHR_DEFAULT -DIN_LIBGCC2 -D__GCC_FLOAT_NOT_NEEDED  
-I. -I. -I../../gcc -I../../gcc/. -I../../gcc/../include 
-I../../gcc/../libcpp/include  -fexceptions -c ../../gcc/unwind-sjlj.c -o 
libgcc/./unwind-sjlj.o 
../../gcc/unwind.inc: In function '_Unwind_DeleteException': 
../../gcc/unwind.inc:278: error: insn does not satisfy its constraints: 
(call_insn 19 15 22 1 ../../gcc/unwind.inc:277 (parallel [ 
            (call (mem:DI (reg:DI 14 r14 [orig:339 D.6395 ] [339]) [0 S8 A64]) 
                (const_int 1 [0x1])) 
            (clobber (reg:DI 320 b0)) 
            (clobber (reg:DI 14 r14)) 
            (clobber (reg:DI 326 b6)) 
        ]) 235 {call_gp} (insn_list:REG_DEP_TRUE 17 (insn_list:REG_DEP_ANTI 13 
(insn_list:REG_DEP_ANTI 12 (insn_list:REG_DEP_TRUE 11 (insn_list:REG_DEP_ANTI 
10 (insn_list:REG_DEP_TRUE 18 (nil))))))) 
    (nil) 
    (expr_list:REG_DEP_TRUE (use (reg:DI 1 r1)) 
        (expr_list:REG_DEP_TRUE (use (reg:DI 121 out1 [ exc ])) 
            (expr_list:REG_DEP_TRUE (use (reg:SI 120 out0)) 
                (nil))))) 
../../gcc/unwind.inc:278: internal compiler error: in 
reload_cse_simplify_operands, at postreload.c:391 
 

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


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

[Bug middle-end/17112] Copying of packed bitfields is wrong

[schwab at suse dot de]

------- Additional Comments From schwab at suse dot de  2004-09-27 10:09 -------
Sorry, wrong command line. 
 
./xgcc -B./ -B/usr/local/ia64-suse-linux/bin/ 
-isystem /usr/local/ia64-suse-linux/include 
-isystem /usr/local/ia64-suse-linux/sys-include 
-L/tmp/cvs/gcc-20040927/Build/gcc/../ld -O2  -DIN_GCC    
-DUSE_LIBUNWIND_EXCEPTIONS -W -Wall -Wwrite-strings -Wstrict-prototypes 
-Wmissing-prototypes -Wold-style-definition  -isystem ./include  -fPIC 
-DUSE_GAS_SYMVER -g -DHAVE_GTHR_DEFAULT -DIN_LIBGCC2 -D__GCC_FLOAT_NOT_NEEDED  
-I. -I. -I../../gcc -I../../gcc/. -I../../gcc/../include 
-I../../gcc/../libcpp/include  -fexceptions 
-c ../../gcc/config/ia64/unwind-ia64.c -o libgcc/./unwind-ia64.o 
../../gcc/config/ia64/unwind-ia64.c: In function 'alloc_reg_state': 
../../gcc/config/ia64/unwind-ia64.c:312: warning: pointer targets in passing 
arg 1 of 'atomic_alloc' differ in signedness 
../../gcc/config/ia64/unwind-ia64.c: In function 'free_reg_state': 
../../gcc/config/ia64/unwind-ia64.c:328: warning: pointer targets in passing 
arg 1 of 'atomic_free' differ in signedness 
../../gcc/config/ia64/unwind-ia64.c: In function 'alloc_label_state': 
../../gcc/config/ia64/unwind-ia64.c:345: warning: pointer targets in passing 
arg 1 of 'atomic_alloc' differ in signedness 
../../gcc/config/ia64/unwind-ia64.c: In function 'free_label_state': 
../../gcc/config/ia64/unwind-ia64.c:361: warning: pointer targets in passing 
arg 1 of 'atomic_free' differ in signedness 
../../gcc/unwind.inc: In function '_Unwind_DeleteException': 
../../gcc/unwind.inc:278: error: insn does not satisfy its constraints: 
(call_insn 19 15 22 1 ../../gcc/unwind.inc:277 (parallel [ 
            (call (mem:DI (reg:DI 14 r14 [orig:339 D.6395 ] [339]) [0 S8 A64]) 
                (const_int 1 [0x1])) 
            (clobber (reg:DI 320 b0)) 
            (clobber (reg:DI 14 r14)) 
            (clobber (reg:DI 326 b6)) 
        ]) 235 {call_gp} (insn_list:REG_DEP_TRUE 17 (insn_list:REG_DEP_ANTI 13 
(insn_list:REG_DEP_ANTI 12 (insn_list:REG_DEP_TRUE 11 (insn_list:REG_DEP_ANTI 
10 (insn_list:REG_DEP_TRUE 18 (nil))))))) 
    (nil) 
    (expr_list:REG_DEP_TRUE (use (reg:DI 1 r1)) 
        (expr_list:REG_DEP_TRUE (use (reg:DI 121 out1 [ exc ])) 
            (expr_list:REG_DEP_TRUE (use (reg:SI 120 out0)) 
                (nil))))) 
../../gcc/unwind.inc:278: internal compiler error: in 
reload_cse_simplify_operands, at postreload.c:391 
 

-- 


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

[Bug middle-end/17112] Copying of packed bitfields is wrong

[pinskia at gcc dot gnu dot org]

------- Additional Comments From pinskia at gcc dot gnu dot org  2004-09-27 12:55 -------
I don' think this patch caused the problem at all, while looking into the powerpc-apple-darwin 
bootstrap failure (which fails in almost the same way, a clobber which is said to be early clobbered is 
being picked as the same register which is an input register), I applied this patch and did not find the 
crash.  I am starting to think Zdenek's big patch for converting BITMAP's iterators causes the problem.

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


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

[Bug middle-end/17112] Copying of packed bitfields is wrong

[pinskia at gcc dot gnu dot org]

------- Additional Comments From pinskia at gcc dot gnu dot org  2004-09-27 13:20 -------
I can prove that this patch does not cause the bootstrap failure so closing as fixed.

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


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

[Bug middle-end/17112] Copying of packed bitfields is wrong

[schwab at suse dot de]

------- Additional Comments From schwab at suse dot de  2004-09-27 14:06 -------
Sorry, I mixed up two unrelated bugs. 

-- 


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