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

Add -Xverify:none to docs for OpenJ9 #50

Merged
merged 4 commits into from
May 30, 2023

Conversation

mandragorn
Copy link
Contributor

@mandragorn mandragorn commented Apr 18, 2020

This PR tries to address issue #37.

On newer versions of java bytecode is verified before it is run. Since
this agent modifies core java bytecode it needs to bypass this
verification. To turn the verification off pass -Xverify:none as an
argument when running the agent. Fixed in unit tests and docs.

Details: https://www.ibm.com/support/pages/ibm-java-linux-howto-resolving-javalangverifyerror-jvmvrfy012-stack-shape-inconsistent

@MarkEWaite
Copy link
Contributor

MarkEWaite commented May 29, 2023

@mandragorn is this specific to the Eclipse J9 virtual machine? The Jenkins platform SIG stopped efforts to support Eclipse J9 in 2021 because we did not have people with enough time to test it.

If it is not specific to J9, are you still interested in this pull request and willing to resolve the conflicts?

@mandragorn
Copy link
Contributor Author

Wow this is from a lifetime ago, I don't remember the details and I don't have whatever env I used to generate the issue (this was before I switched jobs).

Based on the linked issue #37 and my comment there, it looks like I was testing this with the openjdk jvm, but I don't remember any details beyond that.

I'm happy to rebase and push the update if you'd like me to.

On newer versions of java bytecode is verified before it is run.  Since
this agent modifies core java bytecode it needs to bypass this
verification.  To turn the verification off pass -Xverify:none as an
argument when running the agent.  Fixed in unit tests and docs.

Details: https://www.ibm.com/support/pages/ibm-java-linux-howto-resolving-javalangverifyerror-jvmvrfy012-stack-shape-inconsistent
@mandragorn
Copy link
Contributor Author

conflicts resolved.

Copy link
Member

@basil basil left a comment

Choose a reason for hiding this comment

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

-1 from me, I don't think we should be decreasing verification for HotSpot users (which this PR does by changing the recommendations for all JVMs in the documentation) in order to accommodate OpenJ9. If OpenJ9 requires a decrease in verification, I think that should be discussed in a separate section of the documentation.

@mandragorn
Copy link
Contributor Author

Docs updated.

@mandragorn
Copy link
Contributor Author

Note that I think the bytecode generation that happens means that this doesn't work on any version of java beyond java 8 w/o -Xverify:none. BUT I don't currently have the time or setup to test this to verify that assumption. I do not think it is specific to OpenJ9, but again that is an assumption that I haven't tested.

If anyone has a way to test and confirm or deny those assumptions, let me know and I can update the docs as part of this commit.

pom.xml Outdated
@@ -125,7 +125,7 @@
<exclude>**/TransformerTest.java</exclude>
<exclude>**/AgentMainTest.java</exclude>
</excludes>
<argLine>-javaagent:"${project.build.directory}/file-leak-detector-${project.version}-jar-with-dependencies.jar"</argLine>
<argLine>-javaagent:"${project.build.directory}/file-leak-detector-${project.version}-jar-with-dependencies.jar" -Xverify:none</argLine>
Copy link
Member

Choose a reason for hiding this comment

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

This affects our test suite on HotSpot by decreasing verification. I think this PR should not affect HotSpot. The fact that this was working in our tests on HotSpot without -Xverify:none is evidence that this is only needed for OpenJ9.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

alright, sounds like this pr isn't needed. abandoning.

Copy link
Member

Choose a reason for hiding this comment

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

The change to decrease verification on HotSpot in our test suite isn't needed (as far as I can tell) and isn't wanted (by me). The change to add additional documentation for OpenJ9 users seems valid to me though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I honestly don't remember a lot of details around this from 3 years ago. I think the issue I had was I couldn't run the tests locally because I had java9 setup, I had no idea it was a hotspot issue vs j9. I thought it was a java 8 vs java 9+ issue. I assume the test suite is running in something > java 8 at this point so my initial assumptions are wrong. I don't currently have the bandwidth to test this out in different jvms to see what works and what doesn't so I'm not sure how much value the documentation actually adds w/o those details. I guess I could make the docs more vague to cover it.

Copy link
Member

Choose a reason for hiding this comment

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

We run the test suite with HotSpot (Eclipse Temurin) on Java 11 and 17. Yes, I think a general suggestion in the documentation for OpenJ9 users to run without verification should cover it.

@mandragorn mandragorn closed this May 30, 2023
@mandragorn mandragorn reopened this May 30, 2023
@basil basil changed the title Add -Xverify:none to tests and docs Add -Xverify:none to docs for OpenJ9 May 30, 2023
Copy link
Member

@basil basil left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

@basil basil merged commit 367d9e9 into jenkinsci:master May 30, 2023
@mandragorn
Copy link
Contributor Author

Thank you @basil for the feedback!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants