8332287: When printing arguments in error logs, quote arguments with whitespaces

[tstuefe]

Trivial fix to a simple issue.

In hs-err files, when printing the arguments of the crashed JVM process, print arguments with whitespaces in them with quotes. Makes it easier to copy-paste them to terminals.

Before:

Command Line: -Dtest.vm.opts=-XX:+UnlockDiagnosticVMOptions -XX:-TieredCompilation -XX:+StressArrayCopyMacroNode -XX:+StressLCM -XX:+StressGCM -XX:+StressIGVN -XX:+StressCCP -XX:+StressMacroExpansion -XX:+StressMethodHandleLinkerInlining -XX:+StressCompiledExceptionHandlers -XX:CompileCommand=memlimit,*.*,1G~crash -XX:CompileCommand=memstat,*::*,print -Dtest.tool.vm.opts=-J-XX:+UnlockDiagnosticVMOptions -J-XX:-TieredCompilation -J-XX:+StressArrayCopyMacroNode -J-XX:+StressLCM -J-XX:+StressGCM -J-XX:+StressIGVN -J-XX:+StressCCP -J-XX:+StressMacroExpansion -J-XX:+StressMethodHandleLinkerInlining -J-XX:+StressCompiledExceptionHandlers -J-XX:CompileCommand=memlimit,*.*,1G~crash -J-XX:CompileCommand=memstat,*::*,print -XX:+StressCompiledExceptionHandlers -XX:CompileCommand=memlimit,*.*,1G~crash -XX:CompileCommand=memstat,*::*,print -Djava.library.path=/shared/projects/openjdk/jdk-jdk/output-fastdebug/images/test/hotspot/jtreg/native -Xbatch -XX:CompileCommand=option,*::*,bool,Vectorize,true -XX:+PrintOpto -XX:+TraceLoopOpts com.sun.javatest.regtest.agent.MainWrapper /Users/thomas/shared/projects/openjdk/jtreg-runs/jtreg/JTwork/compiler/c2/TestFindNode.d/main.0.jta

Now (notice the gigantic -Dtest.vm.opts and -Dtest.tool.vm.opts that are actually just one large argument each)

Command Line: "-Dtest.vm.opts=-XX:+UnlockDiagnosticVMOptions -XX:-TieredCompilation -XX:+StressArrayCopyMacroNode -XX:+StressLCM -XX:+StressGCM -XX:+StressIGVN -XX:+StressCCP -XX:+StressMacroExpansion -XX:+StressMethodHandleLinkerInlining -XX:+StressCompiledExceptionHandlers -XX:CompileCommand=memlimit,*.*,1G~crash -XX:CompileCommand=memstat,*::*,print" "-Dtest.tool.vm.opts=-J-XX:+UnlockDiagnosticVMOptions -J-XX:-TieredCompilation -J-XX:+StressArrayCopyMacroNode -J-XX:+StressLCM -J-XX:+StressGCM -J-XX:+StressIGVN -J-XX:+StressCCP -J-XX:+StressMacroExpansion -J-XX:+StressMethodHandleLinkerInlining -J-XX:+StressCompiledExceptionHandlers -J-XX:CompileCommand=memlimit,*.*,1G~crash -J-XX:CompileCommand=memstat,*::*,print" -XX:+StressCompiledExceptionHandlers -XX:CompileCommand=memlimit,*.*,1G~crash -XX:CompileCommand=memstat,*::*,print -Djava.library.path=/shared/projects/openjdk/jdk-jdk/output-fastdebug/images/test/hotspot/jtreg/native -Xbatch -XX:CompileCommand=option,*::*,bool,Vectorize,true -XX:+PrintOpto -XX:+TraceLoopOpts com.sun.javatest.regtest.agent.MainWrapper /Users/thomas/shared/projects/openjdk/jtreg-runs/jtreg/JTwork/compiler/c2/TestFindNode.d/main.0.jta

---

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-8332287: When printing arguments in error logs, quote arguments with whitespaces (Enhancement - P4)

Reviewers

• Johan Sjölen (@jdksjolen - Reviewer) ⚠️ Review applies to 72343272

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19248

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper]

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

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

8332287: When printing arguments in error logs, quote arguments with whitespaces

Reviewed-by: jsjolen

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

• da6aa2a: 8332849: Update doc/testing.{md,html} (spelling and stale information)
• b8f2ec9: 8195675: Call to insertText with single character from custom Input Method ignored
• 0f3e2cc: 8331670: Deprecate the Memory-Access Methods in sun.misc.Unsafe for Removal
• 51ae08f: 8333093: Incorrect comment in zAddress_aarch64.cpp
• 4754f05: 8333035: Parallel: Remove ParMarkBitMap::IterationStatus
• 87a06b6: 8325805: Compiler Implementation for Flexible Constructor Bodies (Second Preview)
• e708d13: 8332064: Implementation of Structured Concurrency (Third Preview)
• 7b52d0a: 8332265: RISC-V: Materialize pointers faster by using a temp register
• aa4c83a: 8332505: JEP 457: ClassRemapper forgets to remap bootstrap method references
• cabe337: 8331921: Hotspot assembler files should use common logic to setup exported functions
• ... and 158 more: https://git.openjdk.org/jdk/compare/1a944478a26a766f5a573a1236b642d8e7b0685c...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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[openjdk]

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

• hotspot-runtime

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

• 02: Full - Incremental (1d23376d)
• 01: Full - Incremental (72343272)
• 00: Full (83e17e51)

[afshin-zafari]

Based on a recommendation from CPPReference, the argument of isspace should be casted to unsigned char.

[tstuefe]

ok, done.

[kevinjwalls]

Are we saying we actually ran with, e.g. -Dtest.vm.opts="1 2 3"
and we curently report in e.g. VM.info or hs_err as:

Command Line: -Dtest.vm.opts=1 2 3 AppName
and:
jvm_args: -Dtest.vm.opts=1 2 3

But do we really want to report it with -Dtest.vm.opts itself inside the quotes - to make the copy/paste usable, we need to recognise -Dname=value args containing spaces, and add the first quote after the = sign?

The scheme here would work for args to the app, e.g. java -Dtest.vm.opts="1 2 3" MyApp "one arg with spaces"

Currently reported as:
Command Line: -Dtest.vm.opts=1 2 3 MyApp one arg with spaces

..where the args to the Java app are misleading. However, if the arg to the Java app is:

java MyApp arg="arg with spaces"

..then if we simply quote that one argument it may be clearer to humans, but may not be directly usable in a copy/paste.

[tstuefe]

Hi Kevin,

Are we saying we actually ran with, e.g. -Dtest.vm.opts="1 2 3" and we curently report in e.g. VM.info or hs_err as:

Command Line: -Dtest.vm.opts=1 2 3 AppName and: jvm_args: -Dtest.vm.opts=1 2 3

yes

But do we really want to report it with -Dtest.vm.opts itself inside the quotes - to make the copy/paste usable, we need to recognise -Dname=value args containing spaces, and add the first quote after the = sign?

No, its usable in its current form.

When copy-pasted into a terminal running bash, A="1 2 3" and "A=1 2 3" will expand to the same string "A=1 2 3".

The scheme here would work for args to the app, e.g. java -Dtest.vm.opts="1 2 3" MyApp "one arg with spaces"

Currently reported as: Command Line: -Dtest.vm.opts=1 2 3 MyApp one arg with spaces

..where the args to the Java app are misleading. However, if the arg to the Java app is:

java MyApp arg="arg with spaces"

..then if we simply quote that one argument it may be clearer to humans, but may not be directly usable in a copy/paste.

I don't like the hs-err file to be too smart about this :)

I would like to see the real argument vector, not some interpreted form of it. If something is given as a single argument containing spaces, that argument should be quoted. For that it does not matter if the arguments itself is later interpreted by the JVM or one of its daughter processes as a variable assignment.

[jdksjolen]

LGTM and feature seems reasonable.

[jdksjolen]

Please expand the check for *p to *p != '\0'.

[kevinjwalls]

OK, yes I can expect for the JVM arguments, it doesn't matter if the JVM sees -Dfoo="1 2 3 " or "-Dfoo=1 2 3 "

I just thought that if we report the quotes in a different place to the original, that we should at least acknowledge that difference somewhere in the change/bug report. I was exactly noticing that this is an interpreted form of the arg.

(But yes it should be harmless, and no I don't want to suggest we should do anything much more clever here right now.)

I think -XX args work this way too, they don't mind the quotes moving.

[tstuefe]

OK, yes I can expect for the JVM arguments, it doesn't matter if the JVM sees -Dfoo="1 2 3 " or "-Dfoo=1 2 3 "

I just thought that if we report the quotes in a different place to the original, that we should at least acknowledge that difference somewhere in the change/bug report. I was exactly noticing that this is an interpreted form of the arg.

But how would I know the original? The JVM does not know if you called -Dvm.args="1 2 3" or "-Dvm.args=1 2 3" . In fact, I usually do the latter.

[kevinjwalls]

But how would I know the original?

I don't think you can. 8-) That may be why it's currently done as it is, if the quote placement is lost we just can't put it back and know it was correct.

This next one may be a bit artificial, but we should also at least acknowledge the case where an argument has an embedded quote character, e.g. escaped with a backslash before I type it. There might be an XX or other option where that could apply at some point. You don't get a String you can paste, unless we escape quotes when printing in print_jvm_args_on(). And we don't want to make that much more complicated. We can either print the escape \ before any quotes, or just note in the bug that this is not always a trivial problem, although it is trivial for many cases.

[tstuefe]

But how would I know the original?

I don't think you can. 8-) That may be why it's currently done as it is, if the quote placement is lost we just can't put it back and know it was correct.

This next one may be a bit artificial, but we should also at least acknowledge the case where an argument has an embedded quote character, e.g. escaped with a backslash before I type it. There might be an XX or other option where that could apply at some point. You don't get a String you can paste, unless we escape quotes when printing in print_jvm_args_on(). And we don't want to make that much more complicated. We can either print the escape \ before any quotes, or just note in the bug that this is not always a trivial problem, although it is trivial for many cases.

How deep down the rabbit hole do we go? Escape both single- and double-quotes? What if they are already escaped?

Tbh, I would have liked this to be a small patch without burning too much time on it. I think what you describe is a rare outlier case (arguments that contain both whitespaces and quotes). Wheras the spaces-in-arguments is something that regularly occurs during many jtreg tests.

[kevinjwalls]

Hey @tstuefe Thomas --

I don't mean to stop this happening, and I don't want you or anybody to burn time on it too much, just wanted to note that quoting and trivial are not words that usually go together, and we have winners and losers. 8-)

We want to simply copy paste all the JVM arguments to rerun the same arguments and reproduce an issue.

With this change: don't worry if an option has embedded spaces, but look carefully if an option contains a quote.

e.g.
a test using -Dtest.vm.opts= which contains space(s) to separate an option list.
an app running with -Dquote=" like maybe a text-processing tool.

With mixed feelings about deepening the rabbit hole, if this is triggered by reproducing failures from jtreg tests, then the result .jtr file contains "rerun:" information, with single-quoted args like:
-Dtest.vm.opts='-Xmx768m -XX:MaxRAMPercentage=1.78571 ...etc...'

...so while I realise somebody may only send you an hs_err file, the full script to rerun should be available somewhere.

If you're reproducing a crash, is it the process that had the -Dtest.vm.opts... that crashed and gave you an hs_err file, or is it the thing which that launched? Running with -Dtest.vm.opts suggests you might be looking at the com.sun.javatest.regtest.agent.MainWrapper process and that sounds unusual.

I really want to leave this alone and let you and others decide if this is a net benefit. I think the JBS bug should contain some note on the trade-off.