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

[John Rose]

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.

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

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

> verifier.cpp:
> 
> This has a check for EnableMethodHandles but the parser does not.  I should be consistent one way or another.

Done.

> A check at load time would obviate later checks I think.

Good point.  I'll simplify the downstream logic and leave a comment behind.

> bytecodes.hpp:
> 
> This comment:
> 
> // Abstraction for invoke_{virtual, static, interface, special}
> 
> should move along with Bytecode_invoke and something appropriate should be placed above Bytecode_member_ref.

Good.  Here's the comment:
 + // Common code for decoding invokes and field references.

> Any reason not to delete adjusted_invoke_code completely?

Not really.  Done and good riddance.

> Instead of having to ifdef uses of CPCACHE_INDEX_TAG why not simply define it as 0 in non-debug builds?  DEBUG_ONLY in expressions is fairly awful.

I don't want product code to use it at all, since it's just a device for assertion checking.  But since I know this is true, I suppose I could let it back into the product build as a "harmless" zero.  OK, done with this comment:

   #ifdef ASSERT
     enum { CPCACHE_INDEX_TAG = 0x10000 };  // helps keep CP cache indices distinct from CP indices
 + #else
 +   enum { CPCACHE_INDEX_TAG = 0 };        // in product mode, this zero value is a no-op
   #endif //ASSERT


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

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20100604/525d2962/attachment.html