Re: [5/8] Tweak bitfield alignment handling

[Richard Sandiford <rdsandiford@googlemail.com>]

Richard Sandiford <rdsandiford@googlemail.com> writes:
> Mikael Pettersson <mikpe@it.uu.se> writes:
>> John David Anglin writes:
>>  > On Sun, 18 Nov 2012, Richard Sandiford wrote:
>>  > 
>>  > >        HOST_WIDE_INT start = bitpos_ - (bitpos_ % unit);
>>  > >        if (bitregion_start_ && start < bitregion_start_)
>>  > >  	break;
>>  > > -      if (bitregion_end_ && start + unit > bitregion_end_ + 1)
>>  > > +      if (start + unit > bitregion_end_ + 1)
>>  > 
>>  > This causes:
>>  > 
>>  > /home/dave/gnu/gcc/objdir/./prev-gcc/g++ -B/home/dave/gnu/gcc/objdir/./prev-gcc/ -B/home/dave/opt/gnu/gcc/gcc-4.8.0/hppa-linux-gnu/bin/ -nostdinc++ -B/home/dave/gnu/gcc/objdir/prev-hppa-linux-gnu/libstdc++-v3/src/.libs -B/home/dave/gnu/gcc/objdir/prev-hppa-linux-gnu/libstdc++-v3/libsupc++/.libs -I/home/dave/gnu/gcc/objdir/prev-hppa-linux-gnu/libstdc++-v3/include/hppa-linux-gnu -I/home/dave/gnu/gcc
>>  > /objdir/prev-hppa-linux-gnu/libstdc++-v3/include -I/home/dave/gnu/gcc/gcc/libstdc++-v3/libsupc++ -L/home/dave/gnu/gcc/objdir/prev-hppa-linux-gnu/libstdc++-v3/sr
>>  > c/.libs -L/home/dave/gnu/gcc/objdir/prev-hppa-linux-gnu/libstdc++-v3/libsupc++/.libs -c   -g -O2 -DIN_GCC   -fno-exceptions -fno-rtti -fasynchronous-unwind-tabl
>>  > es -W -Wall -Wno-narrowing -Wwrite-strings -Wcast-qual -Wmissing-format-attribut
>>  > e -pedantic -Wno-long-long -Wno-variadic-macros -Wno-overlength-strings -Werror 
>>  > -fno-common  -DHAVE_CONFIG_H -I. -I. -I../../gcc/gcc -I../../gcc/gcc/. -I../../g
>>  > cc/gcc/../include -I../../gcc/gcc/../libcpp/include  -I../../gcc/gcc/../libdecnu
>>  > mber -I../../gcc/gcc/../libdecnumber/dpd -I../libdecnumber -I../../gcc/gcc/../li
>>  > bbacktrace    ../../gcc/gcc/stor-layout.c -o stor-layout.o../../gcc/gcc/stor-layout.c: In member function 〘bool bit_field_mode_iterator::n
>>  > ext_mode(machine_mode*)〙:
>>  > ../../gcc/gcc/stor-layout.c:2690:43: error: comparison between signed and unsign
>>  > ed integer expressions [-Werror=sign-compare]
>>  >        if (start + unit > bitregion_end_ + 1)
>>  > 					   ^
>>  > cc1plus: all warnings being treated as errors
>>
>> This error also breaks m68k-linux bootstrap.
>>
>> HWI32 issue?
>
> Yeah, I expect so, sorry.
>
> Logically, everything here would be unsigned arithmetic, but as the
> comment says:
>
>   /* We use signed values here because the bit position can be negative
>      for invalid input such as gcc.dg/pr48335-8.c.  */
>
> This is the patch I'm testing.  There are three things being checked here:
>
> - "unit", the size of the mode in isolation.  This really is an unsigned
>   value, and is compared to unsigned values like GET_MODE_PRECISION.
>
> - bitpos_ % unit (+ bitsize_), the start and end positions of the bitfield
>   relative to the start of the mode.  The start position is supposed to be
>   [0, unit), so the modulus and result should be unsigned.  (Using unsigned
>   modulus doesn't cope with negative bit positions combined with
>   non-power-of-2 units, but I don't think we support that.)
>
> - bitregion_start_ and bitregion_end_.  bitpos_ is signed and can be
>   negative, so the bitregion comparison should continue to be signed.
>
> OK to commit if testing succeeds?

Now bootstrapped & regression-tested on x86_64-linux-gnu.

>
> Richard
>
>
> gcc/
> 	* stor-layout.c (bit_field_mode_iterator::next_mode): Fix signedness.
>
> Index: gcc/stor-layout.c
> ===================================================================
> --- gcc/stor-layout.c	2012-11-20 10:15:39.000000000 +0000
> +++ gcc/stor-layout.c	2012-11-20 10:15:39.464712715 +0000
> @@ -2670,10 +2670,6 @@ bit_field_mode_iterator::next_mode (enum
>        if (unit != GET_MODE_PRECISION (mode_))
>  	continue;
>  
> -      /* Skip modes that are too small.  */
> -      if ((bitpos_ % unit) + bitsize_ > unit)
> -	continue;
> -
>        /* Stop if the mode is too wide to handle efficiently.  */
>        if (unit > MAX_FIXED_MODE_SIZE)
>  	break;
> @@ -2683,11 +2679,18 @@ bit_field_mode_iterator::next_mode (enum
>        if (count_ > 0 && unit > BITS_PER_WORD)
>  	break;
>  
> +      /* Skip modes that are too small.  */
> +      unsigned HOST_WIDE_INT substart = (unsigned HOST_WIDE_INT) bitpos_ % unit;
> +      unsigned HOST_WIDE_INT subend = substart + bitsize_;
> +      if (subend > unit)
> +	continue;
> +
>        /* Stop if the mode goes outside the bitregion.  */
> -      HOST_WIDE_INT start = bitpos_ - (bitpos_ % unit);
> +      HOST_WIDE_INT start = bitpos_ - substart;
>        if (bitregion_start_ && start < bitregion_start_)
>  	break;
> -      if (start + unit > bitregion_end_ + 1)
> +      HOST_WIDE_INT end = start + unit;
> +      if (end > bitregion_end_ + 1)
>  	break;
>  
>        /* Stop if the mode requires too much alignment.  */