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

[serguei.spitsyn at oracle.com]

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/serviceability-dev/attachments/20130221/b0da3b53/attachment-0001.html