diff mbox

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

Message ID 20110414185053.6C532A00D@topo.tor.corp.google.com
State New
Headers show

Commit Message

Diego Novillo April 14, 2011, 6:50 p.m. UTC 
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.


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

Comments

Jakub Jelinek April 14, 2011, 7:28 p.m. UTC | #1 
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
diff mbox

Patch

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)