Request for reviews (S): 7092712: JSR 292: unloaded invokedynamic call sites can lead to a crash with signature types not on BCP

[Christian Thalinger]

On Oct 11, 2011, at 8:29 PM, John Rose wrote:

> On Oct 11, 2011, at 8:13 AM, Christian Thalinger wrote:
> 
>>>> I think you need to match the accessor argument in the matching loop of get_unloaded_method.
>>>> 
>>>> Option 1.  In the matching loop, after checking the symbolic signature, also check the resolved signature.  You can do this by making a ciSignature (which resolves types relative to the accessor).  Add an equals method to ciSignature.  Consider adding equals(ciSymbol* sig, ciKlass* accessor), so that the second ciSignature does not need to be created.
>>>> 
>>>> Option 2.  Check the accessor argument against the signature._accessing_klass in the matching loop.  To preserve existing behavior (for non-MH calls) either use null accessors for non-MH lookups and treat them specially (as wildcards) or else keep separate lists (of MH and non-MH unloaded methods).
>>>> 
>>>> I think Option 1 is more precise, but Option 2 might be a more conservative change.
>>> 
>>> Option 1 sounds like a clean way.  But to compare the resolved signature we have to resolve the types anyway so maybe it's better to create the second ciSignature lazily when we need it.  I updated the webrev.
>> 
>> John, can you re-review the changes please?
> 
> In the ciSignature::equals method, why bother to resolve types when you first prove equality of accessing_klass?
> 
> The code in the webrev is (I think) equivalent to this->ac_kl == that->ac_kl && this->symbol == that->symbol.
> 
> Either drop the equality check on accessing_klass, or don't bother to resolve the types.
> 
> (And if you don't resolve the types, you don't really need the temporary ciSignature, since it's just a <sym, ac_kl> ordered pair check.)
> 
> Or am I missing something?

Type resolving happens in the ciSignature constructor.  So when we are in equals everything is already resolved.

But as we talked on IM yesterday we can short-circuit the ciSignature creation in ciObjectFactory::get_unloaded_method
.  I added some debug output to see how often the short-circuiting triggers and it triggers much more often than creating the ciSignature.

I removed the accessing class check in ciSignature::equals (see the method comment) but added a short-cut bailout on a signature pointer-compare.

Additionally I removed two debug output statements in src/share/vm/prims/methodHandleWalk.cpp that we missed.

Webrev is updated:

http://cr.openjdk.java.net/~twisti/7092712/

-- Chris

> 
> -- John