Request for reviews (L): 6939203: JSR 292 needs method handle constants

[John Rose]

On Jun 7, 2010, at 11:30 AM, Tom Rodriguez wrote:

> On Jun 4, 2010, at 6:11 PM, John Rose wrote:
> 
>> On Jun 3, 2010, at 11:57 AM, Tom Rodriguez wrote:
>> 
>>> Can you finish out the SA support or at least file a bug for it to be done?  The obvious list is printing support in ConstantPool.java and HTMLGenerator.java plus the writing support in ClassWriter.java.  All these paths will throw exceptions if they see unexpected constant pool types.
>> 
>> Yes, I'll do this over the weekend, plus add the missing CI support.

It's done.  (Long weekend, that.)  This works in -Xcomp mode with both compilers.  Thanks for your help with the C1 patching logic.

The SA code is more complete, although it's not clear to what "really complete" looks like.  It looks like SA bytecode walkers don't deal with the results of the JVM's Rewriter.  How can I test the ClassWriter?

Updated changes are here:
  http://cr.openjdk.java.net/~jrose/6939203/hs-webrev.04

Here are the diffs relative to the last reviewed change set (03):
  http://cr.openjdk.java.net/~jrose/6939203/hs-webrev.03/diff-03-to-04.patch

Summary:
- SA support roughed out (not tested); some missing bits added in passing
- C1 GraphBuilder, C1 Runtime, C2 parser for ldc are generalized to support ldc/MH
- patching ldc of non-perm oops is supported in C1 Runtime
- CI constant materialization logic supports CP cache and unloaded instance objects
- CI object factory and ciKlass.java_mirror provide unloaded class mirror objects
- cleaned out a few unused ciEnv methods
- ldc parsing code in ciBytecodeStream parallels code in BytecodeStream more closely
- JVM verifier now checks class file version (51) before allowing MH & indy constructs
- small regularizations to parser classes in bytecode.hpp
- CPCACHE_INDEX_TAG is now a zero-valued constant in product build
- changed CP encoding of MH ref_kind/ref_index to use HotSpot standard 16/16 bit packing
- constantTag is given queries for BasicType and internal name.

-- John


>> classFileParser.cpp:
>> 
>> why are these lines in the code?
>> 
>> +    //cp->method_handle_at_put(index, patch());
>> 
>> +      //cp->method_type_at_put(index, patch());
>> 
>> +      //cp->unresolved_method_type_at_put(index, name());
> 
> They match the NYI bits the follow.  Or, I could just delete that code and feed all the unimplemented cases through the default.  Would that be better?

I think leave it completely unimplemented until it's fully implemented.

> 
>> Aren't the new constant pool tags restricted to a minimum class file version?  I don't see a check for that.
> 
> Last year I was permissive about classfile version for practical reasons which no longer exist.  I'll add the version checks, and also to the verifier logic.

Ok.

> 
> constantPoolOop.cpp:
>> 
>> I don't think JVM_CONSTANT_UnresolvedClassInError should be handled in resolve_constant_at_impl.  I thought any attempt to access an entry with that tag should rethrow the same exception kind that caused the original failure?
> 
> Under that case, the call to klass_at_impl will just re-throw the same exception, which is the correct meaning of a resolve.  Elsewhere, this is how it is handled, since constantTag.is_unresolved_klass reports true in such cases, causing any re-resolution attempts to throw the exception.

Right.  I was having trouble figuring out where the rethrow logic is and that makes sense.

tom

> 
>> Otherwise this seems ok.
> 
> Thanks for taking a good look at it.  I'll send out another update when I finish the pending changes.
> 
> -- John
> 
>> tom
>> 
>> On Jun 2, 2010, at 1:29 PM, John Rose wrote:
>> 
>>> On Apr 22, 2010, at 12:22 AM, Christian Thalinger wrote:
>>> 
>>>> On Thu, 2010-04-22 at 00:08 -0700, John Rose wrote:
>>>>> Here's the final webrev (if you agree that I fixed the issues you raised):
>>>>> http://cr.openjdk.java.net/~jrose/6939203/hs-webrev.01
>>>> 
>>>> Yes, that looks good.  -- Christian
>>> 
>>> The bug tail from the cpindex changes is tamed, so now it's time to push the "ldc MH" functionality.
>>> 
>>> The round of bug fixes on cpindex led me to look more closely at BytecodeTracer and MethodComparator.  This led to some more changes to make to the ldc MH code.
>>> 
>>> Here they are, for re-review:
>>> http://cr.openjdk.java.net/~jrose/6939203/hs-webrev.03
>>> 
>>> Here are the diffs relative to the last reviewed change set (02):
>>> http://cr.openjdk.java.net/~jrose/6939203/hs-webrev.03/diff-02-to-03.patch
>>> 
>>> Summary:
>>> - invoke, field ref, and load constant bytecode accessors are regularized; constructors are all the same signature.
>>> - CP cache indexes are more clearly distinguished from plain pool indexes.
>>> - BytecodeTracer and MethodComparator now work properly on fast_aldc instructions.
>>> - BytecodeTracer prints object constants a little more usefully.
>>> 
>>> Thanks in advance.
>>> 
>>> -- John
>> 
>