RFR: 8213415: BitMap::word_index_round_up overflow problems

[Kim Barrett]

> On Jun 3, 2019, at 4:16 AM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
> 
> Hi,
> 
> On Tue, 2019-05-28 at 14:10 -0400, Kim Barrett wrote:
>> Please review this change to BitMap index alignment to check for
>> overflows (in debug builds).  Also made a few related API cleanups.
>> 
>> Constructing a BitMap now ensures the size is such that rounding it
>> up to a word boundary won't overflow.  This is the new
>> max_size_in_bits() value. This lets us add some asserts and otherwise
>> tidy things up in some places by making use of that information.
>> 
>> This engendered some changes to ParallelGC's ParMarkBitMap.  It no
>> longer uses the obsolete BitMap::word_align_up, instead having its
>> own internal helper for aligning range ends that knows about
>> invariants in ParMarkBitMap.
>> 
>> Removed the following static functions from BitMap: word_align_up,
>> word_align_down, and is_word_aligned.  word_align_up is no longer
>> used, after the ParMarkBitMap changes.  The others were only used in
>> a small number of places in tests and asserts, and didn't seem worth
>> keeping instead of just using align.hpp stuff with BitsPerWord
>> alignment value.
>> 
>> CR:                                 
>> https://bugs.openjdk.java.net/browse/JDK-8213415
>> 
>> Webrev:
>> http://cr.openjdk.java.net/~kbarrett/8213415/open.00/
>> 
>> Testing:
>> mach5 tier1-3
> 
> Minor comment:
> 
> - bitmap.hpp:
> 
>  52 // the bitmap.  BitsPerWord bits per element.
> 
> The second part is not a sentence. Maybe join these with a "with"
> instead of the full stop? Or "An element has BitsPerWord bits”.

Fixed.

> Not sure this fact actually needs mention though. Maybe it would be
> useful to do a static assert like the following somewhere:
> 
> STATIC_ASSERT(sizeof(bm_word_t) * BitsPerByte == BitsPerWord);
> 
> To actually enforce this and not only talk about it in some comment?

There are a bunch of places that assume this, currently using
BitsPerWord directly.  I considered adding a BitMap constant for this
value, but decided to just document it.  I've added an assertion in
the BitMap class (so no longer _just_ a comment), but that won't help
find places that are broken if the assertion ever fails.

> It's up to you whether you pick up these suggestions.
> 
> Looks good.

Thanks.

> 
> Thanks,
>  Thomas