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 Sep 23, 2011, at 3:19 PM, Sebastian Sickelmann wrote:

> Am 22.09.2011 12:23, schrieb Christian Thalinger:
>> http://cr.openjdk.java.net/~twisti/7092712/
>> 
>> 7092712: JSR 292: unloaded invokedynamic call sites can lead to a crash with signature types not on BCP
>> Reviewed-by:
>> 
>> The problem is that ciEnv::get_fake_invokedynamic_method_impl calls
>> get_unloaded_method with java.lang.invoke.MethodHandle as the holder
>> for unresolved call sites.
>> 
>> Since the loader of j.l.i.MethodHandle is the boot class loader the
>> resolving of e.g. signature classes is done with the boot class loader
>> resulting in problems like:
>> 
>> (dbx) p this->print()
>> <ciInstanceKlass name=NEW2 loader=0xe5e88000 loaded=true initialized=true finalized=false subklass=false size=16 flags=public,super super=java/lang/Object ident=714 PERM address=0x8480c40>this->print() = (void)
>> (dbx) p that->print()
>> <ciInstanceKlass name=NEW2 loader=0x0 loaded=false ident=720  address=0x8481178>that->print() = (void)
>> 
>> Later in the game a ciInstanceKlass lookup for NEW2 returns a
>> ciInstanceKlass created during the signature resolving in
>> get_unloaded_method with the boot class loader as loader resulting in
>> the above situation.
>> 
>> The fix is to always pass an accessor to get_unloaded_method and
>> subsequently the ciMethod constructor.
>> 
>> src/share/vm/ci/ciEnv.cpp
>> src/share/vm/ci/ciEnv.hpp
>> src/share/vm/ci/ciMethod.cpp
>> src/share/vm/ci/ciMethod.hpp
>> src/share/vm/ci/ciObjectFactory.cpp
>> src/share/vm/ci/ciObjectFactory.hpp
>> 
> It fixed it for me on my Testcase with (jdk8).
> Thanks for that.
> 
> I can't review it formally (because i don't understand the fix in a complete manner) but
> should there be an automated test for this? Or is this not a common practice at hotspot-level?

It is but I couldn't write a test case in plain Java to reproduce this error (using ASM is not an option here).

> 
> Will there be a jdk7 backport?

Yes, I think this fix should be backported to 7.

-- Chris

> 
> -- Sebastian