Update the Lookup specific code against the JDK14 Spec by ChengJin01 · Pull Request #8657 · eclipse-openj9/openj9

ChengJin01 · 2020-02-25T19:25:04Z

The change is to update the code related to MH.Lookup
to deal with the change of the full privilege access
and the new concept called "previous lookup class" introduced
in JDK14.

Fixes: #8570, Fixes: #8571

Signed-off-by: Cheng Jin [email protected]

ChengJin01 · 2020-02-25T19:27:40Z

The code changes are still ongoing and the internal test case update is temporarily excluded for the moment as the new changes have no backwards compatibility with the old version (which means the tests involved need to be updated/modified to deal with these situations)

Reviewer: @DanHeidinga

DanHeidinga

More comments to follow but github soft outage preventing me from commenting

DanHeidinga · 2020-02-26T03:55:53Z

Suggested change

if ( lookupClass.getName().startsWith("java.lang.invoke.")) { //$NON-NLS-1$

if (lookupClass.getName().startsWith("java.lang.invoke.")) { //$NON-NLS-1$

DanHeidinga · 2020-02-26T04:06:19Z

@JasonFengJ9 Any thoughts on whether there's a cleaner way to do the jcl preprocessor tags here with less duplication?

Sincere apology for late response, somehow I didn't catch the git notification (and strangely it didn't show up with mention query in the past).
I think JPP tags could be organized as following:

Suggested change

static Lookup PUBLIC_LOOKUP = new Lookup(Object.class, Lookup.PUBLIC);

static final int mhMask =

/*[IF Java11]

/*[IF Java14]*/

Lookup.UNCONDITIONAL;

/*[ELSE]*/

Lookup.PUBLIC | Lookup.UNCONDITIONAL;

/*[ENDIF] Java14 */

/*[ELSE] Java11 */

Lookup.PUBLIC;

/*[ENDIF] Java11 */

static Lookup PUBLIC_LOOKUP = new Lookup(Object.class, mhMask);

Alternatively static final int mhMask = could be moved within the JPP decoration to avoid the actual code like

static final int mhMask = Lookup.PUBLIC;

I will update the code against the suggestion above by @JasonFengJ9 if there is no other concern on the format.

The code there has been updated as suggested above.

DanHeidinga · 2020-02-26T04:09:25Z

Are all the new forms of these ctors used? Let's only add the ones we actually need

Removed unused constructors as suggested above.

DanHeidinga · 2020-02-27T15:30:49Z

Suggested change

DanHeidinga · 2020-03-02T20:36:00Z

Use Objects.requireNonNull(lookupClass) here rather than the explicit method call for a null check

Agreed and fixed.

DanHeidinga · 2020-03-02T20:40:06Z

This is a good use case for a switch statement:

switch(dropMode) { case PUBLIC: case MODULE: case PACKAGE: case PRIVATE: case PROTECTED: case UNCONDITIONAL: /* dropMode is OK */ break; default: /*[MSG "K065R", "The requested lookup mode: 0x{0} is not one of the existing access modes: 0x{1}"]*/ throw new IllegalArgumentException(com.ibm.oti.util.Msg.getString("K065R", Integer.toHexString(dropMode), Integer.toHexString(fullAccessMode))); }

Agreed and replaced with switch statement.

ChengJin01 · 2020-03-02T21:24:02Z

The code changes in Lookup::in() will be updated later according to the clarification from Oracle as to teleporting across modules (so far the changes are based on the expected behaviour in test which has not yet been confirmed by Oracle)

Considering the difference of behaviour on Java 11 and 14, I will temporarily disable the Lookup specific test (in /test/functional/OpenJ9_Jsr_292_API) on Java 14 in this PR based on the results in personal builds and create a separate PR to address the tests with the new code changes.

ChengJin01 · 2020-03-03T19:41:57Z

@llxia, please help to double-check the changes in test scripts to see whether it still aligns with the existing test framework. We'd like to disable these tests involved temporarily on JDK14 due to the new changes in Spec and will create another PR to re-evaluate/address them all later.

JasonFengJ9 · 2020-03-03T19:45:18Z

nitpick : pls add Java14 here.

JasonFengJ9

PUBLIC_LOOKUP part change looks good to me.

ChengJin01 · 2020-03-04T23:06:26Z

@DanHeidinga, you can keep reviewing the code as there should be no more update except Lookup::dropLookupMode() which needs the clarification from Oracle as to how the MODULE bit is used in there. For now, the code changes in Lookup::dropLookupMode() are based on the expected behaviour from the jtreg tests. So there won't be any update if Oracle considers it as correct.

DanHeidinga · 2020-03-05T14:59:22Z

@tajila Can you review this as well as you've been in this code before? It's well worth reviewing some of the cross-module cases as listed in https://download.java.net/java/early_access/jdk14/docs/api/java.base/java/lang/invoke/MethodHandles.Lookup.html#access-modes

llxia · 2020-03-05T15:35:21Z

The changes in playlist.xml and testng.xml look good me. Please add comments in playlist and reference to the issue as this is a temporarily exclude. Thanks

ChengJin01 · 2020-03-05T18:12:11Z

The changes in playlist.xml and testng.xml look good me. Please add comments in playlist and reference to the issue as this is a temporarily exclude. Thanks

Many thanks @llxia , the comments have been added to these playlists involved.

DanHeidinga

More to come

DanHeidinga · 2020-03-04T15:49:30Z

void.class.isPrimitive() is true

Suggested change

if ((lookupClass == void.class) || lookupClass.isPrimitive() || lookupClass.isArray()) {

if (lookupClass.isPrimitive() || lookupClass.isArray()) {

Agreed and removed lookupClass == void.class against the suggestion above.

DanHeidinga · 2020-03-05T16:15:34Z

Why would dropLookupMode ever modify the previousAccessClass?

at http://cr.openjdk.java.net/~iris/se/14/build/latest/api/java.base/java/lang/invoke/MethodHandles.Lookup.html#UNCONDITIONAL:

public static final int UNCONDITIONAL ... If this lookup mode is set, the previous lookup class is always null. <----- this is the new change when setting up UNCONDITIONAL since Java 14

The newly added description requires to handle previousAccessClass which is not specific to dropLookupMode() but everywhere when the UNCONDITIONAL bit is set initially.

DanHeidinga · 2020-03-05T16:36:21Z

void.class.isPrimitive is true so we don't need to first check

DanHeidinga · 2020-03-05T19:27:41Z

Why is this an exception? I don't see anything about this in the "Access modes" table showing dropLookupMode behaviour in https://download.java.net/java/early_access/jdk14/docs/api/java.base/java/lang/invoke/MethodHandles.Lookup.html#access-modes

It seems like this should apply to all dropped modes & the change to switch (dropMode & newAccessMode) is unnecessary

As mentioned at #8571 (comment),

public void testDropLookupMode() throws Exception { Lookup lookup = MethodHandles.privateLookupIn(m5.type1, m4.lookup); assertTrue((lookup.lookupModes() & MODULE) == 0); <--- MODULE doesn't exist ... Lookup lookup3 = lookup.dropLookupMode(MODULE); ---> assertTrue(lookup3.lookupModes() == (lookup.lookupModes() & ~(PROTECTED|PRIVATE|PACKAGE)));

Here's the printing message for the test above

[TestNG] Running: java/lang/invoke/modules/Driver1.java config jdk.test.ModuleAccessTest.setup(): success ----- testDropLookupMode -------- m4.lookup.lookupModes() = 31 m4.lookup.lookupClass() = class d1.D1 m4.lookup.lookupClass().getModule() = module m4 m5.type1.getModule() = module m5 lookup = e1.E1/d1.D1 lookup.lookupClass = class e1.E1 lookup.lookupClass().getModule() = module m5 lookup.lookupModes() = 15 <--------------------- 0xF or 1111 <---- MODULE doesn't exist lookup.previousLookupClass() = class d1.D1 lookup.lookupModes() & ~(PROTECTED|PRIVATE|PACKAGE) = 1 lookup3.lookupModes() = 1 <------------------------------ PUBLIC is left eventually test jdk.test.ModuleAccessTest.testDropLookupMode(): failure

Based on the weird result, the generated lookup doesn't hold the MODULE bit, in which case it shouldn't drop all PROTECTED|PRIVATE|PACKAGE as if the MODULE bit exists if the previous clarification from Oracle (mode to be dropped must exist in the access mode) is reasonable; Otherwise, Oracle should bring up with another explanation as to how this happens and why.

DanHeidinga · 2020-03-05T19:42:06Z

Suggested change

&& (Modifier.isPublic(targetClassModifiers) || Modifier.isProtected(targetClassModifiers))

&& targetClassIsPublic

DanHeidinga · 2020-03-05T19:42:36Z

Suggested change

/* A protected class (must be a member class) is compiled to a public class as

* the protected flag of this class doesn't exist on the VM level (there is no

* access flag in the binary form representing 'protected')

*/

if (Modifier.isPublic(targetClassModifiers) || Modifier.isProtected(targetClassModifiers)) {

if (targetClassIsPublic) {

DanHeidinga · 2020-03-05T19:58:20Z

Suggested change

&& (Modifier.isPublic(targetClassModifiers) || Modifier.isProtected(targetClassModifiers))

&& (targetClassIsPublic)

DanHeidinga · 2020-03-05T19:58:36Z

Suggested change

&& (Modifier.isPublic(targetClassModifiers) || Modifier.isProtected(targetClassModifiers))

&& (targetClassIsPublic)

The change is to update the code related to MH.Lookup to deal with the change of the full privilege access and the new concept called previous lookup class introduced in JDK14. Fixes: eclipse-openj9#8570, Fixes: eclipse-openj9#8571 Signed-off-by: Cheng Jin <[email protected]>

DanHeidinga · 2020-03-06T04:54:28Z

Jenkins test sanity zlinux jdk11,jdk14

DanHeidinga · 2020-03-06T04:54:47Z

Jenkins test extended xlinux jdk14

DanHeidinga · 2020-03-06T04:55:04Z

Jenkins test sanity osx jdk8

DanHeidinga · 2020-03-06T04:56:39Z

I've launched builds on this to ensure it's passing on all platforms and releases. Given the tests pass and behaviour matches the RI, this may be the final form

DanHeidinga · 2020-03-06T16:44:26Z

@ChengJin01 Can you open a PR with this change against the v0.19.0-release branch?

DanHeidinga

Approving given this matches the RIs behaviour and the JTREGs.

ChengJin01 · 2020-03-06T17:16:50Z

@DanHeidinga , the requested PR against v0.19.0-release has been created at #8776

ChengJin01 mentioned this pull request Feb 25, 2020

JTReg Test Failure: java/lang/invoke/DropLookupModeTest.java and java/lang/invoke/lookup/LookupClassTest.java #8570

Closed

ChengJin01 changed the title ~~[WIP] Update the Lookup specific code against the JKD14 Spec~~ [WIP] Update the Lookup specific code against the JDK14 Spec Feb 25, 2020

ChengJin01 force-pushed the mh_lookup_update_jdk14 branch 2 times, most recently from 3b68d09 to 276e2b4 Compare February 26, 2020 00:05

DanHeidinga added comp:vm jdk14 labels Feb 26, 2020

DanHeidinga added this to the Release 0.19 (Java 14) milestone Feb 26, 2020

ChengJin01 force-pushed the mh_lookup_update_jdk14 branch 2 times, most recently from 0a5357a to 0fe4d0e Compare February 27, 2020 17:22

DanHeidinga reviewed Feb 27, 2020

View reviewed changes

ChengJin01 force-pushed the mh_lookup_update_jdk14 branch 2 times, most recently from 7c16326 to b419269 Compare February 28, 2020 19:31

ChengJin01 mentioned this pull request Feb 28, 2020

Java 14 MethodHandles::privateLookupIn may not produce a Lookup with full privilege access #8571

Closed

ChengJin01 force-pushed the mh_lookup_update_jdk14 branch from b419269 to 893f1f7 Compare February 28, 2020 20:37

DanHeidinga reviewed Mar 2, 2020

View reviewed changes

ChengJin01 force-pushed the mh_lookup_update_jdk14 branch from 893f1f7 to 27703cc Compare March 2, 2020 21:06

ChengJin01 force-pushed the mh_lookup_update_jdk14 branch 4 times, most recently from 593cac9 to 99faacc Compare March 3, 2020 19:35

JasonFengJ9 reviewed Mar 3, 2020

View reviewed changes

ChengJin01 force-pushed the mh_lookup_update_jdk14 branch from 99faacc to 85bb47f Compare March 3, 2020 19:46

JasonFengJ9 approved these changes Mar 3, 2020

View reviewed changes

ChengJin01 force-pushed the mh_lookup_update_jdk14 branch from 85bb47f to 2e5e675 Compare March 4, 2020 21:10

ChengJin01 force-pushed the mh_lookup_update_jdk14 branch from 2e5e675 to f947deb Compare March 5, 2020 18:10

ChengJin01 force-pushed the mh_lookup_update_jdk14 branch from f947deb to fd6f62f Compare March 5, 2020 18:16

DanHeidinga reviewed Mar 5, 2020

View reviewed changes

Comment thread jcl/src/java.base/share/classes/java/lang/invoke/MethodHandles.java Outdated

DanHeidinga reviewed Mar 5, 2020

View reviewed changes

ChengJin01 force-pushed the mh_lookup_update_jdk14 branch from fd6f62f to 2951b12 Compare March 5, 2020 20:51

ChengJin01 changed the title ~~[WIP] Update the Lookup specific code against the JDK14 Spec~~ Update the Lookup specific code against the JDK14 Spec Mar 5, 2020

ChengJin01 force-pushed the mh_lookup_update_jdk14 branch from 2951b12 to 9d59e82 Compare March 5, 2020 21:12

DanHeidinga approved these changes Mar 6, 2020

View reviewed changes

DanHeidinga merged commit 66c281a into eclipse-openj9:master Mar 6, 2020

pshipton modified the milestones: Release 0.19 (Java 14), Release 0.20 (Java 8, 11, 14) Apr refresh Mar 6, 2020

ChengJin01 mentioned this pull request Mar 22, 2020

Update MH.Lookup specific test cases against Java 14 Spec #8943

Closed

ChengJin01 deleted the mh_lookup_update_jdk14 branch March 5, 2021 16:06

	if ( lookupClass.getName().startsWith("java.lang.invoke.")) { //$NON-NLS-1$
	if (lookupClass.getName().startsWith("java.lang.invoke.")) { //$NON-NLS-1$

-		static Lookup PUBLIC_LOOKUP = new Lookup(Object.class, Lookup.PUBLIC);
+		static final int mhMask =
+		/*[IF Java11]
+		/*[IF Java14]*/
+		Lookup.UNCONDITIONAL;
+		/*[ELSE]*/
+		Lookup.PUBLIC | Lookup.UNCONDITIONAL;
+		/*[ENDIF] Java14 */
+		/*[ELSE] Java11 */
+		Lookup.PUBLIC;
+		/*[ENDIF] Java11 */
+		static Lookup PUBLIC_LOOKUP = new Lookup(Object.class, mhMask);

	if ((lookupClass == void.class) \|\| lookupClass.isPrimitive() \|\| lookupClass.isArray()) {
	if (lookupClass.isPrimitive() \|\| lookupClass.isArray()) {

	&& (Modifier.isPublic(targetClassModifiers) \|\| Modifier.isProtected(targetClassModifiers))
	&& targetClassIsPublic

-					/* A protected class (must be a member class) is compiled to a public class as
-					 * the protected flag of this class doesn't exist on the VM level (there is no
-					 * access flag in the binary form representing 'protected')
-					 */
-					if (Modifier.isPublic(targetClassModifiers) || Modifier.isProtected(targetClassModifiers)) {
+					if (targetClassIsPublic) {

	&& (Modifier.isPublic(targetClassModifiers) \|\| Modifier.isProtected(targetClassModifiers))
	&& (targetClassIsPublic)

Conversation

ChengJin01 commented Feb 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ChengJin01 commented Feb 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DanHeidinga left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChengJin01 commented Mar 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ChengJin01 commented Mar 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JasonFengJ9 left a comment

Choose a reason for hiding this comment

Uh oh!

ChengJin01 commented Mar 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DanHeidinga commented Mar 5, 2020

Uh oh!

llxia commented Mar 5, 2020

Uh oh!

ChengJin01 commented Mar 5, 2020

Uh oh!

DanHeidinga left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

ChengJin01 commented Feb 25, 2020 •

edited

Loading

ChengJin01 commented Feb 25, 2020 •

edited

Loading

ChengJin01 commented Mar 2, 2020 •

edited

Loading

ChengJin01 commented Mar 3, 2020 •

edited

Loading

ChengJin01 commented Mar 4, 2020 •

edited

Loading