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

[Tom Rodriguez]

On Jun 8, 2010, at 5:22 PM, John Rose wrote:

> 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.

What do you mean?


> How can I test the ClassWriter?

jcoreproc.sh will dump all the classfiles from a JVM, or you can use the dumpclass command in clhsdbproc.sh.  So dump something that has one of your new classes loaded and compare the result against the original.  It should be the same, apart from some missing sections like the annotations which  I think get dropped along the way.  You can use the jdec utility to dump a fairly raw dump of the class files to make sure they look exactly the same.

java -classpath /java/re/jct-tools/3.0.3/archive/fcs/binaries/asmtools.jar javasoft.sqe.jdec.Main foo.class

It's a printed form the class file format itself so it shows the true structure which should be exactly the same as the original.

You can test the bytecode disassembler too by printing a methodOop in hsdb.  Using class to get klassOop, using print on that to get the methods with their methodOops and then print the methodOop.

I think it all looks good.

tom

> 
> 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
>>> 
>> 
> 
>