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

Pass separate JVM options to cmdLineTester #17109

Merged
merged 3 commits into from
Apr 5, 2023
Merged

Pass separate JVM options to cmdLineTester #17109

merged 3 commits into from
Apr 5, 2023

Conversation

dsouzai
Copy link
Contributor

@dsouzai dsouzai commented Apr 4, 2023 •
edited
Loading

The existing cmdLineTester tests take options that are supposed to be passed into JVMs that the cmdLineTester invokes and also applies it to the JVM running cmdLineTester.

This PR adds a new variable CMDLINETESTER_JVM_OPTIONS which is set to -Xshareclasses:none. This decouples the options under test from and also prevents situations where the JVM that's under test ends up generating a bad AOT method that the JVM running cmdLineTester could load.

Closes #17095

@dsouzai
Pass separate JVM options to cmdLineTester
853e714 
The existing cmdLineTester tests take options that are supposed to be
passed into JVMs that the cmdLineTester invokes and also applies it to
the JVM running cmdLineTester.

This commit creates a new variable CMDLINETESTER_JVM_OPTIONS which is
set to -Xshareclasses:none. This decouples the options under test from
and also prevents situations where the JVM that's under test ends up
generating a bad AOT method that the JVM runing cmdLineTester could
load.

Signed-off-by: Irwin D'Souza <dsouzai.gh@gmail.com>
@dsouzai
Copy link
Contributor Author

dsouzai commented Apr 4, 2023

@llxia could you please review? Also, do you know if PR builds will use the updated definitions from this PR or should I be launching a separate run elsewhere? I did some local testing, but I didn't run everything and on all platforms.

@pshipton
Copy link
Member

pshipton commented Apr 4, 2023

lgtm

@pshipton pshipton requested a review from llxia April 4, 2023 20:14
@llxia
Copy link
Contributor

llxia commented Apr 4, 2023

jenkins test sanity,extended xlinux jdk11

@dsouzai
Copy link
Contributor Author

dsouzai commented Apr 5, 2023 •
edited
Loading

CryptoTests_0 failure appears to be because of #16720

cmdLineTester_bootStrapStaticVerify_0 looks to be the same issue as #13054:

[2023-04-05T03:33:37.955Z] TEST SETUP:
[2023-04-05T03:33:38.642Z] JVMSHRC005I No shared class caches available
[2023-04-05T03:33:38.642Z] JVMSHRC005I No shared class caches available
[2023-04-05T03:33:38.642Z] cache cleanup done
[2023-04-05T03:33:38.642Z] 
[2023-04-05T03:33:38.642Z] TESTING:
[2023-04-05T03:33:39.320Z] *** Starting test suite: J9 Bootstrap Loader Static Verification Command-Line Option Tests ***
[2023-04-05T03:33:39.320Z] Testing: Check that static verification is off for the bootclasspath
[2023-04-05T03:33:39.320Z] Test start time: 2023/04/04 23:33:38 Eastern Standard Time
[2023-04-05T03:33:39.320Z] Running command: "/home/jenkins/workspace/Test_openjdk11_j9_extended.functional_x86-64_linux_Personal_testList_0/openjdkbinary/j2sdk-image/bin/java"   -Xdump -Xtrace:print={j9bcverify.14} -XX:+StartupOpts -cp /home/jenkins/workspace/Test_openjdk11_j9_extended.functional_x86-64_linux_Personal_testList_0/aqa-tests/TKG/../../jvmtest/functional/cmdLineTests/utils/utils.jar VMBench.FibBench
[2023-04-05T03:33:39.320Z] Time spent starting: 88 milliseconds
[2023-04-05T03:33:40.277Z] Time spent executing: 979 milliseconds
[2023-04-05T03:33:40.277Z] Test result: FAILED
[2023-04-05T03:33:40.277Z] Output from test:
[2023-04-05T03:33:40.277Z]  [OUT] Fibonacci: iterations = 10000
[2023-04-05T03:33:40.277Z]  [OUT] fibonacci(12) = 144
[2023-04-05T03:33:40.277Z]  [OUT] done, time = 115
...
[2023-04-05T03:33:40.277Z]  [ERR] 03:33:39.194 0x17000 j9bcverify(j9vm.14       > j9bcv_verifyClassStructure - class: java/lang/Class
...
[2023-04-05T03:33:40.295Z] >> Success condition was found: [Output match: Fibonacci: iterations]
[2023-04-05T03:33:40.295Z] >> Failure condition was found: [Output match: j9bcv_verifyClassStructure - class: java/lang/Class]
[2023-04-05T03:33:40.295Z] >> Failure condition was not found: [Output match: Unhandled Exception]
[2023-04-05T03:33:40.295Z] >> Failure condition was not found: [Output match: Exception:]
[2023-04-05T03:33:40.295Z] >> Failure condition was not found: [Output match: Processing dump event]

I don't see how adding -Xshareclasses:none to the harness could impact this test, unless there's some weird interaction where the SCC created by the harness somehow allows the test to pass, in which case the underlying test (or the functional issue it exposes) should be fixed.

@pshipton
Copy link
Member

pshipton commented Apr 5, 2023

It's a bug in the test, caused by enabling shared classes by default. When creating a new shared cache verification is run. We can modify the tests to use -Xshareclasses:none, except for the "with -Xshareclasses" one, to keep the original intent.

@pshipton
Copy link
Member

pshipton commented Apr 5, 2023 •
edited
Loading

-XX:+StartupOpts could also be removed. I'm not sure what this was, but it's not an option now.

java -XX:-IgnoreUnrecognizedXXColonOptions -XX:+StartupOpts a
JVMJ9VM007E Command-line option unrecognised: -XX:+StartupOpts

@dsouzai
Add -Xshareclasses:none to relevant bootstrap verification tests
8f6b807 
Enabling Shared Classes by default resulted in a bug in the bootstrap
verification testing. This commit fixes this by adding
-Xshareclasses:none.

Signed-off-by: Irwin D'Souza <dsouzai.gh@gmail.com>
@dsouzai
Remove non-existent -XX:+StartupOpts
ee003f8 
Signed-off-by: Irwin D'Souza <dsouzai.gh@gmail.com>
@dsouzai
Copy link
Contributor Author

dsouzai commented Apr 5, 2023

Made the requested changes:

  • Added -Xshareclasses:none to the relevant verification tests
  • Removed -XX:+StartupOpts in all tests that had that specified.

@pshipton
Copy link
Member

pshipton commented Apr 5, 2023 •
edited
Loading

Running cmdLineTester_bootStrapStaticVerify,cmdLineTester_javaAssertions
https://openj9-jenkins.osuosl.org/view/Test/job/Grinder/2197/ - passed

llxia
llxia approved these changes Apr 5, 2023
View reviewed changes
Copy link
Contributor

@llxia llxia left a comment

Choose a reason for hiding this comment

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

Thanks @dsouzai

@llxia llxia merged commit 93ff7e1 into eclipse-openj9:master Apr 5, 2023
@smlambert smlambert mentioned this pull request Apr 12, 2023
Closed
@pshipton pshipton mentioned this pull request Apr 25, 2023
Closed
@dsouzai dsouzai deleted the cmdLineTesterCleanup branch April 3, 2024 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pshipton pshipton pshipton approved these changes

@llxia llxia llxia approved these changes

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

Successfully merging this pull request may close these issues.

Variations under test are also used as JVM args in cmdLineTester
3 participants
@dsouzai @pshipton @llxia