RFR(M): 8201593: Print array length in ArrayIndexOutOfBoundsException.
Lindenmaier, Goetz
goetz.lindenmaier at sap.com
Mon May 7 08:16:23 UTC 2018
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.
> -----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