RFR(M): 8201593: Print array length in ArrayIndexOutOfBoundsException.
David Holmes
david.holmes at oracle.com
Mon May 7 09:00:07 UTC 2018
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