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

[Kim Barrett]

> On Nov 8, 2018, at 4:38 AM, Aleksey Shipilev <shade at redhat.com> wrote:
> http://cr.openjdk.java.net/~shade/8211926/webrev.02/
> 
> This still passes new gtest and hotspot-tier1. Does the patch make more sense now?

------------------------------------------------------------------------------ 
src/hotspot/share/utilities/bitMap.inline.hpp
240   assert(beg <= end, "underflow");
245   assert(beg <= end, "underflow");

These new checks should instead be using verify_range.

Sorry I didn't notice this earlier.

------------------------------------------------------------------------------
src/hotspot/share/utilities/bitMap.hpp
 62   static const size_t SMALL_RANGE_WORDS = 32;

Good to have this value as a named constant.  However, while there
doesn't seem to be a consistent naming convention for constants in
HotSpot, the other constants in *this* class have lower-case names.

------------------------------------------------------------------------------ 
src/hotspot/share/utilities/bitMap.cpp
277   assert(beg_full_word < end_full_word,
278          "Performance: The range includes at least one full word");

295   assert(beg_full_word < end_full_word,
296          "Performance: The range includes at least one full word");

For either of these assertions to trip, the value of SMALL_RANGE_WORDS
would have needed to be poorly chosen.  Better would be a static
assertion on the value of SMALL_RANGE_WORDS.

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

> I'd like to keep the change small and contained, without significant rewrites, to simplify backports
> without introducing new bugs. If additional check is significant on some path, users can just go to
> non-large-hinted method versions.

My suggested revision was not about any (uninteresting) performance
impact of the extra check to avoid underflow.  It was about improving
the readability of the code by eliminating the possibility of
underflow altogether.  (That it has the side-benefit of picking up
some micro-optimizations along the way is really not the point.)  The
new initial check in the webrev change probably needs some additional
commentary; none of the authors or reviewers of the original code or
the JDK-8152188 modification noticed the potential underflow, and it
wasn't immediately obvious (to me at least) what that new check is for
(I had to go re-read the bug report when I initially started reviewing).

I don't buy the backport argument here.  set_large_range is untouched
since the initial mercurial checkin.  The only change since then to
clear_large_range is JDK-8152188 (JDK 9), which changed the similar
assert there to the potentially underflowing conditional call to
set_range that is being addressed here.