RFR(M): 8201593: Print array length in ArrayIndexOutOfBoundsException.
David Holmes
david.holmes at oracle.com
Mon May 7 21:03:05 UTC 2018
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 hotspot-compiler-dev
mailing list