RFR: 8332287: When printing arguments in error logs, quote arguments with whitespaces [v2]

[Kevin Walls]

On Sat, 18 May 2024 05:17:18 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>>> 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.
>
>> > 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.

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.

-------------

PR Comment: https://git.openjdk.org/jdk/pull/19248#issuecomment-2120230954