RFR(M): 8201593: Print array length in ArrayIndexOutOfBoundsException.
Lindenmaier, Goetz
goetz.lindenmaier at sap.com
Fri May 4 09:22:23 UTC 2018
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?
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 hotspot-compiler-dev
mailing list