RFR(M): 8201593: Print array length in ArrayIndexOutOfBoundsException.
David Holmes
david.holmes at oracle.com
Mon May 7 07:30:12 UTC 2018
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!
Again note I can't comment on the detailed CPU specific code.
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.
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