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

[Aleksey Shipilev]

On 11/08/2018 10:41 PM, Kim Barrett wrote:
>> 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.

I don't think they should: verify_range operates on bit indexes, and here beg/end are the word
indexes? While this would always work (famous last words), since word indexes are always below bit
indexes, this would introduce weird dependency on verify_range with implicitly reintepreted
arguments. It seems to me to-the-point asserts win here.

> ------------------------------------------------------------------------------
> 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.

Yes! Fixed.

> ------------------------------------------------------------------------------ 
> 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.

Or, you can think about it this way: this is a precondition for calling the code below, so it should
be near the code that needs it, and it should test the exact invariant that we would like to hold
true. Statically asserting small_range_words runs into risk of missing the errors in conditions
involving small_range_words. But, I have no energy to fight it, reverted.


> 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).

Noted. It looks like what you have experienced can and should be mitigated by adding the comment
near the interesting code. Did so!

I realized (late) that your suggestion has a much nicer check shape (a + b >= c), adopted.

> 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.

What JDK-8152188 teaches me is that seemingly trivial code had introduced the bug, which we have
noticed years later. This is why I am resisting rewriting the code any more than necessary, and
instead willing to point-fix the actual bug, with maybe a little code homogenizing, and provide the
regression test to make sure the exact bug does not happen anymore. This is my backport argument:
make the fix as trivial+obvious as possible to avoid introducing more bugs in backports.

I believe new patch is as straightforward as it could be, see:
  http://cr.openjdk.java.net/~shade/8211926/webrev.03/

Thanks,
-Aleksey