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

8242152: SA does not include StackMapTables when dumping .class files #14556

Closed
wants to merge 3 commits into from

Conversation

quadhier
Copy link
Contributor

@quadhier quadhier commented Jun 20, 2023

This patch adds StackMapTable for the class files generated by clhsdb's buildreplayjars command. This bug manifests itself during my diagnosing JDK-8309751 and needs to be fixed first.

I have run jtreg test tier1-3 of release build on x86 linux finding only one failure in tier2 caused by JDK-8309214. jtreg:test/hotspot/jtreg/serviceability and jtreg:test/jdk/sun/tools/ also passed.


Progress

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

Issue

  • JDK-8242152: SA does not include StackMapTables when dumping .class files (Bug - P3)

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 14556

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

Using diff file

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

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Jun 20, 2023

👋 Welcome back quadhier! 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 Jun 20, 2023
@openjdk
Copy link

openjdk bot commented Jun 20, 2023

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

  • hotspot-gc
  • serviceability

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 serviceability serviceability-dev@openjdk.org hotspot-gc hotspot-gc-dev@openjdk.org labels Jun 20, 2023
@quadhier
Copy link
Contributor Author

/label remove hotspot-gc

@openjdk openjdk bot removed the hotspot-gc hotspot-gc-dev@openjdk.org label Jun 20, 2023
@openjdk
Copy link

openjdk bot commented Jun 20, 2023

@quadhier
The hotspot-gc label was successfully removed.

@mlbridge
Copy link

mlbridge bot commented Jun 20, 2023

Webrevs

@ashu-mehra
Copy link
Contributor

@quadhier you beat me to this! Changes look good.

On another note since you mentioned JDK-8309751 I should let you know I have a fix for it and am planning to open a PR soon. My bad that I didn't assign that issue to myself before starting working on it.

@quadhier
Copy link
Contributor Author

On another note since you mentioned JDK-8309751 I should let you know I have a fix for it and am planning to open a PR soon.

Good news! Never mind and thanks for your notice 😄.

Copy link
Contributor

@plummercj plummercj left a comment

Choose a reason for hiding this comment

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

I made a couple of minor suggestions. Otherwise it looks good.

@openjdk
Copy link

openjdk bot commented Jun 21, 2023

@quadhier 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:

8242152: SA does not include StackMapTables when dumping .class files

Reviewed-by: cjplummer, sspitsyn

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 32 new commits pushed to the master branch:

  • 3661cde: 8309853: StructuredTaskScope.join description improvements
  • ac44ef1: 8310586: ProblemList java/lang/ScopedValue/StressStackOverflow.java#default with virtual threads on linux-all
  • 3be50da: 8310512: Cleanup indentation in jfc files
  • f286e5a: 8310575: no @since for StandardDoclet
  • 16a34e8: 8310384: Add hooks for custom image creation
  • 226c6a0: 8309883: no @since info in com.sun.tools.javac package-info.java, Main.java
  • fd1163d: 8310332: Fix -Wconversion warnings in MethodData
  • 72501cf: 8310379: Relax prerequisites for java.base-jmod target
  • 59c6c0e: 8310335: JFR: Modernize collections and switch statements
  • 826dcb5: 8264899: C1: -XX:AbortVMOnException does not work if all methods in the call stack are compiled with C1 and there are no exception handlers
  • ... and 22 more: https://git.openjdk.org/jdk/compare/4ca548fe74419dc9e110489e3d2d3adf695ef37f...master

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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@plummercj, @sspitsyn) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Jun 21, 2023
Copy link
Contributor

@sspitsyn sspitsyn left a comment

Choose a reason for hiding this comment

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

This looks good in general.
I've posted one nit about formatting.
What test coverage does this update have?

@quadhier
Copy link
Contributor Author

Thank @plummercj and @sspitsyn for your reviews!

What test coverage does this update have?

AFAICS, there are two jtreg tests for class dump — one is test/hotspot/jtreg/serviceability/sa/ClhsdbDumpclass.java and another is test/hotspot/jtreg/serviceability/sa/TestClassDump.java. The former invoke javap on the generated class file to see if it is valid class file while the later directly "invoke" sun.jvm.hotspot.tools.jcore.ClassDump to see if it runs without exception.

Maybe we could add a validation in ClhsdbDumpclass.java to ensure that the generated class file contains some StackMapTables? (As the class under validation LingeredApp should has StackMapTables for some methods.)

@quadhier
Copy link
Contributor Author

/reviewers 2

@openjdk
Copy link

openjdk bot commented Jun 22, 2023

@quadhier
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

@openjdk openjdk bot removed the ready Pull request is ready to be integrated label Jun 22, 2023
@sspitsyn
Copy link
Contributor

sspitsyn commented Jun 22, 2023

Maybe we could add a validation in ClhsdbDumpclass.java to ensure that the generated class file contains some StackMapTables? (As the class under validation LingeredApp should has StackMapTables for some methods.)

It'd be nice to add if it is not hard to do.
Thank you for the update and answer.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Jun 22, 2023
@quadhier
Copy link
Contributor Author

quadhier commented Jun 22, 2023

A simple check has been added. For optional attributes like StackMapTable, a comprehensive check would involve comparing the original class file with the generated one. However, I believe that this deserves another PR.

FYI, jtreg:test/hotspot/jtreg/serviceability still passes.

@quadhier
Copy link
Contributor Author

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Jun 22, 2023
@openjdk
Copy link

openjdk bot commented Jun 22, 2023

@quadhier
Your change (at version d28b785) is now ready to be sponsored by a Committer.

@sspitsyn
Copy link
Contributor

/sponsor

@openjdk
Copy link

openjdk bot commented Jun 22, 2023

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

  • 3bc475e: 8309909: remove test nsk.jvmti test objmonusage006 from ProblemList-Virtual.txt
  • 3661cde: 8309853: StructuredTaskScope.join description improvements
  • ac44ef1: 8310586: ProblemList java/lang/ScopedValue/StressStackOverflow.java#default with virtual threads on linux-all
  • 3be50da: 8310512: Cleanup indentation in jfc files
  • f286e5a: 8310575: no @since for StandardDoclet
  • 16a34e8: 8310384: Add hooks for custom image creation
  • 226c6a0: 8309883: no @since info in com.sun.tools.javac package-info.java, Main.java
  • fd1163d: 8310332: Fix -Wconversion warnings in MethodData
  • 72501cf: 8310379: Relax prerequisites for java.base-jmod target
  • 59c6c0e: 8310335: JFR: Modernize collections and switch statements
  • ... and 23 more: https://git.openjdk.org/jdk/compare/4ca548fe74419dc9e110489e3d2d3adf695ef37f...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Jun 22, 2023
@openjdk openjdk bot closed this Jun 22, 2023
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Jun 22, 2023
@openjdk
Copy link

openjdk bot commented Jun 22, 2023

@sspitsyn @quadhier Pushed as commit 8e04702.

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

@quadhier
Copy link
Contributor Author

On no. CI shows that my added test statement passes on Linux but fails on Windows. This is sort of wierd. Shouldn't class file format be platform agnostic? I'd take a look at it.

@dholmes-ora
Copy link
Member

We see a tier1 failure on Linux-x64-debug running serviceability/sa/ClhsdbDumpclass.java

ava.lang.RuntimeException: Test ERROR java.lang.RuntimeException: 'StackMapTable:' missing from stdout/stderr
	at ClhsdbDumpclass.main(ClhsdbDumpclass.java:97)
	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
	at com.sun.javatest.regtest.agent.MainActionHelper$AgentVMRunnable.run(MainActionHelper.java:333)
	at java.base/java.lang.Thread.run(Thread.java:1570)
Caused by: java.lang.RuntimeException: 'StackMapTable:' missing from stdout/stderr
	at jdk.test.lib.process.OutputAnalyzer.shouldContain(OutputAnalyzer.java:221)
	at ClhsdbDumpclass.main(ClhsdbDumpclass.java:92)
	... 4 more

@dholmes-ora
Copy link
Member

What output is the test looking for i.e why is supposed to be writing it? If it is the if (DEBUG) debugMessage(...) calls they are not enabled.

@quadhier
Copy link
Contributor Author

quadhier commented Jun 22, 2023

My fault. Thanks for your reporting, David. I will look into this immediately. I didn't find that maybe because I run tests on Linux using release build.

@dholmes-ora
Copy link
Member

@quadhier
Copy link
Contributor Author

What output is the test looking for i.e why is supposed to be writing it? If it is the if (DEBUG) debugMessage(...) calls they are not enabled.

It is looking for StackMapTable info in the javap output.

@quadhier
Copy link
Contributor Author

I've filed https://bugs.openjdk.org/browse/JDK-8310618

Thanks, David! Sorry for any inconvenience casued by that.

@kevinjwalls
Copy link
Contributor

I see the failure too. javap needs "-v" to show the StackMap info, I see that on linux release and debug builds.

@quadhier
Copy link
Contributor Author

I see the failure too. javap needs "-v" to show the StackMap info, I see that on linux release and debug builds.

Exactly! @kevinjwalls. And a situation similar to this one happen again. Thanks for your investigation. I would fix it ASAP.

@dcubed-ojdk
Copy link
Member

Folks the GHA for this PR reported a failure in serviceability/sa/ClhsdbDumpclass
on windows-x64 so that should have been a red flag that something was amiss.

The actual integration reported failures on windows-x64, linux-aarch64 and linux-x64
in Mach5 Tier1. Mach5 Tier3 reports a total of 14 failures on the same three platforms.
Mach5 Tier4 reports 3 failures so far on the same three platforms.

@quadhier
Copy link
Contributor Author

quadhier commented Jun 22, 2023

Hi, @dcubed-ojdk, yes, sorry for that. And it should have been fixed in this PR #14612.

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