Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Adds implementation for accessFlags() in Class.java #16217

Merged
merged 1 commit into from
Nov 17, 2022
Merged

Adds implementation for accessFlags() in Class.java #16217

merged 1 commit into from
Nov 17, 2022

Conversation

thallium
Copy link
Contributor

Fixes: #15933
Signed-off-by: Gengchen Tuo gengchen.tuo@ibm.com

@thallium
Copy link
Contributor Author

FYI @hangshao0 @fengxue-IS

@hangshao0
Copy link
Contributor

Did you compare the behaviour of this change with OpenJDK ?

@pshipton
Copy link
Member

Pls ensure we're passing the OpenJDK test java/lang/reflect/AccessFlag/ClassAccessFlagTest.java on jdk20, currently this test is failing. https://openj9-jenkins.osuosl.org/view/Test/job/Grinder/1456/

17:29:35  java.lang.NullPointerException: Cannot invoke "java.util.Set.toString()" because the return value of "java.lang.Class.accessFlags()" is null
17:29:35  	at ClassAccessFlagTest.checkClass(ClassAccessFlagTest.java:79)
17:29:35  	at ClassAccessFlagTest.checkClasses(ClassAccessFlagTest.java:71)
17:29:35  	at ClassAccessFlagTest.main(ClassAccessFlagTest.java:60)

jenkins compile amac jdknext

@thallium
Copy link
Contributor Author

Pls ensure we're passing the OpenJDK test java/lang/reflect/AccessFlag/ClassAccessFlagTest.java on jdk20,

Passed on my local machine.

@thallium thallium force-pushed the accessFlags branch 2 times, most recently from 51bce73 to 5fcb3fd Compare October 27, 2022 21:12
@pshipton
Copy link
Member

Grinder https://openj9-jenkins.osuosl.org/view/Test/job/Grinder/1462/ passed too.

Rerun link
https://openj9-jenkins.osuosl.org/job/Grinder/parambuild/?TARGET=jdk_custom&SDK_RESOURCE=customized&TEST_FLAG=&UPSTREAM_TEST_JOB_NAME=&DOCKER_REQUIRED=false&VENDOR_TEST_DIRS=&TKG_OWNER_BRANCH=AdoptOpenJDK%3Amaster&OPENJ9_SYSTEMTEST_OWNER_BRANCH=eclipse%3Amaster&PLATFORM=aarch64_mac&GENERATE_JOBS=false&PERSONAL_BUILD=true&ADOPTOPENJDK_REPO=https%3A%2F%2Fgithub.com%2Fadoptium%2Faqa-tests.git&LABEL=mac11-aarch64-1&EXTRA_OPTIONS=&CUSTOMIZED_SDK_URL=https%3A%2F%2Fopenj9-artifactory.osuosl.org%2Fartifactory%2Fci-openj9%2FBuild_JDKnext_aarch64_mac_Personal%2F52%2FOpenJ9-JDKnext-aarch64_mac-20221028-072923.tar.gz&DAYS_TO_KEEP=7&ADOPTOPENJDK_BRANCH=master&ARTIFACTORY_SERVER=ci-eclipse-openj9&KEEP_WORKSPACE=false&JDK_VERSION=20&ITERATIONS=1&VENDOR_TEST_REPOS=&JDK_REPO=https%3A%2F%2Fgithub.com%2Fibmruntimes%2Fopenj9-openjdk-jdk.git&OPENJ9_BRANCH=master&OPENJ9_SHA=&VENDOR_TEST_BRANCHES=&OPENJ9_REPO=https%3A%2F%2Fgithub.com%2Feclipse-openj9%2Fopenj9.git&UPSTREAM_JOB_NAME=&CUSTOM_TARGET=&DEBUG_IMAGES_REQUIRED=false&VENDOR_TEST_SHAS=&JDK_BRANCH=openj9&TEST_IMAGES_REQUIRED=true&LABEL_ADDITION=&ARTIFACTORY_REPO=ci-openj9&ARTIFACTORY_ROOT_DIR=Test&UPSTREAM_TEST_JOB_NUMBER=&JDK_IMPL=openj9&AUTO_DETECT=true&DYNAMIC_COMPILE=true&ADOPTOPENJDK_SYSTEMTEST_OWNER_BRANCH=AdoptOpenJDK%3Amaster&CUSTOMIZED_SDK_URL_CREDENTIAL_ID=&ARCHIVE_TEST_RESULTS=false&NUM_MACHINES=1&BUILD_LIST=openjdk&USE_TESTENV_PROPERTIES=false&UPSTREAM_JOB_NUMBER=&STF_OWNER_BRANCH=AdoptOpenJDK%3Amaster&JDK_VENDOR=&BUILDS_TO_KEEP=60&JVM_OPTIONS=&PARALLEL=None

@fengxue-IS fengxue-IS self-requested a review October 27, 2022 22:16
fengxue-IS
fengxue-IS suggested changes Oct 27, 2022
View reviewed changes
Copy link
Contributor

@fengxue-IS fengxue-IS left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One additional point to consider is that the Java EA spec on Class.accessFlags() didn't fully document all possible cases with array classes

The values of its other access flags are not determined by this specification.

So behaviour beyond Public/Private/Protected/Final can change in the future.

Note 2:
I am not sure if we actually need the masking to be done here as javac should be generating the correct access flags in the classfile and a class with an incorrect flag will throw exception in AccessFlag.maskToAccessFlags. we shouldn't try to hide these possible exceptions through masking.

@thallium can you try if directly using results from getModifiersImpl() passes the tests?
Can you also verify the case where the class is a module? (with module-info.class ie.
Class<?> moduleClass = ClassLoader.getSystemClassLoader().findClass("java.base", "module-info"); )

jcl/src/java.base/share/classes/java/lang/Class.java Show resolved Hide resolved
jcl/src/java.base/share/classes/java/lang/Class.java Show resolved Hide resolved
@fengxue-IS
Copy link
Contributor

Just realized that classes with ACC_Module flag is not allowed to be loaded by classloader, so please disregard my comment on testing with module-info.class, I am not sure how the Module access flag is suppose to be validated in testing.

@thallium
Copy link
Contributor Author

can you try if directly using results from getModifiersImpl() passes the tests?

Unfortunately this doesn't work.

fengxue-IS
fengxue-IS approved these changes Nov 15, 2022
View reviewed changes
Copy link
Contributor

@fengxue-IS fengxue-IS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@babsingh can you take a look, and launch the PR build if you are fine with the changes

babsingh
babsingh requested changes Nov 16, 2022
View reviewed changes
Copy link
Contributor

@babsingh babsingh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thallium Also, resolve old comments which have been resolved.

jcl/src/java.base/share/classes/java/lang/Class.java Outdated Show resolved Hide resolved
jcl/src/java.base/share/classes/java/lang/Class.java Outdated Show resolved Hide resolved
jcl/src/java.base/share/classes/java/lang/Class.java Outdated Show resolved Hide resolved
jcl/src/java.base/share/classes/java/lang/Class.java Outdated Show resolved Hide resolved
jcl/src/java.base/share/classes/java/lang/Class.java Outdated Show resolved Hide resolved
@thallium
Adds implementation for accessFlags() in Class.java
82456a3 
Signed-off-by: Gengchen Tuo <gengchen.tuo@ibm.com>
@babsingh
Copy link
Contributor

jenkins test sanity jdknext

@babsingh
Copy link
Contributor

jenkins test sanity amac jdknext

@babsingh
Copy link
Contributor

jenkins compile zlinux jdk11

@babsingh
Copy link
Contributor

@thallium can you reconfirm that java/lang/reflect/AccessFlag/ClassAccessFlagTest.java passes with the latest changes?

@thallium
Copy link
Contributor Author

@thallium can you reconfirm that java/lang/reflect/AccessFlag/ClassAccessFlagTest.java passes with the latest changes?

Yes, still passes

@babsingh babsingh merged commit fddfd3f into eclipse-openj9:master Nov 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JasonFengJ9 JasonFengJ9 JasonFengJ9 left review comments

@babsingh babsingh babsingh approved these changes

@fengxue-IS fengxue-IS fengxue-IS approved these changes

@hangshao0 hangshao0 hangshao0 approved these changes

Assignees
No one assigned
Labels
comp:vm jdk20
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Missing implementation of accessFlags() in Class.java
6 participants
@thallium @hangshao0 @pshipton @fengxue-IS @babsingh @JasonFengJ9