[10] RFR 8186046 Minimal ConstantDynamic support

[Paul Sandoz]

> On 3 Nov 2017, at 11:14, Karen Kinnear <karen.kinnear at oracle.com> wrote:
> 
> Thank you so much for doing this jointly.
> 
> Couple of questions/comments:
> 1. thank you for making UseBootstrapCallInfo diagnostic
> 
> 2. org/objectweb/asmClassReader.java
> why hardcoded 17 instead of ClassWriter.CONDY?
> 

I chose to make the absolute minimal amount of changes to our internal ASM to reduce possible conflicts as i anticipate that a new version of ASM with condy support will eventually be rolled in.


> 3. java/lang/invoke/package-info.java 128-134
> Error handling could be clearer.
> My understanding is that if a LinkageError or subclass is thrown, this will be rethrown
> for all subsequent attempts. Other errors, e.g. VMError may retry resolution
> 
> Also: after line 165: rules do not enable the JVM to share call sites.
> Is it worth noting anywhere that the Constant_Dynamic resolution is shared?
> 
> 4. SD::find_java_mirror_for_type
> lines 2679+
>   ConDy should not be supporting CONSTANT_Class(“;” + FD)
>   IIRC that is temporary for MVT but not part of ConDy’s spec, nor part of what
>   should be checked in for 18.3


Yes, i deleted lines 2678 to 2687

TempNewSymbol type_buf;
if (type->utf8_length() > 1 && type->byte_at(0) == ';') {
  // Strip the quote, which may have come from CONSTANT_Class[";"+FD].
  // (This logic corresponds to the use of "FieldType::is_obj"
  // in resolve_or_null.  A field type can be unwrapped to a class
  // type and vice versa.)
  type = SymbolTable::new_symbol(type->as_C_string() + 1,
                                 type->utf8_length() - 1, CHECK_(empty));
  type_buf = type;  // will free later
}


> also line 2731 - remove comment about “Foo;”
> 

Removed.


> 5. constantTag.hpp
> ofBasicType: case T_OBJECT return constantTag(JVM_CONSTANT_String) ?
> why not JVM_CONSTANT_CLASS?
> 

We would have to ask John on that, i am guessing it does not matter much since round tripping is ok and it is the more benign of the mapping. I do find this sits awkwardly though, perhaps there is a more general cleanup that can follow in this regard?


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