| Message ID | 87y5hwva2d.fsf@sandifor-thinkpad.stglab.manchester.uk.ibm.com |
|---|---|
| State | New |
| Headers | show |
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. */
> gcc/ > * stor-layout.c (bit_field_mode_iterator::next_mode): Fix signedness. This looks fine to me.
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. */