RFR(M): 8201593: Print array length in ArrayIndexOutOfBoundsException.
Lindenmaier, Goetz
goetz.lindenmaier at sap.com
Wed Apr 25 15:17:31 UTC 2018
Hi Martin,
Thanks for the review!
I edited the comments and the one register as proposed.
All makes sense. I also used LP64_ONLY etc, although I think
it's got nothing to do with the length of data types, but with
the architecture (which is expressed by ia32/amd64).
New webrev:
http://cr.openjdk.java.net/~goetz/wr18/8201593-lenInAIOOB/04/
This also contains one fix for the windows build.
The change is going into our tests tonight.
Best regards,
Goetz.
> -----Original Message-----
> From: Doerr, Martin
> Sent: Dienstag, 24. April 2018 18:16
> To: Stuart Monteith <stuart.monteith at linaro.org>; Lindenmaier, Goetz
> <goetz.lindenmaier at sap.com>
> Cc: hotspot-compiler-dev at openjdk.java.net; aarch64-port-
> dev at openjdk.java.net; hotspot-runtime-dev at openjdk.java.net; aarch32-
> port-dev at openjdk.java.net
> Subject: RE: RFR(M): 8201593: Print array length in
> ArrayIndexOutOfBoundsException.
>
> Hi Götz,
>
> I've reviewed the platform code except aarch64/arm and I have some
> requests for comment improvements.
>
> c1_CodeStubs_ppc:
> - Remove comment "// pass in R0".
> - Use std(index, -16, R1_SP).
>
> templateInterpreterGenerator_ppc.cpp:
> - A comment would be good: "R4_ARG2 contains the array."
>
> interp_masm_sparc.cpp:
> - Comment "move aberrant index into G3_scratch" needs update after
> register change.
>
> templateInterpreterGenerator_sparc.cpp:
> - Comment "index in register G3_scratch" ... "index to G4_scratch" is no
> longer correct. Better would be "G3_scratch contains the array. Otos_i
> contains the index."
>
> c1_Runtime1_x86:
> - Better would be to pass num_arguments instead of bool has_argument,
> but it works this way, too. At least the comment describing has_argument
> should get fixed.
>
> templateInterpreterGenerator_x86.cpp:
> - I think the comment "// Pass array to create more detailed exceptions."
> should be in front of the rarg assignment.
>
> templateTable_x86.cpp
> - I think LP64_ONLY + NOT_LP64 would be better than #ifdef IA32.
>
> Thanks and best regards,
> Martin
>
>
> -----Original Message-----
> From: hotspot-compiler-dev [mailto:hotspot-compiler-dev-
> bounces at openjdk.java.net] On Behalf Of Stuart Monteith
> Sent: Dienstag, 24. April 2018 14:28
> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>
> Cc: hotspot-compiler-dev at openjdk.java.net; aarch64-port-
> dev at openjdk.java.net; hotspot-runtime-dev at openjdk.java.net; aarch32-
> port-dev at openjdk.java.net
> Subject: Re: RFR(M): 8201593: Print array length in
> ArrayIndexOutOfBoundsException.
>
> Hello Goetz,
> It appears that it is just C1 that exhibits the problem - there has
> been a delay as I've been investigating a hang in Java JDK running
> some of my automation.
>
> I'll try and track down the issue. Otherwise, I do like the principle
> of the patch.
>
> BR,
> Stuart
>
>
> On 24 April 2018 at 11:44, Lindenmaier, Goetz <goetz.lindenmaier at sap.com>
> wrote:
> > Hi Stuart,
> >
> > could you track down the remaining issue?
> >
> > Best regards,
> > Goetz.
> >
> >> -----Original Message-----
> >> From: Stuart Monteith [mailto:stuart.monteith at linaro.org]
> >> Sent: Donnerstag, 19. April 2018 17:09
> >> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>
> >> Cc: 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
> >> Subject: Re: RFR(M): 8201593: Print array length in
> >> ArrayIndexOutOfBoundsException.
> >>
> >> Hi,
> >> I'm trying to work through what's going on here. With C1 not
> >> crashing, I'll check the behaviour of the interpreter and C2. I'm
> >> doing explicitly with -Xint, etc. WIth -Xint I definitely get the
> >> error.
> >>
> >> You have the condition the wrong way round -
> >> _throw_index_out_of_bounds_exception is true when array is NULL.
> >>
> >>
> >> BR,
> >> Stuart
> >>
> >>
> >> On 19 April 2018 at 15:36, Lindenmaier, Goetz
> <goetz.lindenmaier at sap.com>
> >> wrote:
> >> > Hi Stuart,
> >> >
> >> > thanks a lot for helping out here.
> >> > I added below patch to my latest webrev (replaced it in-place):
> >> > http://cr.openjdk.java.net/~goetz/wr18/8201593-lenInAIOOB/02/
> >> >
> >> > The remaining problem looks like the array argument does not
> >> > reach the method that prints the message properly.
> >> > Did I get a register wrong?
> >> > Did I use a wrong argument register, or does the untangling
> >> > of registers in overwrite something?
> >> > Is it correct to use mov and not movw in
> >> templateInterpreterGenerator_aarch64.cpp?
> >> > Is it safe to use r3 in templateTable_aarch64.cpp? (The "convention"
> here is
> >> stupid,
> >> > it seems to cause unnecessary register moves. But it's similar on all
> >> platforms.)
> >> >
> >> > The test runs several times, assuring it's C1, C2 and Xint.
> >> > This should help you to narrow down the problem.
> >> >
> >> > Best regards,
> >> > Goetz.
> >> >
> >> >
> >> >
> >> >
> >> >
> >> >
> >> >
> >> >
> >> > diff -r 3fe33d48aa16
> src/hotspot/cpu/aarch64/c1_CodeStubs_aarch64.cpp
> >> > --- a/src/hotspot/cpu/aarch64/c1_CodeStubs_aarch64.cpp Mon Apr 16
> >> 15:17:20 2018 +0200
> >> > +++ b/src/hotspot/cpu/aarch64/c1_CodeStubs_aarch64.cpp Thu Apr 19
> >> 16:06:23 2018 +0200
> >> > @@ -73,9 +73,10 @@
> >> > } else {
> >> > __ mov(rscratch1, _index->as_jint());
> >> > }
> >> > - __ mov(rscratch2, _array->as_pointer_register());
> >> > Runtime1::StubID stub_id;
> >> > if (_throw_index_out_of_bounds_exception) {
> >> > + assert(_array != NULL, "sanity");
> >> > + __ mov(rscratch2, _array->as_pointer_register());
> >> > stub_id = Runtime1::throw_index_exception_id;
> >> > } else {
> >> > stub_id = Runtime1::throw_range_check_failed_id;
> >> >
> >> >
> >> >
> >> >
> >> >> -----Original Message-----
> >> >> From: Stuart Monteith [mailto:stuart.monteith at linaro.org]
> >> >> Sent: Donnerstag, 19. April 2018 14:26
> >> >> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>
> >> >> Cc: 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
> >> >> Subject: Re: RFR(M): 8201593: Print array length in
> >> >> ArrayIndexOutOfBoundsException.
> >> >>
> >> >> Hello Goetz,
> >> >> In c1_CodeStubs_aarch64.cpp, RangeCheckStub::emit_code you just
> >> >> need to guard the emission of rscratch2 - you don't use it in one of
> >> >> the cases, something like:
> >> >>
> >> >> if (!_throw_index_out_of_bounds_exception) {
> >> >> __ mov(rscratch2, _array->as_pointer_register());
> >> >> }
> >> >>
> >> >>
> >> >> Once I can run the unit tests, I'm seeing failures in your unit test like:
> >> >>
> >> >> java.lang.AssertionError: expected [trying to access index -5 of an
> >> >> array with length 0] but found [trying to access index -5 of an array
> >> >> with length -1459548190]
> >> >>
> >> >>
> >> >> BR,
> >> >> Stuart
> >> >>
> >> >>
> >> >> On 18 April 2018 at 09:09, Lindenmaier, Goetz
> >> <goetz.lindenmaier at sap.com>
> >> >> 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.
> >> >> >
> >> >> >
More information about the aarch32-port-dev
mailing list