Review request: 8000459: assert(java_lang_String::is_instance(entry)) failure with various mlvm tests

[Jiangli Zhou]

Thanks, Serguei!

Jiangli

On 10/11/2012 10:46 AM, serguei.spitsyn at oracle.com wrote:
> Looks good.
>
> Thanks,
> Serguei
>
> On 10/11/12 8:59 AM, Jiangli Zhou wrote:
>> Hi Coleen,
>>
>> Here is the webrev that removes the assert from the latest hsx25 
>> repository with PermGen removal. The assert doesn't add any real 
>> value. I also fixed a small typo.
>>
>>   http://cr.openjdk.java.net/~jiangli/8000459/webrev.01/
>>
>> The previous webrev was based on hsx24, which is pre-PerGen removal. 
>> I should have included that detail in the first review request.
>>
>> Thanks,
>>
>> Jiangli
>>
>> On 10/10/2012 10:50 AM, Coleen Phillimore wrote:
>>>
>>> This change looks good.  Thank you for fixing it.
>>> Coleen
>>>
>>> On 10/10/2012 12:44 PM, serguei.spitsyn at oracle.com wrote:
>>>> Hi Jiangli,
>>>>
>>>>
>>>> On 10/10/12 9:37 AM, Jiangli Zhou wrote:
>>>>> Hi Serguei,
>>>>>
>>>>> Thanks for the review! Please see comments below.
>>>>>
>>>>> On 10/09/2012 06:05 PM, serguei.spitsyn at oracle.com wrote:
>>>>>> Hi Jiangli,
>>>>>>
>>>>>> I'm not very familiar with the pseudo strings, and so, just have 
>>>>>> a question:
>>>>>>
>>>>>> *src/share/vm/prims/jvmtiTagMap.cpp*
>>>>>> 2927             entry = pool->resolved_string_at(i);
>>>>>> 2928 assert(java_lang_String::is_instance(entry) ||
>>>>>> 2929                    pool->is_pseudo_string_at(i), "must be 
>>>>>> string");
>>>>>> I have a little of doubt the above is correct.
>>>>>>
>>>>>> What value the "entry" will have in a case of pseudo string cp 
>>>>>> entry?
>>>>>> Just want to understand what constant pool reference will be 
>>>>>> reported.
>>>>>> Do you see all related failing tests passed?
>>>>>>
>>>>>> Would something like the following be more accurate ?
>>>>>>    entry = pool->is_pseudo_string_at(i) ?
>>>>>>                pool->pseudo_string_at(i, cp_to_object_index(i)) :
>>>>>>                pool->resolved_string_at(i);
>>>>>> assert(java_lang_String::is_instance(entry));
>>>>>>
>>>>>> In this case, a reference from CP to string is reported 
>>>>>> regardless it is pseudo string or not.
>>>>>
>>>>> Those are very good questions. Here is an example of the specific 
>>>>> case with a generated JSR292 java.lang.invoke.LambdaForm$MH class. 
>>>>> The follow are two constant pool entries of the class. Entry 16 is 
>>>>> a "sun/misc/Unsafe" String, which could be patched using a 
>>>>> sun.misc.Unsafe klass oop.
>>>>>
>>>>>         ...
>>>>>         (0016) tag=Utf8 "sun/misc/Unsafe"
>>>>>         (0017) tag=Class "sun/misc/Unsafe"
>>>>>         ...
>>>>>
>>>>> I discussed with John Rose about different ways to fix the 
>>>>> assertion issue. The code you have above uses pseudo_string_at() 
>>>>> to access the 'entry', which is similar to one of the version we 
>>>>> discussed. The current change was chosen as it's the simplest 
>>>>> solution.
>>>>
>>>> I'm Ok with the simplest solution.
>>>>
>>>> Thank you for the details!
>>>> -Serguei
>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Jiangli
>>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> Serguei
>>>>>>
>>>>>> On 10/9/12 4:26 PM, Jiangli Zhou wrote:
>>>>>>> Please review following fix for 8000459:
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~jiangli/8000459/webrev/
>>>>>>>
>>>>>>> With JSR292, a constant pool String entry could be patched to a 
>>>>>>> non-string oop. The assert in 
>>>>>>> VM_HeapWalkOperation::iterate_over_class() needs to be adjusted 
>>>>>>> to reflect that.
>>>>>>>
>>>>>>> Thanks,
>>>>>>>
>>>>>>> Jiangli
>>>>>>
>>>>>
>>>>
>>
>