[10] RFR 8186046 Minimal ConstantDynamic support

[David Holmes]

On 4/11/2017 7:28 AM, Paul Sandoz wrote:
>> On 3 Nov 2017, at 11:14, Karen Kinnear <karen.kinnear at oracle.com> wrote:
>> 6. SD::find_java_mirror_for_type
>> You have resolve_or_null/fail doing CHECK_(empty) which should
>> check for a NULL constant_type_klass. This is followed by an explicit if
>> (constant_type_klass == NULL) — is that needed?
>>
> 
> Can SD:resolve_or_null return a null value when HAS_PENDING_EXCEPTION=false?

I don't believe it actually can - the only reason you would get NULL is 
if something went wrong, for which an exception must be pending. 
However, even the internal implementation underlying this seems unclear 
on that point e.g in SystemDictionary::resolve_instance_class_or_null we 
have:

   if (HAS_PENDING_EXCEPTION || k == NULL) {
     return NULL;
   }

David
-----

> I see other usages that suggest this to be the case. Where as for resolve_or_fail:
> 
> Klass* SystemDictionary::resolve_or_fail(Symbol* class_name, Handle class_loader, Handle protection_domain, bool throw_error, TRAPS) {
>    Klass* klass = resolve_or_null(class_name, class_loader, protection_domain, THREAD);
>    if (HAS_PENDING_EXCEPTION || klass == NULL) {
>      // can return a null klass
>      klass = handle_resolution_exception(class_name, throw_error, klass, THREAD);
>    }
>    return klass;
> }
> 
> 
> It suggests we can change the code from:
> 
> if (failure_mode == SignatureStream::ReturnNull) {
>    constant_type_klass = resolve_or_null(type, class_loader, protection_domain,
>                                          CHECK_(empty));
> } else {
>    bool throw_error = (failure_mode == SignatureStream::NCDFError);
>    constant_type_klass = resolve_or_fail(type, class_loader, protection_domain,
>                                          throw_error, CHECK_(empty));
> }
> if (constant_type_klass == NULL) {
>    return Handle();  // report failure this way
> }
> 
> to
> 
> if (failure_mode == SignatureStream::ReturnNull) {
>    constant_type_klass = resolve_or_null(type, class_loader, protection_domain,
>                                          CHECK_(empty));
>    if (constant_type_klass == NULL) {
>      return Handle();  // report failure this way
>    }
> } else {
>    bool throw_error = (failure_mode == SignatureStream::NCDFError);
>    constant_type_klass = resolve_or_fail(type, class_loader, protection_domain,
>                                          throw_error, CHECK_(empty));
> }
> 
> ?
> 
> 
>> 7. reflection.cpp
>> get_mirror_from_signature
>> Looks like potential for side effects. Odd to set mirror_oop inside if (log_is_enabled)
>> Is the intent to assert that k->java_mirror() == mirror_oop?
>> Or is the issue that we have a make drop here and the potential for a safe point?
>> If so, make a handle and extract it on return?
>>
>> — Or better yet - don’t make any changes to reflection.cpp - not necessary
>>
> 
> Lois, John?
> 
> My recollection was John was attempting to tunnel things through a single method find_java_mirror_for_type, but i agree the setting of mirror_oop is odd.
> 
> 
> 
>> 8. BootstrapMethodInvoker
>> Could you possibly add a comment that the default today is vmIsPushing and IsPullModeBSM is false?
>> Or find a slightly different naming to make that clearer?
> 
> boolean pullMode = isPullModeBSM(bootstrapMethod);  // default value is false
> boolean vmIsPushing = !staticArgumentsPulled(info); // default value is true
> 
> ?
> 
> 
>>
>> 9. test/jdk/java/lang/invoke/common/test/java/lang/invoke/lib/InstructionHelper.java
>> How would I write an ldc CONSTANT_Dynamic which referred to a bootstrap method that
>> was not ACC_STATIC?
> 
> https://docs.oracle.com/javase/specs/jvms/se9/html/jvms-4.html#jvms-4.7.23
> 
> See the note under bootstrap_method_ref. The kind of method handle is constrained because of the arguments that are pushed on the stack before invocation. I believe it’s possible to have a constructor, but the declaring class would need to be a subtype of CallSite in the case of indy, and a subtype of the constant value type in the case of condy.
> 
> 
>> Or was not ACC_PUBLIC?
> 
> That’s dependent on the accessibility between the lookup class the the BSM declaring class.
> 
> 
>> Or was
>> Or did I read the invoke dynamic method incorrectly?
>>
> 
> By default, and for convenience, the InstructionHelper assumes the BSM is declared by the lookup class. I recently modified that to support the BSM being declared on another class, to test the minimal set of BSMs for condy (separate issue [1]). Note it’s always possible to use the bytecode API more directly.
> 
> So we can easily add more -ve tests for non-accessible or non-static BSMs.
> 
> Paul.
> 
> [1] http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8187742-condy-bootstraps/webrev/
>