[OpenJDK 2D-Dev] RFR 8144445: Maximum size checking in Marlin ArrayCache utility methods is not optimal

[Laurent Bourgès]

Hi Jim,

I took me some time to understand your detailled analysis and make a new
webrev:
http://cr.openjdk.java.net/~lbourges/marlin/marlin-8144445.2/

I think I adopted all your change proposals, see below:

In looking this over again I notice that really the exceptions are only
> thrown on "out of range" needSize values and they are probably always
> thrown in that case, but sometimes we only discover those range issues if
> other things fail and many of the tests that lead to getting to that
> statement may or may not operate correctly depending on whether they are
> dealing with a valid needSize (for instance testing against "< needSize"
> when needSize might be negative, etc.).  It makes verifying this code much
> more difficult due to having to deal with cases where multiple variables
> might be out of range when they are computed or compared against each
> other.  To that end, it might be more straightforward to simply test
> needSize at the top and then the rest of the function can know it is
> dealing with a proper value for needSize and only has to worry about
> returning a non-overflowing number.  Knowing that needSize is a valid
> number makes any computations with it or compares against it have fewer
> "failure conditions" to note and vet.
>

I followed your approach and I agree it is more clear.

>
> For example:
>
> first function:
>
> 185-189 move to the top of the function and only test needSize and then
> later on line 177 we capture any overflow since we know that needSize
> cannot be negative as well.  180,182 are then sufficient to make sure that
> the value calculated in that case will be >= needSize.
>

Fixed.


> second function:
>
> 215-220 also move to the top of that function and 214 (if it compares size
> in both cases) is sufficient to make sure we are returning a value >=
> needSize in all cases.  As it stands 210 and 212 could be computing random
> values and the tests at 214 and later are no longer complicated by having
> to consider the case that needSize itself is wrong - they only have to deal
> with whether the returned size bumped out of range.
>

Fixed.


> general note:
>
> Also, "(longval < 0L || longval > MAX_INT)" can be calculated as
> "((longval >> 31) != 0)".
>

Thanks for the tip: I use it now.


> I could also check size < 0L if you want but it is only possible if
>> curSize < 0 (that should never happen) !
>>
>
> That may be true at line 209, but then you test it against needSize and do
> more calculations where the new value of size depends only on needSize and
> so its dependence on curSize no longer offers any protection.  At 214 you
> might not be able to assert that size>=0 any more as a result.
>
>   That works.  I was thinking more along the lines of this which is more
>> straightforward:
>>
>
> try {
>     do test;
>     throw new RuntimeException("AIOBException not thrown");
> } catch (AIOBException e) (
>     return;
> } catch (Throwable t) {
>     throw new RuntimeException("Wrong exception (not AIOB) thrown", t);
> }
>

Fixed.

Cheers,
Laurent
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/2d-dev/attachments/20151209/39ba7e29/attachment.html>