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.

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

8291360: Create entry points to expose low-level class file information #9688

Closed
wants to merge 4 commits into from

Conversation

hseigel
Copy link
Member

@hseigel hseigel commented Jul 29, 2022

Please review this change to fix JDK-8291360. This fix adds entry points getClassFileVersion() and getClassAccessFlagsRaw() to class java.lang.Class. The new entry points return the current class's class file version and its raw access flags.

The fix was tested by running Mach5 tiers 1-2 on Linux, Mac OS, and Windows, and Mach5 tiers 1-3 on Linux x64. Additionally, the JCK lang, vm, and api tests and new regression tests were run locally on Linux x64.

Thanks, Harold


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issue

  • JDK-8291360: Create entry points to expose low-level class file information

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/9688/head:pull/9688
$ git checkout pull/9688

Update a local copy of the PR:
$ git checkout pull/9688
$ git pull https://git.openjdk.org/jdk pull/9688/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9688

View PR using the GUI difftool:
$ git pr show -t 9688

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/9688.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Jul 29, 2022

👋 Welcome back hseigel! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr Pull request is ready for review label Jul 29, 2022
@openjdk
Copy link

openjdk bot commented Jul 29, 2022

@hseigel The following labels will be automatically applied to this pull request:

  • build
  • core-libs
  • hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added build build-dev@openjdk.org hotspot hotspot-dev@openjdk.org core-libs core-libs-dev@openjdk.org labels Jul 29, 2022
* If the class is an array type then the access flags of the component type is
* returned. If the class is a primitive then ACC_ABSTRACT | ACC_FINAL | ACC_PUBLIC.
*/
private int getClassAccessFlagsRaw() {
Copy link
Member

Choose a reason for hiding this comment

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

For a "raw" method, it might be better to return the flags on the array class object itself rather than loop down to the component type.

Copy link
Member Author

Choose a reason for hiding this comment

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

There's no bytecode stream for arrays. It's created using anewarray with a dimension operand and a cp pointer to the component type. So there are no flags for the array object.

@mlbridge
Copy link

mlbridge bot commented Jul 29, 2022

Webrevs

src/hotspot/share/prims/jvm.cpp Show resolved Hide resolved
*
*/

// Class with ACC_SUPER set
Copy link
Contributor

Choose a reason for hiding this comment

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

Can these classes be defined more succinctly either in Java or .asm?

Copy link
Member Author

Choose a reason for hiding this comment

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

The jcod test files are relatively small so I don' think they need to be rewritten using asm.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, It mostly knowing which few of the lines are relevant to the test vs. just boilerplate to make a complete class.

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Choose a reason for hiding this comment

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

Hi Harold,

Generally seems fine. A few nits and comments.

Thanks.

src/hotspot/share/include/jvm.h Outdated Show resolved Hide resolved
src/hotspot/share/prims/jvm.cpp Outdated Show resolved Hide resolved
src/hotspot/share/prims/jvm.cpp Outdated Show resolved Hide resolved
src/hotspot/share/prims/jvm.cpp Show resolved Hide resolved
* the original setting of ACC_SUPER.
*
* If the class is an array type then the access flags of the component type is
* returned. If the class is a primitive then ACC_ABSTRACT | ACC_FINAL | ACC_PUBLIC.
Copy link
Member

Choose a reason for hiding this comment

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

The ACC_ABSTRACT seems odd - is that way of indicating this "class" can't be instantiated? Is there some spec document that explains this choice?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know why API's such as JVM_GetClassModifiers() and JVM_GetClassAccessFlags return ACC_ABSTRACT | ACC_FINAL | ACC_PUBLIC for primitives. Nor could I find a spec that discussed access flags for primitives. But, I didn't want to change what's been returned for primitives in existing code.

Copy link
Member

Choose a reason for hiding this comment

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

Interesting. Class.getModifiers() does not specify ACC_ABSTRACT so this seems to be something implementation specific (although no doubt a long standing situation.)

Comment on lines 44 to 45
throw new RuntimeException(
"expected " + expectedResult + ", got " + ver + " for class " + className);
Copy link
Member

Choose a reason for hiding this comment

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

It would be clearer to show the expected and actual in minor:major format. That way if the test fails we can easily see which bit is wrong.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed to display as major:minor version.

Copy link
Member

Choose a reason for hiding this comment

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

Seems odd to display as major:minor when it is stored as minor:major

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it seems more natural to display the major version first, but I can change it if you prefer.

Copy link
Member

Choose a reason for hiding this comment

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

I think it should match the underlying representation else it may cause someone trying to debug a failure to look at the wrong part of the flags value, not realizing the printed value has the opposite format of the actual raw value.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do humans read this or computers? A human would want major:minor because that would make more sense. Isn't this a test? Just a drive by comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

@dholmes-ora Please see latest commit that changes the test to display minor:major.

Copy link
Member

Choose a reason for hiding this comment

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

Latest change is good.

@coleenp this is for humans reading failed test output. If it reports "expected 0:1 but got 1:1" then the natural tendency would be to look for an issue in the upper word, when in fact the problem is in the lower word.

// test primitive array. should return latest version.
int ver = (int)m.invoke((new int[3]).getClass());
if (ver != 64) {
throw new RuntimeException("expected 64, got " + ver + " for primitive array");
Copy link
Member

Choose a reason for hiding this comment

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

Again minor:major format.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed to display as major:minor version.

@hseigel
Copy link
Member Author

hseigel commented Aug 1, 2022

Thanks Brian, Roger, and David for looking at this. I pushed a new commit to address the review comments, add mulit-dimensional array test cases, and simplify new java.lang.Class API's.

component = component.getComponentType();
} while (component.isArray());
}
Class<?> component = isArray() ? elementType() : this;
Copy link
Member

Choose a reason for hiding this comment

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

The variable name component seems completely not applicable now. I would just use c.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

* the original setting of ACC_SUPER.
*
* If the class is an array type then the access flags of the component type is
* returned. If the class is a primitive then ACC_ABSTRACT | ACC_FINAL | ACC_PUBLIC.
Copy link
Member

Choose a reason for hiding this comment

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

Interesting. Class.getModifiers() does not specify ACC_ABSTRACT so this seems to be something implementation specific (although no doubt a long standing situation.)

Comment on lines +66 to +67
int got_minor = (ver >> 16) & 0x0000FFFF;
int got_major = ver & 0x0000FFFF;
Copy link
Member

Choose a reason for hiding this comment

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

got seems odd in this context as these are not boolean queries.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry I "got" it now :)

Copy link
Contributor

@RogerRiggs RogerRiggs left a comment

Choose a reason for hiding this comment

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

LGTM

@openjdk
Copy link

openjdk bot commented Aug 2, 2022

@hseigel This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8291360: Create entry points to expose low-level class file information

Reviewed-by: dholmes, rriggs

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 51 new commits pushed to the master branch:

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Aug 2, 2022
@hseigel
Copy link
Member Author

hseigel commented Aug 4, 2022

Thanks Roger, David, Joe, and Coleen for your comments and reviews!

/integrate

@openjdk
Copy link

openjdk bot commented Aug 4, 2022

Going to push as commit a3040fc.
Since your change was applied there have been 51 commits pushed to the master branch:

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Aug 4, 2022
@openjdk openjdk bot closed this Aug 4, 2022
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Aug 4, 2022
@openjdk
Copy link

openjdk bot commented Aug 4, 2022

@hseigel Pushed as commit a3040fc.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build build-dev@openjdk.org core-libs core-libs-dev@openjdk.org hotspot hotspot-dev@openjdk.org integrated Pull request has been integrated
5 participants