Re: [wide-int] out-of-range set_bit in java

2014-05-05 Thread Richard Biener
On Fri, May 2, 2014 at 5:20 PM, Richard Sandiford
rsand...@linux.vnet.ibm.com wrote:
 I locally tried adding an assertion to the wide-int version of set_bit
 to make sure that the bit number was in range.  It triggers for this
 code in boehm.c:mark_reference_fields (quoting trunk version):

   /* First word in object corresponds to most significant byte of
  bitmap.

  In the case of a multiple-word record, we set pointer
  bits for all words in the record. This is conservative, but the
  size_words != 1 case is impossible in regular java code. */
   for (i = 0; i  size_words; ++i)
 *mask = (*mask).set_bit (ubit - count - i - 1);

   if (count = ubit - 2)
 *pointer_after_end = 1;

 if count + i + 1 = ubit.

 AIUI the lower 2 bits are used for something else:

   /* Bottom two bits for bitmap mark type are 01.  */
   mask = mask.set_bit (0);
   value = double_int_to_tree (value_type, mask);

 which is why the pointer_after_end condition checks for count = ubit - 2.
 We never actually use the mask if pointer_after_end is true, so this
 patch puts the set_bit in an else branch.

 On face value it looks like the condition should be:

   count + size_words  ubit - 2

 instead, but it'd go without saying that I don't really understand this code.

 Tested on x86_64-linux-gnu and powerpc64-linux-gnu for wide-int.
 OK to install?

Ok.

Thanks,
Richard.

 Thanks,
 Richard


 gcc/java/
 * boehm.c (mark_reference_fields): Don't update the mask when
 setting pointer_after_end.

 Index: gcc/java/boehm.c
 ===
 --- gcc/java/boehm.c2014-01-13 15:05:22.543887284 +
 +++ gcc/java/boehm.c2014-05-02 16:08:25.500760537 +0100
 @@ -101,17 +101,17 @@ mark_reference_fields (tree field,

   *last_set_index = count;

 - /* First word in object corresponds to most significant byte of
 -bitmap.
 -
 -In the case of a multiple-word record, we set pointer
 -bits for all words in the record. This is conservative, but the
 -size_words != 1 case is impossible in regular java code. */
 - for (i = 0; i  size_words; ++i)
 -   *mask = wi::set_bit (*mask, ubit - count - i - 1);
 -
   if (count = ubit - 2)
 *pointer_after_end = 1;
 + else
 +   /* First word in object corresponds to most significant byte of
 +  bitmap.
 +
 +  In the case of a multiple-word record, we set pointer
 +  bits for all words in the record. This is conservative, but the
 +  size_words != 1 case is impossible in regular java code. */
 +   for (i = 0; i  size_words; ++i)
 + *mask = wi::set_bit (*mask, ubit - count - i - 1);

   /* If we saw a non-reference field earlier, then we can't
  use the count representation.  We keep track of that in



[wide-int] out-of-range set_bit in java

2014-05-02 Thread Richard Sandiford
I locally tried adding an assertion to the wide-int version of set_bit
to make sure that the bit number was in range.  It triggers for this
code in boehm.c:mark_reference_fields (quoting trunk version):

  /* First word in object corresponds to most significant byte of 
 bitmap. 
 
 In the case of a multiple-word record, we set pointer 
 bits for all words in the record. This is conservative, but the 
 size_words != 1 case is impossible in regular java code. */
  for (i = 0; i  size_words; ++i)
*mask = (*mask).set_bit (ubit - count - i - 1);

  if (count = ubit - 2)
*pointer_after_end = 1;

if count + i + 1 = ubit.

AIUI the lower 2 bits are used for something else:

  /* Bottom two bits for bitmap mark type are 01.  */
  mask = mask.set_bit (0);
  value = double_int_to_tree (value_type, mask);

which is why the pointer_after_end condition checks for count = ubit - 2.
We never actually use the mask if pointer_after_end is true, so this
patch puts the set_bit in an else branch.

On face value it looks like the condition should be:

  count + size_words  ubit - 2

instead, but it'd go without saying that I don't really understand this code.

Tested on x86_64-linux-gnu and powerpc64-linux-gnu for wide-int.
OK to install?

Thanks,
Richard


gcc/java/
* boehm.c (mark_reference_fields): Don't update the mask when
setting pointer_after_end.

Index: gcc/java/boehm.c
===
--- gcc/java/boehm.c2014-01-13 15:05:22.543887284 +
+++ gcc/java/boehm.c2014-05-02 16:08:25.500760537 +0100
@@ -101,17 +101,17 @@ mark_reference_fields (tree field,
 
  *last_set_index = count;
 
- /* First word in object corresponds to most significant byte of 
-bitmap. 
-
-In the case of a multiple-word record, we set pointer 
-bits for all words in the record. This is conservative, but the 
-size_words != 1 case is impossible in regular java code. */
- for (i = 0; i  size_words; ++i)
-   *mask = wi::set_bit (*mask, ubit - count - i - 1);
-
  if (count = ubit - 2)
*pointer_after_end = 1;
+ else
+   /* First word in object corresponds to most significant byte of 
+  bitmap. 
+
+  In the case of a multiple-word record, we set pointer 
+  bits for all words in the record. This is conservative, but the 
+  size_words != 1 case is impossible in regular java code. */
+   for (i = 0; i  size_words; ++i)
+ *mask = wi::set_bit (*mask, ubit - count - i - 1);
 
  /* If we saw a non-reference field earlier, then we can't
 use the count representation.  We keep track of that in