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

[Thomas Stüfe]

Hi Aleksey,

change looks fine. Some small remarks:

http://cr.openjdk.java.net/~shade/8211926/webrev.05/src/hotspot/share/utilities/bitMap.inline.hpp.udiff.html

+  assert(beg <= end, "underflow");

I would feel slightly safer with a runtime check for product as well.

--

http://cr.openjdk.java.net/~shade/8211926/webrev.05/test/hotspot/gtest/utilities/test_bitMap_large.cpp.html

The tests favor the left side (lower space) of the bitmap, since l
never grows beyond FUZZ_WINDOW. I wonder whether testing the right
side (small ranges near the bitmap end) would be useful. Maybe with a
gliding window, or just repeat the tests anchored at the right instead
of the left side.

--

Side notes (nothing to do with your patch):

- BitMap::set_range_within_word, clear_range... : would like to see
asserts here for beg <= end.

- I find it confusing that we use the same type (idx_t) both for word-
and bit-indices. It would make the code a bit more readable if those
were separate types (even if they both were to alias to size_t,
ultimately).

- Generally, I find putting the decision between "large" and
"non-large" APIs off to the caller a bit strange. As a user of bitmap,
I cannot know at which point which variant would be better. I would
prefer the class itself handling that.

--

All these are idle musings, nothing more. The change is fine to me as it is.

..Thomas

On Sat, Nov 10, 2018 at 12:32 AM Aleksey Shipilev <shade at redhat.com> wrote:
>
> On 11/09/2018 09:02 PM, Kim Barrett wrote:
> > Almost, but not quite, I'm afraid.  All but the first comment below
> > are optional, though I think make things better.
>
> Introduced helper method, moved the small_range_words to private, handled (l == r) corner case in
> test. See:
>   http://cr.openjdk.java.net/~shade/8211926/webrev.05/
>
> -Aleksey
>
> P.S. For the love of any deity of choice, can we please push this corner-case fix and move on to
> solve meatier bugs? I appreciate the attention to details, but the economics of spending any more
> time to this issue feels dubious to me.
>