Re: problem with long long bitfields in varasm.c

[Jeffrey A Law <law at cygnus dot com>]

  In message <199905141757.SAA17428@net.HCC.nl>you write:
  > Hello,
  > 
  > I discovered a bug in varasm.c that caused incorrect code to be
  > generated for long long bitfields.
  > 
  > I did sent this problem about 1/2 year ago but nothing happened until
  > now so I just resent it as a reminder.
  > 
  > I found a problem with long long bitfields. The problem occurred with the
  > following program:
  > 
  > struct tmp
  > {
  >   long long int pad : 12;
  >   long long int field : 52;
  > };
  > struct tmp tmp = {0x123LL, 0x123456789ABCDLL};
  > 
  > The code result for the i386 is:
  > tmp:
  > .byte 0x23
  > .byte 0xd1
  > .byte 0xbc
  > .byte 0x9a
  > .byte 0x78
  > .byte 0x55		<--- wrong
  > .byte 0x34
  > .byte 0x12
  > 
  > I found the bug in output_constructor in varasm.c. The value this_time and
  > shift are not calculated correctly.
  > I also found another bug. This bug only occures on the c4x where 
  > HOST_BITS_PER_WIDE_INT == 32. It is illegal to do a shift with this value
  > so I clamped the value of this_time to HOST_BITS_PER_WIDE_INT-1.
  > 
You need to describe the bug in more detail.  There is not enough information
in this description for me to really understand why the original code was
wrong and your new code is correct without going through all the debugging
you've already done.

  > @@ -4192,6 +4192,7 @@ output_constructor (exp, size)
  >  		 (all part of the same byte).  */
  >  	      this_time = MIN (end_offset - next_offset,
  >  			       BITS_PER_UNIT - next_bit);
  > +	      this_time = MIN (this_time, HOST_BITS_PER_WIDE_INT-1);
  >  	      if (BYTES_BIG_ENDIAN)
  >  		{
This does not make any sense.  At least not relative to your description.

About the only thing I can potentially see wrong here would be when all of
the following are true.

BITS_PER_UNIT >= HOST_BITS_PER_WIDE_INT
BITS_PER_UNIT <= end_offset - next_offset
BITS_PER_UNIT - next_bit >= HOST_BITS_PER_WIDE_INT

If those are the conditions, then I think you need better comments in the
code.  Also note you need a space around the binary "-" in the line you
added.


  >  		  /* On big-endian machine, take the most significant bits
  > @@ -4203,8 +4204,7 @@ output_constructor (exp, size)
  >  		  if (shift < HOST_BITS_PER_WIDE_INT
  >  		      && shift + this_time > HOST_BITS_PER_WIDE_INT)
  >  		    {
  > -		      this_time -= (HOST_BITS_PER_WIDE_INT - shift);
  > -		      shift = HOST_BITS_PER_WIDE_INT;
  > +		      this_time = (HOST_BITS_PER_WIDE_INT - shift);
  >  		    }
  >  
  >  		  /* Now get the bits from the appropriate constant word.  */
  > @@ -4236,8 +4236,7 @@ output_constructor (exp, size)
  >  		  if (shift < HOST_BITS_PER_WIDE_INT
  >  		      && shift + this_time > HOST_BITS_PER_WIDE_INT)
  >  		    {
  > -		      this_time -= (HOST_BITS_PER_WIDE_INT - shift);
  > -		      shift = HOST_BITS_PER_WIDE_INT;
  > +		      this_time = (HOST_BITS_PER_WIDE_INT - shift);
  >  		    }
You need to explain these hunks in a little more detail.

Also, it would be good if you could turn your testcase into a executable
testcase so that it could be added to the testsuite.

jeff