[lto/pph] Do not pack more bits than requested (issue4415044)

[lto/pph] Do not pack more bits than requested (issue4415044)

[Diego Novillo]

This is a corner case that happens not to affect LTO because the "bad"
value is the last one packed.

In pack_ts_type_value_fields, the last thing we pack is
TYPE_ALIAS_SET, which in many cases ends up being -1.  However, we
request bp_pack_value to pack HOST_BITS_PER_INT (32), whereas -1 is 64
bits long on x86_64.  bp_pack_value stores all 64 bits in the word,
but only counts 32.  Any further bit packed will simply be ignored
(since they're OR'd in).

I found this while packing more bits from C++ types, no matter what I
packed, I would get 1s on the reading side.  I fixed it by making
bp_pack_value chop VAL so that only the first NBITS are stored.
Alternately, we could just assert that the caller didn't send a value
that overflows NBITS.

Richi, do you prefer one over the other?


Diego.


	* lto-streamer.h (bitpack_create): Only use the lower NBITS
	from VAL.

diff --git a/gcc/lto-streamer.h b/gcc/lto-streamer.h
index 5f56fc6..0d49430 100644
--- a/gcc/lto-streamer.h
+++ b/gcc/lto-streamer.h
@@ -1190,8 +1190,19 @@ bitpack_create (struct lto_output_stream *s)
 static inline void
 bp_pack_value (struct bitpack_d *bp, bitpack_word_t val, unsigned nbits)
 {
-  bitpack_word_t word = bp->word;
+  bitpack_word_t mask, word;
   int pos = bp->pos;
+
+  word = bp->word;
+
+  gcc_assert (nbits > 0 && nbits <= BITS_PER_BITPACK_WORD);
+
+  /* Make sure that VAL only has the lower NBITS set.  Generate a
+     mask with the lower NBITS set and use it to filter the upper
+     bits from VAL.  */
+  mask = ((bitpack_word_t) 1 << nbits) - 1;
+  val = val & mask;
+
   /* If val does not fit into the current bitpack word switch to the
      next one.  */
   if (pos + nbits > BITS_PER_BITPACK_WORD)

--
This patch is available for review at http://codereview.appspot.com/4415044

Re: [lto/pph] Do not pack more bits than requested (issue4415044)

[Jakub Jelinek]

On Thu, Apr 14, 2011 at 02:50:53PM -0400, Diego Novillo wrote:
> @@ -1190,8 +1190,19 @@ bitpack_create (struct lto_output_stream *s)
>  static inline void
>  bp_pack_value (struct bitpack_d *bp, bitpack_word_t val, unsigned nbits)
>  {
> -  bitpack_word_t word = bp->word;
> +  bitpack_word_t mask, word;
>    int pos = bp->pos;
> +
> +  word = bp->word;
> +
> +  gcc_assert (nbits > 0 && nbits <= BITS_PER_BITPACK_WORD);
> +
> +  /* Make sure that VAL only has the lower NBITS set.  Generate a
> +     mask with the lower NBITS set and use it to filter the upper
> +     bits from VAL.  */
> +  mask = ((bitpack_word_t) 1 << nbits) - 1;

If bitpack_word_t has BITS_PER_BITPACK_WORD bits, then for
nbits = BITS_PER_BITPACK_WORD this will be undefined.
Use say
mask = ((bitpack_word_t) 2 << (nbits - 1)) - 1;
or something similar (assertion ensures that nbits isn't 0).

	Jakub

Re: [lto/pph] Do not pack more bits than requested (issue4415044)

[Diego Novillo]

On Thu, Apr 14, 2011 at 15:28, Jakub Jelinek <jakub@redhat.com> wrote:

> If bitpack_word_t has BITS_PER_BITPACK_WORD bits, then for
> nbits = BITS_PER_BITPACK_WORD this will be undefined.
> Use say
> mask = ((bitpack_word_t) 2 << (nbits - 1)) - 1;
> or something similar (assertion ensures that nbits isn't 0).

Quite right, thanks.  In the meantime, I've changed my mind with this.
 I think it's safer if we just assert that the value we are about to
pack fit in the number of bits the caller specified.

The only problematic user is pack_ts_type_value_fields when it tries
to pack a -1 for the type's alias set.  I think we should just stream
that as an integer and not go through the bitpacking overhead.

For now, I'm applying this to the pph branch.  Tested on x86_64.  No
LTO failures.


Diego.

        * lto-streamer-out.c (pack_ts_type_value_fields): Pack all bits
        of -1 value.
        * lto-streamer.h (bitpack_create): Assert that the value to
        pack does not overflow NBITS.
        * lto-streamer-in.c (unpack_ts_type_value_fields): Unpack
        BITS_PER_BITPACK_WORD bits for TYPE_ALIAS_SET.

diff --git a/gcc/lto-streamer-in.c b/gcc/lto-streamer-in.c
index 97b86ce..f04e031 100644
--- a/gcc/lto-streamer-in.c
+++ b/gcc/lto-streamer-in.c
@@ -1734,7 +1734,7 @@ unpack_ts_type_value_fields (struct bitpack_d
*bp, tree expr)
   TYPE_USER_ALIGN (expr) = (unsigned) bp_unpack_value (bp, 1);
   TYPE_READONLY (expr) = (unsigned) bp_unpack_value (bp, 1);
   TYPE_ALIGN (expr) = (unsigned) bp_unpack_value (bp, HOST_BITS_PER_INT);
-  TYPE_ALIAS_SET (expr) = bp_unpack_value (bp, HOST_BITS_PER_INT);
+  TYPE_ALIAS_SET (expr) = bp_unpack_value (bp, BITS_PER_BITPACK_WORD);
 }


diff --git a/gcc/lto-streamer-out.c b/gcc/lto-streamer-out.c
index 3ccad8b..89ad9c5 100644
--- a/gcc/lto-streamer-out.c
+++ b/gcc/lto-streamer-out.c
@@ -518,7 +518,8 @@ pack_ts_type_value_fields (struct bitpack_d *bp, tree expr)
   bp_pack_value (bp, TYPE_USER_ALIGN (expr), 1);
   bp_pack_value (bp, TYPE_READONLY (expr), 1);
   bp_pack_value (bp, TYPE_ALIGN (expr), HOST_BITS_PER_INT);
-  bp_pack_value (bp, TYPE_ALIAS_SET (expr) == 0 ? 0 : -1, HOST_BITS_PER_INT);
+  bp_pack_value (bp, TYPE_ALIAS_SET (expr) == 0 ? 0 : -1,
+                BITS_PER_BITPACK_WORD);
 }


diff --git a/gcc/lto-streamer.h b/gcc/lto-streamer.h
index 0d49430..73afd46 100644
--- a/gcc/lto-streamer.h
+++ b/gcc/lto-streamer.h
@@ -1190,18 +1190,15 @@ bitpack_create (struct lto_output_stream *s)
 static inline void
 bp_pack_value (struct bitpack_d *bp, bitpack_word_t val, unsigned nbits)
 {
-  bitpack_word_t mask, word;
+  bitpack_word_t word = bp->word;
   int pos = bp->pos;

-  word = bp->word;
-
+  /* We shouldn't try to pack more bits than can fit in a bitpack word.  */
   gcc_assert (nbits > 0 && nbits <= BITS_PER_BITPACK_WORD);

-  /* Make sure that VAL only has the lower NBITS set.  Generate a
-     mask with the lower NBITS set and use it to filter the upper
-     bits from VAL.  */
-  mask = ((bitpack_word_t) 1 << nbits) - 1;
-  val = val & mask;
+  /* The value to pack should not overflow NBITS.  */
+  gcc_assert (nbits == BITS_PER_BITPACK_WORD
+             || val <= ((bitpack_word_t) 1 << nbits));

   /* If val does not fit into the current bitpack word switch to the
      next one.  */

Re: [lto/pph] Do not pack more bits than requested (issue4415044)

[Richard Guenther]

On Thu, 14 Apr 2011, Diego Novillo wrote:

> On Thu, Apr 14, 2011 at 15:28, Jakub Jelinek <jakub@redhat.com> wrote:
> 
> > If bitpack_word_t has BITS_PER_BITPACK_WORD bits, then for
> > nbits = BITS_PER_BITPACK_WORD this will be undefined.
> > Use say
> > mask = ((bitpack_word_t) 2 << (nbits - 1)) - 1;
> > or something similar (assertion ensures that nbits isn't 0).
> 
> Quite right, thanks.  In the meantime, I've changed my mind with this.
>  I think it's safer if we just assert that the value we are about to
> pack fit in the number of bits the caller specified.
> 
> The only problematic user is pack_ts_type_value_fields when it tries
> to pack a -1 for the type's alias set.  I think we should just stream
> that as an integer and not go through the bitpacking overhead.
> 
> For now, I'm applying this to the pph branch.  Tested on x86_64.  No
> LTO failures.

See below
 
> 
> Diego.
> 
>         * lto-streamer-out.c (pack_ts_type_value_fields): Pack all bits
>         of -1 value.
>         * lto-streamer.h (bitpack_create): Assert that the value to
>         pack does not overflow NBITS.
>         * lto-streamer-in.c (unpack_ts_type_value_fields): Unpack
>         BITS_PER_BITPACK_WORD bits for TYPE_ALIAS_SET.
> 
> diff --git a/gcc/lto-streamer-in.c b/gcc/lto-streamer-in.c
> index 97b86ce..f04e031 100644
> --- a/gcc/lto-streamer-in.c
> +++ b/gcc/lto-streamer-in.c
> @@ -1734,7 +1734,7 @@ unpack_ts_type_value_fields (struct bitpack_d
> *bp, tree expr)
>    TYPE_USER_ALIGN (expr) = (unsigned) bp_unpack_value (bp, 1);
>    TYPE_READONLY (expr) = (unsigned) bp_unpack_value (bp, 1);
>    TYPE_ALIGN (expr) = (unsigned) bp_unpack_value (bp, HOST_BITS_PER_INT);
> -  TYPE_ALIAS_SET (expr) = bp_unpack_value (bp, HOST_BITS_PER_INT);
> +  TYPE_ALIAS_SET (expr) = bp_unpack_value (bp, BITS_PER_BITPACK_WORD);
>  }
> 
> 
> diff --git a/gcc/lto-streamer-out.c b/gcc/lto-streamer-out.c
> index 3ccad8b..89ad9c5 100644
> --- a/gcc/lto-streamer-out.c
> +++ b/gcc/lto-streamer-out.c
> @@ -518,7 +518,8 @@ pack_ts_type_value_fields (struct bitpack_d *bp, tree expr)
>    bp_pack_value (bp, TYPE_USER_ALIGN (expr), 1);
>    bp_pack_value (bp, TYPE_READONLY (expr), 1);
>    bp_pack_value (bp, TYPE_ALIGN (expr), HOST_BITS_PER_INT);
> -  bp_pack_value (bp, TYPE_ALIAS_SET (expr) == 0 ? 0 : -1, HOST_BITS_PER_INT);
> +  bp_pack_value (bp, TYPE_ALIAS_SET (expr) == 0 ? 0 : -1,
> +                BITS_PER_BITPACK_WORD);

As we only want to stream alias-set zeros just change it to a single bit,
like

     bp_pack_value (bp, TYPE_ALIAS_SET (expr) == 0, 1);

and on the reader side restore either a zero or -1.

>  }
> 
> 
> diff --git a/gcc/lto-streamer.h b/gcc/lto-streamer.h
> index 0d49430..73afd46 100644
> --- a/gcc/lto-streamer.h
> +++ b/gcc/lto-streamer.h
> @@ -1190,18 +1190,15 @@ bitpack_create (struct lto_output_stream *s)
>  static inline void
>  bp_pack_value (struct bitpack_d *bp, bitpack_word_t val, unsigned nbits)
>  {
> -  bitpack_word_t mask, word;
> +  bitpack_word_t word = bp->word;
>    int pos = bp->pos;
> 
> -  word = bp->word;
> -
> +  /* We shouldn't try to pack more bits than can fit in a bitpack word.  */
>    gcc_assert (nbits > 0 && nbits <= BITS_PER_BITPACK_WORD);

Asserts will break merging the operations and make them slow again.
Please no asserts in these core routines.  Look at the .optimized
dump of a series of bp_pack_value, they should be basically optimized to
a series of ORs.

As for the -1 case, it's simply broken use of the interface.

Richard.

> -  /* Make sure that VAL only has the lower NBITS set.  Generate a
> -     mask with the lower NBITS set and use it to filter the upper
> -     bits from VAL.  */
> -  mask = ((bitpack_word_t) 1 << nbits) - 1;
> -  val = val & mask;
> +  /* The value to pack should not overflow NBITS.  */
> +  gcc_assert (nbits == BITS_PER_BITPACK_WORD
> +             || val <= ((bitpack_word_t) 1 << nbits));
> 
>    /* If val does not fit into the current bitpack word switch to the
>       next one.  */

Re: [lto/pph] Do not pack more bits than requested (issue4415044)

[Diego Novillo]

On Fri, Apr 15, 2011 at 04:56, Richard Guenther <rguenther@suse.de> wrote:

>> @@ -518,7 +518,8 @@ pack_ts_type_value_fields (struct bitpack_d *bp, tree expr)
>>    bp_pack_value (bp, TYPE_USER_ALIGN (expr), 1);
>>    bp_pack_value (bp, TYPE_READONLY (expr), 1);
>>    bp_pack_value (bp, TYPE_ALIGN (expr), HOST_BITS_PER_INT);
>> -  bp_pack_value (bp, TYPE_ALIAS_SET (expr) == 0 ? 0 : -1, HOST_BITS_PER_INT);
>> +  bp_pack_value (bp, TYPE_ALIAS_SET (expr) == 0 ? 0 : -1,
>> +                BITS_PER_BITPACK_WORD);
>
> As we only want to stream alias-set zeros just change it to a single bit,
> like
>
>     bp_pack_value (bp, TYPE_ALIAS_SET (expr) == 0, 1);
>
> and on the reader side restore either a zero or -1.

Ah, yes.  Much better.

> As for the -1 case, it's simply broken use of the interface.

Which would've been caught by the assertion.  How about this, we keep
the asserts with #ifdef ENABLE_CHECKING. This would've saved me some
ugly debugging.


Diego.

Re: [lto/pph] Do not pack more bits than requested (issue4415044)

[Richard Guenther]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1292 bytes --]

On Fri, 15 Apr 2011, Diego Novillo wrote:

> On Fri, Apr 15, 2011 at 04:56, Richard Guenther <rguenther@suse.de> wrote:
> 
> >> @@ -518,7 +518,8 @@ pack_ts_type_value_fields (struct bitpack_d *bp, tree expr)
> >>    bp_pack_value (bp, TYPE_USER_ALIGN (expr), 1);
> >>    bp_pack_value (bp, TYPE_READONLY (expr), 1);
> >>    bp_pack_value (bp, TYPE_ALIGN (expr), HOST_BITS_PER_INT);
> >> -  bp_pack_value (bp, TYPE_ALIAS_SET (expr) == 0 ? 0 : -1, HOST_BITS_PER_INT);
> >> +  bp_pack_value (bp, TYPE_ALIAS_SET (expr) == 0 ? 0 : -1,
> >> +                BITS_PER_BITPACK_WORD);
> >
> > As we only want to stream alias-set zeros just change it to a single bit,
> > like
> >
> >     bp_pack_value (bp, TYPE_ALIAS_SET (expr) == 0, 1);
> >
> > and on the reader side restore either a zero or -1.
> 
> Ah, yes.  Much better.
> 
> > As for the -1 case, it's simply broken use of the interface.
> 
> Which would've been caught by the assertion.  How about this, we keep
> the asserts with #ifdef ENABLE_CHECKING. This would've saved me some
> ugly debugging.

I think we should rather add the masking.  The assert would only
trigger for particular values, not for bogus use of the interface
(you get sign-extension for signed arguments).

Richard.

Re: [lto/pph] Do not pack more bits than requested (issue4415044)

[Diego Novillo]

On Fri, Apr 15, 2011 at 08:49, Richard Guenther <rguenther@suse.de> wrote:
> On Fri, 15 Apr 2011, Diego Novillo wrote:
>
>> On Fri, Apr 15, 2011 at 04:56, Richard Guenther <rguenther@suse.de> wrote:
>>
>> >> @@ -518,7 +518,8 @@ pack_ts_type_value_fields (struct bitpack_d *bp, tree expr)
>> >>    bp_pack_value (bp, TYPE_USER_ALIGN (expr), 1);
>> >>    bp_pack_value (bp, TYPE_READONLY (expr), 1);
>> >>    bp_pack_value (bp, TYPE_ALIGN (expr), HOST_BITS_PER_INT);
>> >> -  bp_pack_value (bp, TYPE_ALIAS_SET (expr) == 0 ? 0 : -1, HOST_BITS_PER_INT);
>> >> +  bp_pack_value (bp, TYPE_ALIAS_SET (expr) == 0 ? 0 : -1,
>> >> +                BITS_PER_BITPACK_WORD);
>> >
>> > As we only want to stream alias-set zeros just change it to a single bit,
>> > like
>> >
>> >     bp_pack_value (bp, TYPE_ALIAS_SET (expr) == 0, 1);
>> >
>> > and on the reader side restore either a zero or -1.
>>
>> Ah, yes.  Much better.
>>
>> > As for the -1 case, it's simply broken use of the interface.
>>
>> Which would've been caught by the assertion.  How about this, we keep
>> the asserts with #ifdef ENABLE_CHECKING. This would've saved me some
>> ugly debugging.
>
> I think we should rather add the masking.  The assert would only
> trigger for particular values, not for bogus use of the interface
> (you get sign-extension for signed arguments).

OK, that works too.  I'll prepare a patch.


Diego.