RFR(M): 8201593: Print array length in ArrayIndexOutOfBoundsException.

Tue Apr 24 14:08:02 UTC 2018

Hi, 

I implemented what we figured out in 
http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2018-April/027555.html

Some examples:
"Index 12 out-of-bounds for length 10."
"arraycopy source index -17 out of bounds for object array[10]."
"arraycopy destination index -18 out of bounds for double[5]."
"arraycopy length -19 is negative."
"arraycopy: last source index 13 out of bounds for double[10]."
"arraycopy: last destination index 7 out of bounds for long[5]."

Incremental webrev:
http://cr.openjdk.java.net/~goetz/wr18/8201593-lenInAIOOB/03-incremental/
Full webrev:
http://cr.openjdk.java.net/~goetz/wr18/8201593-lenInAIOOB/03/

I'll push it through our tests tonight.

See further comments in line:

> -----Original Message-----
> From: David Holmes [mailto:david.holmes at oracle.com]
> Sent: Freitag, 20. April 2018 09:25
> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; hotspot-runtime-
> dev at openjdk.java.net; hotspot-compiler-dev at openjdk.java.net; aarch64-
> port-dev at openjdk.java.net; aarch32-port-dev at openjdk.java.net; core-libs-
> dev Libs <core-libs-dev at openjdk.java.net>
> Subject: Re: RFR(M): 8201593: Print array length in
> ArrayIndexOutOfBoundsException.
> 
> Hi Goetz,
> 
> This is not a file by file review ...
> 
> On 19/04/2018 10:24 PM, Lindenmaier, Goetz wrote:
> > Hi,
> >
> > New webrev:
> > http://cr.openjdk.java.net/~goetz/wr18/8201593-lenInAIOOB/02/
> >
> > I admit my wording is not optimal.
> 
> src/hotspot/share/oops/typeArrayKlass.cpp
> 
> Sorry but this is still far too verbose for my tastes. The type of the
> array is not relevant. For the array copy its okay to indicate src or
> dst array. But the message should be clear and succinct not prose-like.
> Plus you have a lot of repetition in the ss.print statements when only
> one thing is changing.
We discussed this in some further mails. Implemented as proposed there.

> src/hotspot/cpu/x86/c1_CodeStubs_x86.cpp
> 
> I'm not seeing why the throw_index_out_of_bounds_exception should be
> tied to whether or not you have an array reference. Certainly I hate
> seeing the array ref being used as an implicit bool!
I split the constructor into two, one for ArrayIndexOutOfBounds, the other
for IndexOutOfBounds.

...

> > I extended the test to cover the exception thrown in arraycopy better
> 
> The test seems somewhat excessive, and I now see it is because of the
> more elaborate error messages you have at SAP. But with only the index
> and the array length of interest here the test can be considerably smaller.
> 
> The creation tests for ArrayIndexOutOfBoundsException don't seem
> relevant in this context either. This looks more TCK like.
Yes, the constructor tests are for the code not yet contributed.
I simplified the tests to only check the messages.

Best regards,
  Goetz.

> 
> David
> -----
> 
> > and added the elementary type to the message text.  This probably
> > needs improvement in the text, too.  There are (currently) these cases:
> >
> > bject[]  oa1 = new Object[10];
> > Object[]  oa2 = new Object[5];
> > System.arraycopy(oa1, -17, oa2, 0, 5);
> > "while trying to copy from index -17 of an object array with length 10");
> > System.arraycopy(oa1, 2, oa2, -18, 5);
> > "while trying to copy to index -18 of an object array with length 5");
> > System.arraycopy(oa1, 2, oa2, 0, -19);
> > "while trying to copy a negative range -19 from an object array with length
> 10 to an object array with length 5");
> > System.arraycopy(oa1, 8, oa2, 0, 5);
> > "while trying to copy from index 13 of an object array with length 10");
> > System.arraycopy(oa1, 1, oa2, 0, 7);
> > "while trying to copy to index 7 of an object array with length 5");
> > double[]  ta1 = new double[10];
> > double[]  ta2 = new double[5];
> > System.arraycopy(ta1, -17, ta2, 0, 5);
> > "while trying to copy from index -17 of a doubl array with length 10");
> > System.arraycopy(ta1, 2, ta2, -18, 5);
> > "while trying to copy to index -18 of a double array with length 5");
> > System.arraycopy(ta1, 2, ta2, 0, -19);
> > "while trying to copy a negative range -19 from a double array with length
> 10 to a double array with length 5");
> > System.arraycopy(ta1, 8, ta2, 0, 5);
> > "while trying to copy from index 13 of a double array with length 10");
> > System.arraycopy(ta1, 1, ta2, 0, 7);
> > "while trying to copy to index 7 of a double array with length 5");
> >
> > Maybe it should say:
> > Arraycopy source index -1 out-of-bounds for double array of length 10.
> > Arraycopy target index 10 out-of-bounds for object array of length 10.
> > Negative range -19 when copying from an object array of length 10 to an
> object array of length 5.
> >
> > Best regards,
> >    Goetz.
> >
> >> -----Original Message-----
> >> From: David Holmes [mailto:david.holmes at oracle.com]
> >> Sent: Mittwoch, 18. April 2018 10:55
> >> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; hotspot-runtime-
> >> dev at openjdk.java.net; hotspot-compiler-dev at openjdk.java.net;
> aarch64-
> >> port-dev at openjdk.java.net; aarch32-port-dev at openjdk.java.net; core-
> libs-
> >> dev Libs <core-libs-dev at openjdk.java.net>
> >> Subject: Re: RFR(M): 8201593: Print array length in
> >> ArrayIndexOutOfBoundsException.
> >>
> >> Adding core-libs-dev as you're changing
> >> java.lang.ArrayIndexOutOfBoundsException.
> >>
> >> I appreciate the intent here but I find the messages excessively
> >> verbose. The basic error is:
> >>
> >> index N is outside range [0, length-1]
> >>
> >> David
> >>
> >> On 18/04/2018 6:09 PM, Lindenmaier, Goetz wrote:
> >>> Hi,
> >>>
> >>> I would like to print a more verbose text on ArrayIndexOutOfBounds
> >> exception
> >>> that not only mentions the index, but also the length of the array
> accessed.
> >>> See the bug for documentation of the change of the message.
> >>> http://cr.openjdk.java.net/~goetz/wr18/8201593-lenInAIOOB/01/
> >>>
> >>> @aarch/arm people:
> >>> I edited the aarch/arm files. Could you please verify this is correct?
> >>> I can not build on these platforms.
> >>>
> >>> The code on all the other platforms is tested with all the jtreg and jck
> tests
> >> etc.
> >>>
> >>> Best regards,
> >>>     Goetz.
> >>>
> >>>