Request for reviews (M): 6939196: method handle signatures off the boot class path get linkage errors

[John Rose]

On Apr 6, 2010, at 6:41 AM, Christian Thalinger wrote:

> On Tue, 2010-03-30 at 04:06 -0700, John Rose wrote:
>> 6939196: method handle signatures off the boot class path get linkage errors
>> 
>> http://cr.openjdk.java.net/~jrose/6939196/hs-webrev.00/
>> http://cr.openjdk.java.net/~jrose/6939196/jdk-webrev.00/
>> 
>> The bugtraq description is copied below.
>> 
>> - Adjust MethodType lookup logic to search off the BCP, but not to cache those results.
>> - Pass accessing class where it needs to be seen.
>> - A SignatureStream idiom "as_klass" for class lookup has been refactored.
> 
> src/share/vm/classfile/systemDictionary.cpp:
> 
> 2382   bool is_on_bcp;  // keep this true as long as we can materialize from the boot classloader
> 
> Why do need an extra flag?  return_bcp_flag seems to always be set to
> the same value.

Just a style thing mainly:  Because return_bcp_flag is an out parameter, it is write-only.

Also, is_on_bcp is a local variable that the C++ optimizer can control fully.  (I don't like putting loop control in unknown reference variables.)

I'll move the write of return_bcp_flag out of the loop, to make its role more clear.

> src/share/vm/prims/methodHandles.cpp:
> 
> 441       (//defc() == SystemDictionary::InvokeDynamic_klass() || //FIXME
> 
> What is the problem here?

Oops, thanks for noticing that.  There is no problem at that point, so I'll remove the "//".  The note is to remind me of Lookup.findStatic(InvokeDynamic.class, "x", T). It's a corner case that needs some attention.  (There's a similar issue with Class.getDeclaredMethod on those magic classes.)

> Otherwise looks good.

Thanks!

>> Please review the JDK changes along with the JVM changes.  They are very simple:
>> - Remove workaround from MethodHandleImpl lookup code
>> - Add JUnit regression test to MethodHandlesTest
> 
> These changes look good.

Good; thanks again.

Working on invokeGeneric now...

-- John