8339871: serviceability/sa/TestDebugInfoDecode.java should be driver

[rsunderbabu]

Passing "-Xmx1g -Xcomp" to the LingeredApp.
Testing: tier1

---

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-8339871: serviceability/sa/TestDebugInfoDecode.java should be driver (Enhancement - P5)

Reviewers

• Chris Plummer (@plummercj - Reviewer)
• Leonid Mesnik (@lmesnik - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 21519

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back rsunderbabu! 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]

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

8339871: serviceability/sa/TestDebugInfoDecode.java should be driver

Reviewed-by: cjplummer, lmesnik

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

• 7ff4ea8: 8341966: Broken annotated module may lead to an exception in javac
• 9bdface: 8341622: Tag-specific disabled default decorators for UnifiedLogging
• 1ea1f33: 8340336: Open some checkbox awt tests
• 7a64fbb: 8338851: Hoist os::Posix::realpath() to os::realpath() and implement on Windows
• 8862ca0: 8342338: Remove redundant IIOPURLTest.java
• f9208fa: 8341715: PPC64: ObjectMonitor::_owner should be reset unconditionally in nmethod unlocking
• fa39e84: 8342042: PPC64: compiler_fast_unlock_object flags failure instead of success
• e0dabfb: 8340279: Open source several AWT Dialog tests - Batch 2
• f6fe5ea: 8341072: Open source several AWT Canvas and Rectangle related tests
• 22a1fee: 8341328: Refactor initial Assertion Predicate creation into separate classes
• ... and 43 more: https://git.openjdk.org/jdk/compare/a601cd2e100958e3f37ae65e32e4b3cac246c079...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, @lmesnik) 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]

@rsunderbabu The following label will be automatically applied to this pull request:

• serviceability

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

[mlbridge]

Webrevs

• 01: Full - Incremental (7b3c8847)
• 00: Full (47c105b4)

[plummercj]

We have 3 processes here:

1. The test.
2. The SA Debuggee process spawned by the test (LingeredApp)
3. The SA "tool", also spawned by the test.

(1) and (2) will automatically get the default test args, which will include -Xcomp if it is a -Xcomp test task. (3) will not get the default test args since it is spawned with createLimitedTestJavaProcessBuilder(), which is likely what we want. The only process that needs to be run with -Xcomp (and any other test args) is (2) the debuggee process, but I don't see why we don't just allow it to get -Xcomp when run as part of a -Xcomp test task, and not have -Xcomp otherwise.

It doesn't appear that -Xcomp on the debuggee is a requirement for this test, because in the past it has not been explicitly run with -Xcomp. The changes in the PR get rid of test runs where the debuggee does not use -Xcmp, and also continue to force -Xcomp on the test process, which is unnecessary.

If we really only want the debuggee run with -Xcomp, then we should probably limit this test to only support -Xcomp test runs. So that means adding @requires vm.compMode == "Xcomp". It also means getting rid of -Xcomp elsewhere in the test. I'm not sure of the reason for -Xmx1g. Maybe @tkrodriguez can explain.

I think another options is to have the test explicitly launch the debuggee with -Xcomp as is done in this PR, but also add @requires vm.flagless. @lmesnik?

[tkrodriguez]

The -Xmx1g is probably just copied from ClhsdbPrintAll.java so it could probably be dropped. Since the point of this test is to exercise nmethod debug info decoding we want LingeredApp to run with Xcomp so that the inspected process has nmethods. It doesn't matter if anything else is run with Xcomp so whatever accomplishes that seems fine.

[lmesnik]

The only thing that is missed it change othervm to driver.

The adding -Xcomp in LingeredApp.startApp(theApp, "-Xmx1g", "-Xcomp");
should works fine except when -Xint/-Xmixed is set explicitly.
So generally test like needs to have
@requires vm.compMode == "null" | vm.compMode == "Xcomp"
However we don't set these modes, so it might be skipped. A lot of our test set Xcomp without any requires now.
Also, the vm.flagless is not required, since test support various vm flags.
Limiting tests to run only -Xcomp is set is also not needed for this test.

The another thing to check (unrelated to the fix) is if test is executed in tier1 and should it be there.

[lmesnik]

Please check that test pass with options from CI.

[rsunderbabu]

/integrate

[openjdk]

@rsunderbabu
Your change (at version 7b3c884) is now ready to be sponsored by a Committer.

[lmesnik]

/sponsor

[openjdk]

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

• 363327e: 8341452: Test runtime/cds/appcds/DumpRuntimeClassesTest.java from JDK-8324259 is failing
• d6f8b46: 8340389: vmTestbase/gc/gctests/PhantomReference/phantom001/TestDescription.java Test exit code: 97 with -Xcomp UseAVX=3
• 8e16e67: 8342329: G1: Rename G1HeapRegionManager::_allocated_heapregions_length
• 3ebfc6c: 8342336: Optimize ClassFile imports
• 7ff4ea8: 8341966: Broken annotated module may lead to an exception in javac
• 9bdface: 8341622: Tag-specific disabled default decorators for UnifiedLogging
• 1ea1f33: 8340336: Open some checkbox awt tests
• 7a64fbb: 8338851: Hoist os::Posix::realpath() to os::realpath() and implement on Windows
• 8862ca0: 8342338: Remove redundant IIOPURLTest.java
• f9208fa: 8341715: PPC64: ObjectMonitor::_owner should be reset unconditionally in nmethod unlocking
• ... and 47 more: https://git.openjdk.org/jdk/compare/a601cd2e100958e3f37ae65e32e4b3cac246c079...master

Your commit was automatically rebased without conflicts.

[openjdk]

@lmesnik @rsunderbabu Pushed as commit d915ac2.

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