RFR(M): 8201593: Print array length in ArrayIndexOutOfBoundsException.
Lindenmaier, Goetz
goetz.lindenmaier at sap.com
Mon May 7 21:21:53 UTC 2018
Hi David,
Thanks a lot!
I'll push it tomorrow once our tests have passed.
Best regards,
Goetz.
> -----Original Message-----
> From: David Holmes [mailto:david.holmes at oracle.com]
> Sent: Monday, May 7, 2018 11:03 PM
> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; Stuart Monteith
> <stuart.monteith at linaro.org>
> 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.
>
> Nothing further from me.
>
> Thanks,
> David
>
> On 7/05/2018 10:20 PM, Lindenmaier, Goetz wrote:
> > Hi,
> >
> > Webrev withoug %i:
> > http://cr.openjdk.java.net/~goetz/wr18/8201593-lenInAIOOB/08/
> > it passed the jdk/submit testing.
> >
> > Best regards,
> > Goetz.
> >
> >> -----Original Message-----
> >> From: David Holmes [mailto:david.holmes at oracle.com]
> >> Sent: Montag, 7. Mai 2018 11:00
> >> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; Stuart Monteith
> >> <stuart.monteith at linaro.org>
> >> 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 Goetz,
> >>
> >> On 7/05/2018 6:16 PM, Lindenmaier, Goetz wrote:
> >>> Hi David,
> >>>
> >>> New webrev with the punctuation changed:
> >>> http://cr.openjdk.java.net/~goetz/wr18/8201593-lenInAIOOB/07/
> >>> For the punctuation see also my mail reply to Roger's mail.
> >>
> >> Okay. That seems okay.
> >>
> >> Only further oddity I noticed is the use of %i rather than %d. Use of %i
> >> is very rare in hotspot. (I had to go and lookup what it means :) ).
> >>
> >> Thanks,
> >> David
> >>
> >>>> -----Original Message-----
> >>>> From: David Holmes [mailto:david.holmes at oracle.com]
> >>>> Sent: Montag, 7. Mai 2018 09:30
> >>>>
> >>>> Hi Goetz,
> >>>>
> >>>> On 4/05/2018 7:22 PM, Lindenmaier, Goetz wrote:
> >>>>> Hi,
> >>>>>
> >>>>> thanks to Aleksey and Boris this is now also tested on arm.
> >>>>> This final webrev contains some fixes needed in the arm files:
> >>>>> http://cr.openjdk.java.net/~goetz/wr18/8201593-lenInAIOOB/06-
> arm/
> >>>>>
> >>>>> David, can I consider this as finally reviewed?
> >>>>
> >>>> This follows on top of my +1 comments to Roger about the message
> >>>> consistency and punctuation etc.
> >>>>
> >>>> Aside: Took me a while to realize the
> >>>> throw_index_out_of_bounds_exception field was not a throw/don't-
> >> throw
> >>>> flag, but a throw AIOOBE or IOOBE flag!
> >>> Yes, this is not very intuitive ...
> >>>
> >>>> Again note I can't comment on the detailed CPU specific code.
> >>> The CPU code was reviewed by Martin Doerr, Stuart Monteith, Aleksey
> >> Shipilev and Boris Ulasevich.
> >>>
> >>>> One further nit:
> >>>> src/hotspot/cpu/aarch64/templateTable_aarch64.cpp
> >>>> I don't like the
> >>>> // ??? convention
> >>>> comments. It suggests the code is not understood. I don't expect you
> to
> >>>> fix existing ones but adding new ones doesn't seem good.
> >>> I don't like that either, nor the design of using a convention here.
> >>> The reviewers had trouble with that, too.
> >>> But as the two values are handled similarly, I would to like to
> >>> document it similarly, following the existing format. Stuart
> >>> was fine with that, anyways.
> >>>
> >>> An improvement of the design how these values are handled
> >>> would require changes on all platforms (it's similarly bad everywhere)
> >>> and I don't like to do that in this scope.
> >>>
> >>> Best regards,
> >>> Goetz.
> >>>
> >>>
> >>>>
> >>>>
> >>>> Thanks,
> >>>> David
> >>>>
> >>>>> Best regards,
> >>>>> Goetz
> >>>>>
> >>>>>
> >>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: aarch64-port-dev [mailto:aarch64-port-dev-
> >>>>>> bounces at openjdk.java.net] On Behalf Of Lindenmaier, Goetz
> >>>>>> Sent: Mittwoch, 2. Mai 2018 17:57
> >>>>>> To: Stuart Monteith <stuart.monteith at linaro.org>
> >>>>>> Cc: hotspot-compiler-dev at openjdk.java.net; hotspot-runtime-
> >>>>>> dev at openjdk.java.net; aarch64-port-dev at openjdk.java.net;
> aarch32-
> >>>> port-
> >>>>>> dev at openjdk.java.net
> >>>>>> Subject: [CAUTION] Re: [aarch64-port-dev ] RFR(M): 8201593: Print
> >> array
> >>>>>> length in ArrayIndexOutOfBoundsException.
> >>>>>>
> >>>>>> Hi,
> >>>>>>
> >>>>>> I needed to move the edit from c1_LIRGenerator_<cpu>.cpp to
> >>>>>> the shared file after "8201543: Modularize C1 GC barriers"
> >>>>>> New webrev:
> >>>>>> http://cr.openjdk.java.net/~goetz/wr18/8201593-lenInAIOOB/06/
> >>>>>>
> >>>>>> @Stuart
> >>>>>> Thanks for testing!
> >>>>>>> so as to accommodate the array pointer you are pushing onto the
> >> stack?
> >>>>>> Yes, what you are pointing out seems to be wrong, I changed it to '2'.
> >>>>>>
> >>>>>> Best regards,
> >>>>>> Goetz.
> >>>>>>
> >>>>>>
> >>>>>>> -----Original Message-----
> >>>>>>> From: Stuart Monteith [mailto:stuart.monteith at linaro.org]
> >>>>>>> Sent: Freitag, 27. April 2018 16:37
> >>>>>>> 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.
> >>>>>>>
> >>>>>>> Hi,
> >>>>>>> JTregs hasn't flagged any issues, so it should be ok.
> >>>>>>>
> >>>>>>> Regarding the 32-bit arm code, in "void
> >>>>>>> RangeCheckStub::emit_code(LIR_Assembler* ce)" should:
> >>>>>>> ce->verify_reserved_argument_area_size(1);
> >>>>>>> be
> >>>>>>> ce->verify_reserved_argument_area_size(2);
> >>>>>>>
> >>>>>>> so as to accommodate the array pointer you are pushing onto the
> >> stack?
> >>>>>>>
> >>>>>>> I've not tested 32-bit arm.
> >>>>>>>
> >>>>>>>
> >>>>>>> BR,
> >>>>>>> Stuart
> >>>>>>>
> >>>>>>> On 26 April 2018 at 15:31, Stuart Monteith
> >> <stuart.monteith at linaro.org>
> >>>>>>> wrote:
> >>>>>>>> Thanks, I'm happy with that.
> >>>>>>>>
> >>>>>>>> The registers have a clean path to call_RT - r22 and r23 aren't used
> >>>>>>>> inbetween. They are an arbitrary choice - c_rarg0 and c_rarg1
> were
> >>>>>>>> always going to cause problems. If _array->as_pointer_register()
> >>>>>>>> and/or _index->as_register() or _index->as_jint() were the
> registers
> >>>>>>>> we were using as parameters there would be trouble. However,
> >> with
> >>>>>>>> pd_last_allocatable_cpu_reg = 16, that shouldn't happen with
> >> r22/23,
> >>>>>>>> or indeed anything else in the range r17 to r28.
> >>>>>>>>
> >>>>>>>> I'm going to run all of JTRegs and seem what that produces now.
> >>>>>>>>
> >>>>>>>> BR,
> >>>>>>>> Stuart
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> On 26 April 2018 at 15:14, Lindenmaier, Goetz
> >>>>>>> <goetz.lindenmaier at sap.com> wrote:
> >>>>>>>>> Hi Stuart,
> >>>>>>>>>
> >>>>>>>>> thanks for fixing this! Webrev with your changes:
> >>>>>>>>> http://cr.openjdk.java.net/~goetz/wr18/8201593-
> lenInAIOOB/05/
> >>>>>>>>>
> >>>>>>>>>> There is the possibility of overwriting live values though, aren't
> >>>>>>>>>> there? The registers are saved by call_RT. Should I be
> concerned
> >>>> about
> >>>>>>>>>> deopt and debugging going wrong? Furthermore, won't there
> be
> >>>>>> issues
> >>>>>>> in
> >>>>>>>>>> exception handlers?
> >>>>>>>>> As I understand, this just has to survive the far_call.
> >>>>>>>>> The call_RT in c1_Runtime then moves it into the
> >>>>>>>>> proper argument registers. This is just the handling of an
> >>>>>>>>> exception, and in these few instructions no java code is
> >>>>>>>>> executed, no safepoint is passed, so this should be fine.
> >>>>>>>>>
> >>>>>>>>> incremental diff:
> >>>>>>>>> iff -r 874f2b999ff6
> >>>> 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
> >>>> 26
> >>>>>>> 15:55:18 2018 +0200
> >>>>>>>>> @@ -75,16 +75,16 @@
> >>>>>>>>> }
> >>>>>>>>>
> >>>>>>>>> if (_index->is_cpu_register()) {
> >>>>>>>>> - __ mov(rscratch1, _index->as_register());
> >>>>>>>>> + __ mov(r22, _index->as_register());
> >>>>>>>>> } else {
> >>>>>>>>> - __ mov(rscratch1, _index->as_jint());
> >>>>>>>>> + __ mov(r22, _index->as_jint());
> >>>>>>>>> }
> >>>>>>>>> Runtime1::StubID stub_id;
> >>>>>>>>> if (_throw_index_out_of_bounds_exception) {
> >>>>>>>>> stub_id = Runtime1::throw_index_exception_id;
> >>>>>>>>> } else {
> >>>>>>>>> assert(_array != NULL, "sanity");
> >>>>>>>>> - __ mov(rscratch2, _array->as_pointer_register());
> >>>>>>>>> + __ mov(r23, _array->as_pointer_register());
> >>>>>>>>> stub_id = Runtime1::throw_range_check_failed_id;
> >>>>>>>>> }
> >>>>>>>>> __ far_call(RuntimeAddress(Runtime1::entry_for(stub_id)),
> >> NULL,
> >>>>>>> rscratch2);
> >>>>>>>>> diff -r 874f2b999ff6
> >>>> src/hotspot/cpu/aarch64/c1_Runtime1_aarch64.cpp
> >>>>>>>>> --- a/src/hotspot/cpu/aarch64/c1_Runtime1_aarch64.cpp Mon
> >> Apr
> >>>> 16
> >>>>>>> 15:17:20 2018 +0200
> >>>>>>>>> +++ b/src/hotspot/cpu/aarch64/c1_Runtime1_aarch64.cpp Thu
> >> Apr
> >>>> 26
> >>>>>>> 15:55:18 2018 +0200
> >>>>>>>>> @@ -327,7 +327,7 @@
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> // target: the entry point of the method that creates and posts
> >> the
> >>>>>>> exception oop
> >>>>>>>>> -// has_argument: true if the exception needs an argument
> >> (passed
> >>>> in
> >>>>>>> rscratch1)
> >>>>>>>>> +// has_argument: true if the exception needs arguments
> (passed
> >> in
> >>>>>> r22
> >>>>>>> and r23)
> >>>>>>>>>
> >>>>>>>>> OopMapSet*
> >>>> Runtime1::generate_exception_throw(StubAssembler*
> >>>>>>> sasm, address target, bool has_argument) {
> >>>>>>>>> // make a frame and preserve the caller's caller-save registers
> >>>>>>>>> @@ -336,7 +336,7 @@
> >>>>>>>>> if (!has_argument) {
> >>>>>>>>> call_offset = __ call_RT(noreg, noreg, target);
> >>>>>>>>> } else {
> >>>>>>>>> - call_offset = __ call_RT(noreg, noreg, target, rscratch1,
> >> rscratch2);
> >>>>>>>>> + call_offset = __ call_RT(noreg, noreg, target, r22, r23);
> >>>>>>>>> }
> >>>>>>>>> OopMapSet* oop_maps = new OopMapSet();
> >>>>>>>>> oop_maps->add_gc_map(call_offset, oop_map);
> >>>>>>>>>
> >>>>>>>>> Best regards,
> >>>>>>>>> Goetz.
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>> -----Original Message-----
> >>>>>>>>>> From: Stuart Monteith [mailto:stuart.monteith at linaro.org]
> >>>>>>>>>> Sent: Donnerstag, 26. April 2018 12:52
> >>>>>>>>>> To: Andrew Haley <aph at redhat.com>
> >>>>>>>>>> Cc: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>;
> 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,
> >>>>>>>>>> Using c_rarg1 and c_rarg2 instead of rscratch1 and
> overwriting
> >>>>>>>>>> rscratch2 causes a SIGSEGV.
> >>>>>>>>>> Using r22 and r23 instead, the test ran successfully.
> >>>>>>>>>>
> >>>>>>>>>> In c1_CodeStubs_aarch64.cpp
> >>>>>>>>>> :
> >>>>>>>>>> 77 if (_index->is_cpu_register()) {
> >>>>>>>>>> 78 __ mov(r22, _index->as_register());
> >>>>>>>>>> 79 } else {
> >>>>>>>>>> 80 __ mov(r22, _index->as_jint());
> >>>>>>>>>> 81 }
> >>>>>>>>>> 82 Runtime1::StubID stub_id;
> >>>>>>>>>> 83 if (_throw_index_out_of_bounds_exception) {
> >>>>>>>>>> 84 stub_id = Runtime1::throw_index_exception_id;
> >>>>>>>>>> 85 } else {
> >>>>>>>>>> 86 assert(_array != NULL, "sanity");
> >>>>>>>>>> 87 __ mov(r23, _array->as_pointer_register());
> >>>>>>>>>> 88 stub_id = Runtime1::throw_range_check_failed_id;
> >>>>>>>>>> 89 }
> >>>>>>>>>>
> >>>>>>>>>> in c1_Runtime_aarch64.cpp:
> >>>>>>>>>>
> >>>>>>>>>> 336 if (!has_argument) {
> >>>>>>>>>> 337 call_offset = __ call_RT(noreg, noreg, target);
> >>>>>>>>>> 338 } else {
> >>>>>>>>>> 339 call_offset = __ call_RT(noreg, noreg, target, r22, r23);
> >>>>>>>>>> 340 }
> >>>>>>>>>>
> >>>>>>>>>> There is the possibility of overwriting live values though, aren't
> >>>>>>>>>> there? The registers are saved by call_RT. Should I be
> concerned
> >>>> about
> >>>>>>>>>> deopt and debugging going wrong? Furthermore, won't there
> be
> >>>>>> issues
> >>>>>>> in
> >>>>>>>>>> exception handlers?
> >>>>>>>>>>
> >>>>>>>>>> BR,
> >>>>>>>>>> Stuart
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> On 25 April 2018 at 16:49, Stuart Monteith
> >>>>>> <stuart.monteith at linaro.org>
> >>>>>>>>>> wrote:
> >>>>>>>>>>> Indeed - and that is what I am seeing. Usually no parameters
> are
> >>>>>> being
> >>>>>>>>>>> called with this pattern, or rscratch1, with the temporary
> variable
> >>>>>>>>>>> being changed to use rscratch2 in such circumstances.
> >>>>>>>>>>> I'll try c_rarg1 and c_rarg2 - they should pass straight
> through,if I
> >>>>>>>>>>> interpret the code correcting.
> >>>>>>>>>>>
> >>>>>>>>>>> On 25 April 2018 at 16:26, Andrew Haley <aph at redhat.com>
> >> wrote:
> >>>>>>>>>>>> On 04/25/2018 04:00 PM, Stuart Monteith wrote:
> >>>>>>>>>>>>> I'm not quite sure to solve this yet - we'll need to use the
> >> stack in
> >>>>>>>>>>>>> some safe way.
> >>>>>>>>>>>>
> >>>>>>>>>>>> It's not a great idea to pass arguments in rscratch1 or
> rscratch2.
> >>>>>> These
> >>>>>>>>>>>> registers are for use in macros and should be treated as
> >> volatile.
> >>>>>>> Given
> >>>>>>>>>>>> that you're throwing an exception, registers will be
> clobbered
> >>>>>>> anyway.
> >>>>>>>>>>>>
> >>>>>>>>>>>> --
> >>>>>>>>>>>> Andrew Haley
> >>>>>>>>>>>> Java Platform Lead Engineer
> >>>>>>>>>>>> Red Hat UK Ltd. <https://www.redhat.com>
> >>>>>>>>>>>> EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
More information about the aarch32-port-dev
mailing list