Request for review: 8000797: NPG: is_pseudo_string_at() doesn't work

[Coleen Phillimore]

Thank you for filing the new bug on reconstitution.   I think it needs 
more than this patch#42 symbol because the oop has to be carried forth 
from the resolved_references array in the old constant pool, and 
repatched into the new constant pool once the new resolved_references 
array is created.   And I didn't follow what this this symbol hashtable 
was trying to do.   It would be good to start with a test for this.

Thanks,
Coleen

On 2/21/2013 2:16 PM, serguei.spitsyn at oracle.com wrote:
> On 2/20/13 11:59 AM, Coleen Phillimore wrote:
>> On 2/20/2013 2:51 PM, John Rose wrote:
>>> On Feb 20, 2013, at 10:20 AM, Coleen Phillimore 
>>> <coleen.phillimore at oracle.com <mailto:coleen.phillimore at oracle.com>> 
>>> wrote:
>>>
>>>> Summary: Add JVM_CONSTANT_PseudoString in place of 
>>>> JVM_CONSTANT_Object and use this tag to distinguish patched pseudo 
>>>> strings. The original string is retained if it was present.
>>>
>>> This is reasonable; it is a good cleanup.  If you can propose a name 
>>> better than "PseudoString" I'm all ears.
>>
>> If the string is really meaningless, maybe it can be deleted and we 
>> don't need this JVM_CONSTANT_PseudoString.  The only reason I kept 
>> "String" in the name is because I thought the string would have some 
>> meaning to be preserved.
>>
>>>
>>> Consider getting rid of set_has_pseudo_string.  That flag was 
>>> present (IIRC) only to tell the GC that there might be non-perm oops 
>>> in the constant pool.  Do we still need that?
>>
>> I'd be happy to.  I noticed it wasn't being used.   Neither is 
>> _has_invokedynamic for that matter.   _has_preresolution does do 
>> something.
>>>
>>>> I'm not sure how class file reconstitution for pseudo-strings is 
>>>> going to work, but I thought it was prudent to leave the Symbol* in 
>>>> the slot for the patched string.
>>>
>>> If you really wanted to reconstitute a class file for an anonymous 
>>> class, and if that class has oop patching (pseudo-strings), you 
>>> would need either to (a) reconstitute the patches array handed to 
>>> Unsafe.defineAnonymousClass, or (b) accept whatever odd strings were 
>>> there first, as an approximation.  The "odd strings" are totally 
>>> insignificant, and are typically something like 
>>> "CONSTANT_PLACEHOLDER_42" (see 
>>> InvokerBytecodeGenerator::constantPlaceholder).
>>>
>>
>> Maybe there isn't a way or API to reconstitute an anonymous class.   
>> I don't know if there is.  I'm not sure how to reconstitute a normal 
>> class in the first place.   Maybe Serguei can comment.   If this 
>> class cannot be reconsitituted, I'll change this to remove the string 
>> in the patched case and won't need JVM_CONSTANT_PseudoString (and the 
>> constant for Object can be removed too).
>
> Reconstitution of the ConstantPool is needed for both the JVMTI 
> GetConstantPool() and RetransformClasses().
> Finally, it goes to the ConstantPool::copy_cpool_bytes().
>
> This is the place that has to be fixed:
>
> ConstantPool.cpp:
> int ConstantPool::copy_cpool_bytes(int cpool_size,
>       . . .
>       case JVM_CONSTANT_String: {
>         *bytes = JVM_CONSTANT_String;
>         Symbol* sym = unresolved_string_at(idx);
>         idx1 = tbl->symbol_to_value(sym);
>         assert(idx1 != 0, "Have not found a hashtable entry");
>         Bytes::put_Java_u2((address) (bytes+1), idx1);
>         DBG(char *str = sym->as_utf8());
>         DBG(printf("JVM_CONSTANT_String: idx=#%03hd, %s", idx1, str));
>         break;
>       }
>
> I think, the John's suggestion for odd strings should work here.
> Something like:
>   if (sym == NULL) {
>     str = "CONSTANT_PLACEHOLDER_42";
>   }
>
> I will file a bug on this.
>
>
> Thanks,
> Serguei
>
>>
>> Thanks!
>> Coleen
>>
>>> — John
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/attachments/20130221/fcf3cc30/attachment.html