RFR(M): 8201593: Print array length in ArrayIndexOutOfBoundsException.
Lindenmaier, Goetz
goetz.lindenmaier at sap.com
Mon May 7 14:08:26 UTC 2018
Hi Roger,
thanks a lot!
Best regards,
Goetz.
> -----Original Message-----
> From: Roger Riggs [mailto:Roger.Riggs at Oracle.com]
> Sent: Montag, 7. Mai 2018 15:45
> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>
> 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: Re: RFR(M): 8201593: Print array length in
> ArrayIndexOutOfBoundsException.
>
> Hi Goetz,
>
> Looks good to me (as to the exception message format).
>
> Thanks, Roger
>
>
> On 5/7/2018 8:20 AM, 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>
> <mailto:goetz.lindenmaier at sap.com> ; Stuart Monteith
> <stuart.monteith at linaro.org>
> <mailto:stuart.monteith at linaro.org>
> Cc: hotspot-compiler-dev at openjdk.java.net
> <mailto:hotspot-compiler-dev at openjdk.java.net> ; aarch64-port-
> dev at openjdk.java.net <mailto:dev at openjdk.java.net> ;
> hotspot-runtime-dev at openjdk.java.net <mailto:hotspot-runtime-
> dev at openjdk.java.net> ; aarch32-
> port-dev at openjdk.java.net <mailto: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
> <mailto: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> <mailto:stuart.monteith at linaro.org>
> Cc: hotspot-compiler-
> dev at openjdk.java.net <mailto:hotspot-compiler-dev at openjdk.java.net> ;
> hotspot-runtime-
> dev at openjdk.java.net
> <mailto:dev at openjdk.java.net> ; aarch64-port-dev at openjdk.java.net
> <mailto:aarch64-port-dev at openjdk.java.net> ; aarch32-
>
> port-
>
> dev at openjdk.java.net
> <mailto: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> <mailto:goetz.lindenmaier at sap.com>
> Cc: hotspot-compiler-
> dev at openjdk.java.net <mailto:hotspot-compiler-dev at openjdk.java.net> ;
> aarch64-port-
> dev at openjdk.java.net
> <mailto:dev at openjdk.java.net> ; hotspot-runtime-dev at openjdk.java.net
> <mailto:hotspot-runtime-dev at openjdk.java.net> ;
>
> aarch32-
>
> port-dev at openjdk.java.net
> <mailto: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>
> <mailto: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>
> <mailto: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>
> <mailto:aph at redhat.com>
> Cc: Lindenmaier, Goetz
> <goetz.lindenmaier at sap.com> <mailto:goetz.lindenmaier at sap.com> ;
> hotspot-
>
> compiler-
>
> dev at openjdk.java.net
> <mailto:dev at openjdk.java.net> ; aarch64-port-dev at openjdk.java.net
> <mailto:aarch64-port-dev at openjdk.java.net> ;
>
> hotspot-
>
> runtime-dev at openjdk.java.net
> <mailto:runtime-dev at openjdk.java.net> ; aarch32-port-
>
> dev at openjdk.java.net
> <mailto: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>
> <mailto: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> <mailto: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> <https://www.redhat.com>
> EAC8 43EB D3EF DB98 CC77 2FAD A5CD
> 6035 332F A671
>
More information about the hotspot-compiler-dev
mailing list