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

[Staffan Larsen]

Looks good.

Thanks,
/Staffan

On 23 aug 2013, at 23:49, Jiangli Zhou <jiangli.zhou at oracle.com> 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
>>> 
>> 
>