Request for review:8023477: Invalid CP index when reading ConstantPool

[Jiangli Zhou]

Thanks again, Serguei!

Jiangli

On 08/23/2013 03:27 PM, serguei.spitsyn at oracle.com wrote:
> Hi Jiangli,
>
> The fix looks good and safe.
>
> Thanks,
> Serguei
>
> On 8/23/13 2:49 PM, Jiangli Zhou wrote:
>> Hi,
>>
>> Please review the fix for 8023477:
>>
>>   http://cr.openjdk.java.net/~jiangli/8023477/webrev.00/
>>
>> Both tests reported by the bug failed due to the same problem. They 
>> both are passing now.
>>
>> The original memory reduction change for 8021948 turned out to be a 
>> little trickier than I expected.
>>
>> Thanks,
>> Jiangli
>>
>>
>> On 08/23/2013 11:00 AM, Jiangli Zhou wrote:
>>> I found the issue in the SA code. For class without generic 
>>> signature, the InstanceKlass::_generic_signature_index is 0. Need to 
>>> check for this case in the SA code. I'm working on the fix and doing 
>>> testing.
>>>
>>> Thanks,
>>> Jiangli
>>>
>>> On 08/23/2013 08:21 AM, Jiangli Zhou wrote:
>>>> Hi Staffan,
>>>>
>>>> Thanks for the info. Will look into that one.
>>>>
>>>> Jiangli
>>>>
>>>> On 08/23/2013 05:27 AM, Staffan Larsen wrote:
>>>>> Unfortunately there are two more tests that started failing after 
>>>>> the initial change. See: JDK-8023477.
>>>>>
>>>>> /Staffan
>>>>>
>>>>> On 23 aug 2013, at 01:16, Jiangli Zhou <jiangli.zhou at oracle.com> 
>>>>> wrote:
>>>>>
>>>>>> Hi Coleen,
>>>>>>
>>>>>> Yes, that's the case. Thanks for the review. I'll push this to 
>>>>>> hotspot-rt.
>>>>>>
>>>>>> Thanks,
>>>>>> Jiangli
>>>>>>
>>>>>> On 08/22/2013 03:34 PM, Coleen Phillimore wrote:
>>>>>>> Hi,
>>>>>>> Is it the case that the old index isn't in the index map because 
>>>>>>> it didn't change?  If so, this looks correct.
>>>>>>> Thanks,
>>>>>>> Coleen
>>>>>>>
>>>>>>>
>>>>>>> On 08/22/2013 06:15 PM, Jiangli Zhou wrote:
>>>>>>>> On 08/22/2013 03:10 PM, serguei.spitsyn at oracle.com wrote:
>>>>>>>>> Hi Jiangli,
>>>>>>>>>
>>>>>>>>> The fix is good and safe.
>>>>>>>>> I'm happy you fixed another case as well!
>>>>>>>>> Let's consider current bug as a clean-up issue so that we do 
>>>>>>>>> not need to file a separate bug. :)
>>>>>>>> Ok.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Jiangli
>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Serguei
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 8/22/13 2:50 PM, Jiangli Zhou wrote:
>>>>>>>>>> Hi Serguei,
>>>>>>>>>>
>>>>>>>>>> I've also made change to the case that you discovered. Please 
>>>>>>>>>> let me know if you think a separate bug should be filed to 
>>>>>>>>>> track it instead.
>>>>>>>>>>
>>>>>>>>>> http://cr.openjdk.java.net/~jiangli/8023547/webrev.01/
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> Jiangli
>>>>>>>>>>
>>>>>>>>>> On 08/22/2013 02:24 PM, Jiangli Zhou wrote:
>>>>>>>>>>> Hi Serguei,
>>>>>>>>>>>
>>>>>>>>>>> Thank you very much for the review and confirmation with the 
>>>>>>>>>>> test.
>>>>>>>>>>>
>>>>>>>>>>> Jiangli
>>>>>>>>>>>
>>>>>>>>>>> On 08/22/2013 02:18 PM, serguei.spitsyn at oracle.com wrote:
>>>>>>>>>>>> Hi Jiangli,
>>>>>>>>>>>>
>>>>>>>>>>>> Thank you for the quick fix which looks fine to me.
>>>>>>>>>>>> I confirm the test is passed with it.
>>>>>>>>>>>>
>>>>>>>>>>>> Staffan, thank you for the regression isolation.
>>>>>>>>>>>> I've noticed the following fragment in this file which 
>>>>>>>>>>>> seems has a similar issue:
>>>>>>>>>>>>
>>>>>>>>>>>>   // We also need to rewrite the parameter name indexes, if 
>>>>>>>>>>>> there is
>>>>>>>>>>>>   // method parameter data present
>>>>>>>>>>>>   if(method->has_method_parameters()) {
>>>>>>>>>>>>     const int len = method->method_parameters_length();
>>>>>>>>>>>>     MethodParametersElement* elem = 
>>>>>>>>>>>> method->method_parameters_start();
>>>>>>>>>>>>
>>>>>>>>>>>>     for (int i = 0; i < len; i++) {
>>>>>>>>>>>>       const u2 cp_index = elem[i].name_cp_index;
>>>>>>>>>>>>       elem[i].name_cp_index = find_new_index(cp_index);
>>>>>>>>>>>>     }
>>>>>>>>>>>>   }
>>>>>>>>>>>> } // end rewrite_cp_refs_in_method()
>>>>>>>>>>>>
>>>>>>>>>>>> The result of the find_new_index() above is not checked for 0.
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks,
>>>>>>>>>>>> Serguei
>>>>>>>>>>>>
>>>>>>>>>>>> On 8/22/13 12:38 PM, Jiangli Zhou wrote:
>>>>>>>>>>>>> Hi Staffan, Serguei and others,
>>>>>>>>>>>>>
>>>>>>>>>>>>> Here is the webrev for the 8023547 fix:
>>>>>>>>>>>>>
>>>>>>>>>>>>> http://cr.openjdk.java.net/~jiangli/8023547/webrev.00/
>>>>>>>>>>>>>
>>>>>>>>>>>>> Thanks!
>>>>>>>>>>>>>
>>>>>>>>>>>>> Jiangli
>>>>
>>>
>>
>