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:[email protected]] > Sent: Freitag, 20. April 2018 09:25 > To: Lindenmaier, Goetz <[email protected]>; hotspot-runtime- > [email protected]; [email protected]; aarch64- > [email protected]; [email protected]; core-libs- > dev Libs <[email protected]> > 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:[email protected]] > >> Sent: Mittwoch, 18. April 2018 10:55 > >> To: Lindenmaier, Goetz <[email protected]>; hotspot-runtime- > >> [email protected]; [email protected]; > aarch64- > >> [email protected]; [email protected]; core- > libs- > >> dev Libs <[email protected]> > >> 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. > >>> > >>>
