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

[serguei.spitsyn at oracle.com]

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