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

[serguei.spitsyn at oracle.com]

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
>>>
>>
>