8304834: Fix wrapper insertion in TestScaffold.parseArgs(String args[]) #13170

lmesnik · 2023-03-23T21:49:55Z

The TestScaffold incorrectly parse options, it should insert wrapper class between VM options and applications classame.

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-8304834: Fix wrapper insertion in TestScaffold.parseArgs(String args[])

Reviewers

Chris Plummer (@plummercj - Reviewer)
David Holmes (@dholmes-ora - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 13170

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2023-03-23T21:51:07Z

👋 Welcome back lmesnik! 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 · 2023-03-23T21:54:17Z

@lmesnik 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 · 2023-03-23T21:58:07Z

Webrevs

dholmes-ora · 2023-03-24T04:39:19Z

Sorry I'm struggling a bit to see where the current parsing logic is failing. Can you given an example of a command-line that will get processed incorrectly? I'm not sure why the generic "if it starts with - then add it to vmargs" is needed as any option that starts with - should either be one known by the framework and explicitly handled in the parsing, or else be a -J-XXX flag to pass to the VM. So AFAICS the existing logic will just append anything it doesn't recognise to the app command line - and that seem the right thing to do.

lmesnik · 2023-03-24T06:27:02Z

I've added comment with example from one of tests and expected result of parsing.
The problem was that wrapper inserted it's arguments before vm options because didn't expect them in the args.

plummercj

Changes look good. I was hoping it would fix more of the failing tests than it did. I get we'll need to take a closer look at them. Would be good to eventually diagnose the root cause of the failures and get bugs filed for each category of failure.

plummercj · 2023-03-24T21:33:10Z

test/jdk/com/sun/jdi/TestScaffold.java

+        if (mainWrapper != null && !argInfo.targetAppCommandLine.isEmpty()) {
+            argInfo.targetAppCommandLine = TestScaffold.class.getName() + ' '
+                    + mainWrapper + ' ' + argInfo.targetAppCommandLine;
+            argInfo.targetVMArgs += "--enable-preview ";


It looks like previously we ignored main.wrapper if not set to Virtual, but with your chagnes we accept any setting. That's ok, but --enable-preview is really on needed when set to Virtual.

openjdk · 2023-03-24T21:37:36Z

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

8304834: Fix wrapper insertion in TestScaffold.parseArgs(String args[])

Reviewed-by: cjplummer, dholmes

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

82e8b03: 8305203: Simplify trimming operation in Region::Ideal
27cf638: 8300912: Update java/nio/MappedByteBuffer/PmemTest.java to run on x86_64 only
42fa000: 8305484: Compiler::init_c1_runtime unnecessarily uses an Arena that lives for the lifetime of the process
475e9a7: 8305809: (fs) Review obsolete Linux kernel dependency on os.version (Unix kernel 2.6.39)

Please see this link for an up-to-date comparison between the source branch of this pull request and 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.

lmesnik · 2023-03-24T21:44:59Z

The non Virtual modes are Kernel (should be Platform) and None. They are mostly used to find id test fail because of wrapper or virtual threads. Not used in testing.

dholmes-ora · 2023-03-27T05:09:55Z

test/jdk/com/sun/jdi/TestScaffold.java

+        // Parse arguments, like java/j* tools command-line arguments
+        // the first argument not-starting with '-' is treated as a classname
+        // the other arguments are split to targetVMArgs targetAppCommandLine correspondingly
+        // The example of args for line '@run driver Frames2Test -Xss4M' is  '-Xss4M' 'Frames2Targ'.


Is Frames2Test a typo there?

Frames2Test is the name of test that uses additional command-line options. It is an example.

In that case did you mean:

'@run driver Frames2Test -Xss4M Frames2Targ'

?

the @run line contains only the test name and additional command-line options if needed, the target app class 'Frames2Targ' is not included. I have copy-pasted this example from

jdk/test/jdk/com/sun/jdi/Frames2Test.java

Line 34 in 0deb648

* @run driver Frames2Test -Xss4M

That is very confusing because the Frames2Targ seems to appear out of nowhere.

Well, I could update the comment, but it is basically how the all jdi tests work. The test adds target by itself after all command line arguments.

dholmes-ora · 2023-03-27T05:13:21Z

The problem was that wrapper inserted it's arguments before vm options because didn't expect them in the args.

Sorry still not sure what you mean by that. Do you mean the --enable-preview arg? That was appended to VM args:

argInfo.targetVMArgs += "--enable-preview ";

which seems perfectly fine.

lmesnik · 2023-03-27T05:50:45Z

before the fix, the parse args incorrectly compos argInfo, putting all args into targetAppCommandLine, including VM args. So the result for
-Xss4M Frames2Targ'
was
argInfo.targetAppCommandLine : -Xss4M Frames2Targ
argInfo.targetVMArgs :
without wrapper.
Which is not very correct but didn't cause failures. But becomes a problem when the wrapper tries to insert a new class.
The old command;
argInfo.targetAppCommandLine : TestScaffold Virtual -Xss4M Frames2Targ
argInfo.targetVMArgs : --enable-preview
The new commands:
argInfo.targetAppCommandLine : TestScaffold Virtual Frames2Targ
argInfo.targetVMArgs : -Xss4M --enable-preview

dholmes-ora · 2023-03-27T05:55:52Z

Isn't the -Xss4M supposed to be passed as -J-Xss4M? That is what the original logic is expecting.

lmesnik · 2023-03-27T18:25:52Z

Isn't the -Xss4M supposed to be passed as -J-Xss4M? That is what the original logic is expecting.

I haven't seen that -J-.. is used here in the args, all tests just use vm flags directly. I am not sure if updating tests is the correct option here.

dholmes-ora · 2023-03-28T04:34:31Z

Isn't the -Xss4M supposed to be passed as -J-Xss4M? That is what the original logic is expecting.

I haven't seen that -J-.. is used here in the args, all tests just use vm flags directly. I am not sure if updating tests is the correct option here.

Well whomever wrote the original parsing code expected that VM args would be passed using -J AFAICS. And if handling of non -J VM args is currently broken (per this PR) then how are those other tests working?

Also note that if a test tries to set the classpath without using the -J-classpath <path> form then your code will not work correctly as the <path> would be treated as the class name.

dholmes-ora · 2023-03-28T04:43:12Z

Also note that both the old code and the new code won't correctly deal with other VM options that take separate arguments like classpath does e.g --add-exports <export-spec>

lmesnik · 2023-03-28T04:59:48Z

Yes, you are right. The ability to process options is very limited. Probably would be better to get rid of setting VM flags using driver test parameters completely. Let me check if it could be implemented.

lmesnik · 2023-04-07T19:00:15Z

I have updated parsing to don't accept -J which has been used in the one test only. Also improved parsing to recognize double-argument options.
Also, I provided statistic of current options in the bug.

dholmes-ora

A small nit but otherwise the new approach to parsing args now seems complete/consistent.

Thanks.

test/jdk/com/sun/jdi/TestScaffold.java

dholmes-ora

Looks good. Thanks

lmesnik · 2023-04-12T17:42:36Z

/integrate

openjdk · 2023-04-12T17:44:50Z

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

19380d7: 8305324: C2: Wrong execution of vectorizing Interger.reverseBytes
87017b5: 8295859: Update Manual Test Groups
99a9dbc: 8305783: x86_64: Optimize AbsI and AbsL
d8af7a6: 8304725: AsyncGetCallTrace can cause SIGBUS on M1
b9bdbe9: 8305524: AArch64: Fix arraycopy issue on SVE caused by matching rule vmask_gen_sub
82e8b03: 8305203: Simplify trimming operation in Region::Ideal
27cf638: 8300912: Update java/nio/MappedByteBuffer/PmemTest.java to run on x86_64 only
42fa000: 8305484: Compiler::init_c1_runtime unnecessarily uses an Arena that lives for the lifetime of the process
475e9a7: 8305809: (fs) Review obsolete Linux kernel dependency on os.version (Unix kernel 2.6.39)

Your commit was automatically rebased without conflicts.

openjdk · 2023-04-12T17:45:02Z

@lmesnik Pushed as commit bc15163.

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

lmesnik added 4 commits March 23, 2023 17:36

fix

03a3209

comments

c57a1b2

problemlist fixed

4865cb2

parsing updted

e7c5e99

openjdk bot added the rfr Pull request is ready for review label Mar 23, 2023

openjdk bot added the serviceability serviceability-dev@openjdk.org label Mar 23, 2023

added comments and trim arguments

1b74ae2

plummercj approved these changes Mar 24, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 24, 2023

dholmes-ora reviewed Mar 27, 2023

View reviewed changes

updated parsing.

bc8dd5e

dholmes-ora reviewed Apr 11, 2023

View reviewed changes

test/jdk/com/sun/jdi/TestScaffold.java Outdated Show resolved Hide resolved

test/jdk/com/sun/jdi/TestScaffold.java Outdated Show resolved Hide resolved

lmesnik added 2 commits April 11, 2023 10:48

Merge branch 'master' of https://github.com/openjdk/jdk into 8289546

cf5b38b

updated after David's comments.

e0a29a7

plummercj suggested changes Apr 11, 2023

View reviewed changes

openjdk bot removed the ready Pull request is ready to be integrated label Apr 11, 2023

Updated comments

17f81d2

plummercj approved these changes Apr 11, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 11, 2023

dholmes-ora approved these changes Apr 12, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Apr 12, 2023

openjdk bot closed this Apr 12, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Apr 12, 2023

8304834: Fix wrapper insertion in TestScaffold.parseArgs(String args[]) #13170

8304834: Fix wrapper insertion in TestScaffold.parseArgs(String args[]) #13170

Uh oh!

Conversation

lmesnik commented Mar 23, 2023 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Mar 23, 2023

Uh oh!

openjdk bot commented Mar 23, 2023

Uh oh!

mlbridge bot commented Mar 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

dholmes-ora commented Mar 24, 2023

Uh oh!

lmesnik commented Mar 24, 2023

Uh oh!

plummercj left a comment

Choose a reason for hiding this comment

Uh oh!

plummercj Mar 24, 2023

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Mar 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lmesnik commented Mar 24, 2023

Uh oh!

dholmes-ora Mar 27, 2023

Choose a reason for hiding this comment

Uh oh!

lmesnik Mar 27, 2023

Choose a reason for hiding this comment

Uh oh!

dholmes-ora Mar 27, 2023

Choose a reason for hiding this comment

Uh oh!

lmesnik Mar 27, 2023

Choose a reason for hiding this comment

Uh oh!

dholmes-ora Mar 28, 2023

Choose a reason for hiding this comment

Uh oh!

lmesnik Mar 28, 2023

Choose a reason for hiding this comment

Uh oh!

dholmes-ora commented Mar 27, 2023

Uh oh!

lmesnik commented Mar 27, 2023

Uh oh!

dholmes-ora commented Mar 27, 2023

Uh oh!

lmesnik commented Mar 27, 2023

Uh oh!

dholmes-ora commented Mar 28, 2023

Uh oh!

dholmes-ora commented Mar 28, 2023

Uh oh!

lmesnik commented Mar 28, 2023

Uh oh!

lmesnik commented Apr 7, 2023

Uh oh!

dholmes-ora left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dholmes-ora left a comment

Choose a reason for hiding this comment

lmesnik commented Mar 23, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Mar 23, 2023 •

edited

Loading

openjdk bot commented Mar 24, 2023 •

edited

Loading