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

Mon Apr 23 07:08:03 UTC 2018

Hi,

> I still don't like the array type information - it's not relevant to the
> error IMHO.
> 
> That aside, for the array copy cases I'd go for something like:
> 
> arraycopy src index -1 out of bounds for double[10]
> arraycopy dest index -1 out of bounds for double[10]
> arraycopy end src index 11 out of bounds for double[10]
> arraycopy end dest index 11 out of bounds for double[10]
It's a good idea to point out it's the end of the region to copy.
But I would find "last src index" better to understand than
"end src index":
"last arraycopy src index 11 out of bounds for double[10]"

> arraycopy length -19 is negative
> 
> In the final case the array lengths are not relevant.
I think they might help in case the length computation depends on the 
array lengths, which is quite likely the case. But I'll leave it out.

> > I'll reply to the other points in a comprehensive mail when I know
> > how to put the message.
... these all are not related to the wording of the message.
> 
> I don't oppose additional useful information in exception messages, but
> if it not directly relevant it just makes the error message harder to
> decipher IMO. 
SAP JVM tries to give all information useful to track down the cause.
In many situations the printed information might be superfluous, but
in this one critical situation you will be very happy to have it.  SAP builds
'real' software on Java, not just 100 line jtreg tests ... 
But the message should stay clear. Therefore I also don't like the 
1-sentence rule you sometimes mention. Several sentences can give
structure to distinguish root cause information and additional information.
Not relevant here currently, though.

> I also don't want to pay additional runtime costs just to
> make an exception message a little bit clearer.
Yes, especially it should not impose any cost on the case without exception.

Best regards,
  Goetz.

> Thanks,
> David
> 
> > Thanks,
> >    Goetz.
> >
> >
> >> -----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.
> >>
> >> 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!
> >>
> >>> It's because I extracted this change from a bigger one. Our message
> reads
> >> like this:
> >>> Object [] oa1 = new Object[10]
> >>> oa1[12]
> >>> "ArrayIndexOutOfBoundsException while trying to load from index 12 of
> an
> >> object array with length 10, loaded from local variable 'oa1'"
> >>> ... which seems not optimal, either. But it mentions the type (object),
> the
> >> operation (load, store etc ...) and the variable name.
> >>> Naming the array is quite detailed if it is in a complex expression (like
> >> returned from a call).
> >>> I'll contribute more of this if appreciated, this is a first step.
> >>
> >> I've never thought this complexity was warranted. We've had a few RFE's
> >> of this nature over the years and they have been closed (not necessarily
> >> by me I should point out!).
> >>
> >>> Printing "index N is outside range [0, length-1]" is problematic
> >>> for length '0'. I followed the proposal by Roger:
> >>> "Index -1 out-of-bounds for length 10."
> >>
> >> You can easily special-case length 0. But Roger's proposal for
> >> consistency with existing messages make sense - not withstanding
> >> Andrew's dislike for the hyphens.
> >>
> >>> I removed the change to ArrayIndexOutOfBoundsException.java.
> >>
> >> That's good.
> >>
> >>> 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.
> >>
> >> 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.
> >>>>>
> >>>>>