[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-runtime-dev
Subject:    Re: [9] RFR: 8152207: Perform array bound checks while getting a length of bytecode instructions
From:       harold seigel <harold.seigel () oracle ! com>
Date:       2016-05-27 12:00:27
Message-ID: 1a55ca14-d778-a48a-5067-18f10332448c () oracle ! com
[Download RAW message or body]

Hi Artem,

The changes look good.  If you want to restore the "int length = ..." 
code that's okay, too.  I don't need to see another webrev.

Thanks, Harold

On 5/26/2016 5:58 PM, David Holmes wrote:
> On 27/05/2016 7:51 AM, Artem Smotrakov wrote:
>> Hi Harold,
>>
>> Thank you for review. Here is an updated webrev
>>
>> http://cr.openjdk.java.net/~asmotrak/8152207/jdk/webrev.01/
>>
>> If I am not missing something, it might return -1 if an instruction
>> index is invalid, so that it doesn't need to do anything else then.
>>
>> I noticed that the following code may fail with "declaration can not
>> follow a statement (E_DECLARATION_IN_CODE)" on some gcc versions (I've
>> seen this on Solaris):
>
> That would either be a very old compiler or else we're not explicitly 
> enabling the right C version and/or GNU extensions.
>
> Cheers,
> David
>
>> ...
>>        default: {
>>             if (instruction < 0 || instruction > JVM_OPC_MAX)
>>                 return -1;
>>
>>             /* A length of 0 indicates an error. */
>>             int length = opcode_length[instruction];
>>             return (length <= 0) ? -1 : length;
>>        }
>> ...
>>
>> So, I just removed the 'length' variable.
>>
>> Artem
>>
>> On 05/26/2016 10:36 AM, harold seigel wrote:
>>> Hi Artem,
>>>
>>> The hotspot changes look good.
>>>
>>> For the jdk webrev, can you move the new code at lines 1699 and 1700
>>> to just inside the 'default:' alternative?
>>>
>>> Thanks, Harold
>>>
>>>
>>> On 5/20/2016 4:05 PM, Artem Smotrakov wrote:
>>>> Hello,
>>>>
>>>> Could someone review this fix, please?
>>>>
>>>> http://cr.openjdk.java.net/~asmotrak/8152207/hotspot/webrev.01/
>>>> http://cr.openjdk.java.net/~asmotrak/8152207/jdk/webrev.00/
>>>>
>>>> Artem
>>>>
>>>> On 05/10/2016 01:49 PM, Artem Smotrakov wrote:
>>>>> Hi Christian,
>>>>>
>>>>> Thank you for review. Here is updated webrev
>>>>>
>>>>> http://cr.openjdk.java.net/~asmotrak/8152207/hotspot/webrev.01/
>>>>>
>>>>> Artem
>>>>>
>>>>> On 05/09/2016 01:24 PM, Christian Thalinger wrote:
>>>>>>
>>>>>>> On May 4, 2016, at 1:48 PM, Artem Smotrakov
>>>>>>> <artem.smotrakov@oracle.com <mailto:artem.smotrakov@oracle.com>>
>>>>>>> wrote:
>>>>>>>
>>>>>>> Hello,
>>>>>>>
>>>>>>> Please review two small patches for jdk and hotspot repos which
>>>>>>> add array bound checks to functions which return a length of
>>>>>>> bytecode instruction.
>>>>>>>
>>>>>>> Please see details in
>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8152207
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~asmotrak/8152207/jdk/webrev.00/
>>>>>>> <http://cr.openjdk.java.net/%7Easmotrak/8152207/jdk/webrev.00/>
>>>>>>> http://cr.openjdk.java.net/~asmotrak/8152207/hotspot/webrev.00/
>>>>>>
>>>>>> static bool        is_defined     (int  code)    { return 0 <= code
>>>>>> && code < number_of_codes && flags(code, false) != 0; }
>>>>>> + static int length_for (Code code) { return 0 <= code && code <
>>>>>> number_of_codes ? _lengths[code] & 0xF : -1; }
>>>>>> + static int wide_length_for(Code code) { return 0 <= code && code
>>>>>> < number_of_codes ? _lengths[code] >> 4 : -1; }
>>>>>> You should factor the bound check into a separate method.
>>>>>>
>>>>>>>
>>>>>>> Artem
>>>>>>
>>>>>
>>>>
>>>
>>

[prev in list] [next in list] [prev in thread] [next in thread]