[10] RFR 8186046 Minimal ConstantDynamic support

[Karen Kinnear]

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?

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
also line 2731 - remove comment about “Foo;”

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

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?

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

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?

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?
Or was not ACC_PUBLIC?
Or was 
Or did I read the invoke dynamic method incorrectly?

thanks,
Karen

> On Oct 26, 2017, at 1:03 PM, Paul Sandoz <paul.sandoz at oracle.com> wrote:
> 
> Hi,
> 
> Please review the following patch for minimal dynamic constant support:
> 
>  http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8186046-minimal-condy-support-hs/webrev/ <http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8186046-minimal-condy-support-hs/webrev/>
> 
>  https://bugs.openjdk.java.net/browse/JDK-8186046 <https://bugs.openjdk.java.net/browse/JDK-8186046>
>  https://bugs.openjdk.java.net/browse/JDK-8186209 <https://bugs.openjdk.java.net/browse/JDK-8186209>
> 
> This patch is based on the JDK 10 unified HotSpot repository. Testing so far looks good.
> 
> By minimal i mean just the support in the runtime for a dynamic constant pool entry to be referenced by a LDC instruction or a bootstrap method argument. Much of the work leverages the foundations built by invoke dynamic but is arguably simpler since resolution is less complex.
> 
> A small set of bootstrap methods will be proposed as a follow on issue for 10 (these are currently being refined in the amber repository).
> 
> Bootstrap method invocation has not changed (and the rules are the same for dynamic constants and indy). It is planned to enhance this in a further major release to support lazy resolution of bootstrap method arguments.
> 
> The CSR for the VM specification is here:
> 
>  https://bugs.openjdk.java.net/browse/JDK-8189199 <https://bugs.openjdk.java.net/browse/JDK-8189199>
> 
> the j.l.invoke package documentation was also updated but please consider the VM specification as the definitive "source of truth" (we may clean up this area further later on so it becomes more informative, and that may also apply to duplicative text on MethodHandles/VarHandles).
> 
> Any AoT-related work will be deferred to a future release.
> 
> —
> 
> This patch only supports x64 platforms. There is a small set of changes specific to x64 (specifically to support null and primitives constants, as prior to this patch null was used as a sentinel for resolution and certain primitives types would never have been encountered, such as say byte).
> 
> We will need to follow up with the SPARC platform and it is hoped/anticipated that OpenJDK members responsible for other platforms (namely ARM and PPC) will separately provide patches.
> 
> —
> 
> Many of tests rely on an experimental byte code API that supports the generation of byte code with dynamic constants.
> 
> One test uses class file bytes produced from a modified version of asmtools.  The modifications have now been pushed but a new version of asmtools need to be rolled into jtreg before the test can operate directly on asmtools information rather than embedding class file bytes directly in the test.
> 
> —
> 
> Paul.