Re: [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.kinn...@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/