RFR (S) 8211926: Catastrophic size_t underflow in BitMap::*_large methods

[Kim Barrett]

> On Nov 9, 2018, at 4:33 AM, Aleksey Shipilev <shade at redhat.com> wrote:
> 
> On 11/09/2018 02:36 AM, Kim Barrett wrote:
>>> I believe new patch is as straightforward as it could be, see:
>>> http://cr.openjdk.java.net/~shade/8211926/webrev.03/
>> 
>> I was about ready to approve, but took another look at the new test.
>> Unfortunately, I think it has some problems.
> 
> Okay, let's try again after fixing the problems you mentioned:
>  http://cr.openjdk.java.net/~shade/8211926/webrev.04/
> 
> Moved STATIC_ASSERT and comments to the actual use site, rewritten test. Test takes 0.5s per TEST().
> Looks good now?
> 
> -Aleksey

Almost, but not quite, I'm afraid.  All but the first comment below
are optional, though I think make things better.

------------------------------------------------------------------------------
test/hotspot/gtest/utilities/test_bitMap_large.cpp 
  54   for (size_t size_class = 1; size_class <= BITMAP_SIZE; size_class *= 2) {
  55     for (BitMap::idx_t l = 0; l < FUZZ_WINDOW; l++) {
  56       for (BitMap::idx_t tr = l; tr < FUZZ_WINDOW; tr++) {
  57         BitMap::idx_t r = MIN2(BITMAP_SIZE, size_class + tr); // avoid overflow

I generally like the new test structure.  Except there's a bug; this
never tests the case of l == r.

Maybe hoist the inner loop body into a helper function, and add an
empty range call before the loop on tr.

------------------------------------------------------------------------------ 
test/hotspot/gtest/utilities/test_bitMap_large.cpp
  29 static const BitMap::idx_t BITMAP_SIZE = 8192;

Consider instead
const BitMap::idx_t BITMAP_SIZE = 4 * BitMap::bit_index(BitMap::small_range_words);

------------------------------------------------------------------------------
test/hotspot/gtest/utilities/test_bitMap_large.cpp 
  34 static const BitMap::idx_t FUZZ_WINDOW = sizeof(BitMap::bm_word_t) * 8;

Consider instead
const BitMap::idx_t FUZZ_WINDOW = BitMap::bit_index(1);

------------------------------------------------------------------------------
test/hotspot/gtest/utilities/test_bitMap_large.cpp 
  54   for (size_t size_class = 1; size_class <= BITMAP_SIZE; size_class *= 2) {

Using (size_class <= BITMAP_SIZE) rather than < ensures we hit the
case of r == BITMAP_SIZE, which is good.  I thought about other ways
of doing that, but they all make the code more complicated in order to
reduce the number of times we check that case, so probably not worth
the trouble.  But it's a somewhat subtle point, that might be worth a
comment on the use of <= there.

------------------------------------------------------------------------------ 
src/hotspot/share/utilities/bitMap.cpp
 272   // There is little point to call large version on small ranges.
 273   // Need to check carefully, keeping potential idx_t underflow in mind.
 274   // The threshold should be at least one word.
 275   STATIC_ASSERT(small_range_words >= 1);
 276   if (beg_full_word + small_range_words >= end_full_word) {

Consider hoisting this out into a helper function for the two
identical uses.

------------------------------------------------------------------------------