> On 3 Nov 2017, at 11:14, Karen Kinnear <karen.kinn...@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/
