I’m seeing no issue with rcx being aliased in this code. Fred

On Oct 30, 2017, at 15:44, Paul Sandoz <paul.sandoz@oracle.com> wrote:

Hi,

Thanks for reviewing.

...

On 30 Oct 2017, at 11:05, Dmitry Samersoff <dmitry.samersoff@bell-sw.com> wrote:

Paul,

templateTable_x86.cpp:

564 const Register flags = rcx; 565 const Register rarg = NOT_LP64(rcx) LP64_ONLY(c_rarg1);

Should we use another register for rarg under NOT_LP64 ?

I think it should be ok, it i ain’t an expert here on the interpreter and the calling conventions, so please correct me.

Some more context:

+ const Register flags = rcx; + const Register rarg = NOT_LP64(rcx) LP64_ONLY(c_rarg1); + __ movl(rarg, (int)bytecode());

The current bytecode code is loaded into “rarg”

+ call_VM(obj, CAST_FROM_FN_PTR(address, InterpreterRuntime::resolve_ldc), rarg);

Then “rarg" is the argument to the call to InterpreterRuntime::resolve_ldc, after which it is no longer referred to.

+#ifndef _LP64 + // borrow rdi from locals + __ get_thread(rdi); + __ get_vm_result_2(flags, rdi); + __ restore_locals(); +#else + __ get_vm_result_2(flags, r15_thread); +#endif

The result from the call is then loaded into flags.

So i don’t think it matters in this case if rcx is aliased.

Paul.

...

-Dmitry

On 10/26/2017 08:03 PM, Paul Sandoz wrote:

...

Hi,

Please review the following patch for minimal dynamic constant support:

http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8186046-minimal-condy-support-... <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.

On 31 Oct 2017, at 05:58, Dmitry Samersoff <dmitry.samersoff@bell-sw.com> wrote:

Paul and Frederic,

Thank you.

One more question. Do we need to call verify_oop below?

509 { // Check for the null sentinel. ... 517 xorptr(result, result); // NULL object reference ...

521 if (VerifyOops) { 522 verify_oop(result); 523 }

I believe it’s harmless. When the flag is on it eventually results in a call to the stub generated by generate_verify_oop: http://hg.openjdk.java.net/jdk10/hs/file/tip/src/hotspot/cpu/x86/stubGenerat... // make sure object is 'reasonable' __ testptr(rax, rax); __ jcc(Assembler::zero, exit); // if obj is NULL it is OK If the oop is null the verification will exit safely. Paul.

-Dmitry

On 31.10.2017 00:56, Frederic Parain wrote:

...

I’m seeing no issue with rcx being aliased in this code.

Fred

...

On Oct 30, 2017, at 15:44, Paul Sandoz <paul.sandoz@oracle.com> wrote:

Hi,

Thanks for reviewing.

...

On 30 Oct 2017, at 11:05, Dmitry Samersoff <dmitry.samersoff@bell-sw.com> wrote:

Paul,

templateTable_x86.cpp:

564 const Register flags = rcx; 565 const Register rarg = NOT_LP64(rcx) LP64_ONLY(c_rarg1);

Should we use another register for rarg under NOT_LP64 ?

I think it should be ok, it i ain’t an expert here on the interpreter and the calling conventions, so please correct me.

Some more context:

+ const Register flags = rcx; + const Register rarg = NOT_LP64(rcx) LP64_ONLY(c_rarg1); + __ movl(rarg, (int)bytecode());

The current bytecode code is loaded into “rarg”

+ call_VM(obj, CAST_FROM_FN_PTR(address, InterpreterRuntime::resolve_ldc), rarg);

Then “rarg" is the argument to the call to InterpreterRuntime::resolve_ldc, after which it is no longer referred to.

+#ifndef _LP64 + // borrow rdi from locals + __ get_thread(rdi); + __ get_vm_result_2(flags, rdi); + __ restore_locals(); +#else + __ get_vm_result_2(flags, r15_thread); +#endif

The result from the call is then loaded into flags.

So i don’t think it matters in this case if rcx is aliased.

Paul.

...

-Dmitry

On 10/26/2017 08:03 PM, Paul Sandoz wrote:

...

Hi,

Please review the following patch for minimal dynamic constant support:

http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8186046-minimal-condy-support-... <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.

On 31 Oct 2017, at 14:43, mandy chung <mandy.chung@oracle.com> wrote:

On 10/26/17 10:03 AM, Paul Sandoz wrote:

...

Hi,

Please review the following patch for minimal dynamic constant support:

http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8186046-minimal-condy-support-... <http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8186046-minimal-condy-support-hs/webrev/>

I reviewed the non-hotspot change as a learning exercise (I am not close to j.l.invoke implementation). I assume DynamicConstant intends to be non-public in this patch, right? 30 public final class DynamicConstant

Well spotted. More likely to be renamed to ConstantBootstraps when a minimal set of dynamic constant bootstraps will be proposed (likely this week) as a follow on patch. I’ll made it non-public in the updated webrev so as to keep this patch self-contained. Paul.

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

On 5 Nov 2017, at 16:55, David Holmes <david.holmes@oracle.com> wrote:

On 4/11/2017 7:28 AM, Paul Sandoz wrote:

...

...

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

Right, i am gonna leave things as they are for now unless we come up with a more definitive answer. Paul.

e.g in SystemDictionary::resolve_instance_class_or_null we have:

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

David ——

On 7 Nov 2017, at 12:04, Karen Kinnear <karen.kinnear@oracle.com> wrote:

Paul,

Thank you for the explanations. Asking for your help in some test case construction at the end here:

...

...

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 I was WRONG here - this does match the JVMS. Apologies for the confusion.

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.

I am wrong, i forgot i updated the API to remove the declaration of the BSM kind, and it only supports static BSMs.

Thank you - that is what we were trying to do - test BSM declared in another class, test in-accessible BSM and test non static method.

The latter (access to a BSM in another class) is easy now using the high-level construct of InstructionHelper.ldcDynamicConstant (see patch for the condy BSMs, which is also in the amber repo in the condy branch).

Could you help us figure out how to 1) invoke a constructor? 2) invoke a virtual method? How do you pass the receiver?

I think for these you will need to use ASM tools to massage the MH for the BSM e.g. develop for a class with a static BSM then adjust the constant pool entry for the associated MH. Paul.

2017/11/20 11:56:42 -0800, paul.sandoz@oracle.com:

The schedule of dynamic constants has been adjusted, the JEP is currently proposed to target for 11.

There is risk to integrating a change such as constant dynamic this late in the 10 release schedule. Instead it's better, and more aligned with the new release process, to integrate early on into the main repository so it has time to bake.

Thanks for the update. It's too bad that JEP 309 won't make JDK 10, but this sounds like the right decision. - Mark