RFR(S): 8198608: Improvements to command-line flags printing

[David Holmes]

There are also test failures:

compiler/jvmci/compilerToVM/GetFlagValueTest

java.lang.RuntimeException: Unexpected line in -XX:+PrintFlagsFinal 
output: bool BootstrapJVMCI = false {JVMCI experimental}{default}: 
expected true, was false

Can you please ensure you're checked for all tests that use PrintFlags 
or PrintFlagsFinal and verify that they run okay in both product and 
fastdebug. I see the failures on multiple platforms but linux-x64 is one 
so you should be able to test that one easily.

Thanks,
David

On 3/03/2018 2:44 PM, David Holmes wrote:
> I'll sponsor this.
> 
> I don't see Goetz's email to the list but will take it as per your 
> response.
> 
> Cheers,
> David
> 
> On 3/03/2018 1:04 AM, Schmidt, Lutz wrote:
>> Hi David,
>>
>> it would be great if you could sponsor this change. I was able to 
>> successfully test on darwinintel64, linuxs390x, linuxppc64, and 
>> linuxx86_64. Our AIX systems are not playing nice with me at the 
>> moment (no issues with the change, just general misbehavior).
>>
>> I have modified line 551 according to your suggestion (and line 519 as 
>> well). Webrev updated in-place.
>>
>> So let's hope for a second review over the weekend.
>>
>> Regards,
>> Lutz
>>
>>
>> On 02.03.18, 02:19, "David Holmes" <david.holmes at oracle.com> wrote:
>>
>>      On 1/03/2018 11:25 PM, Schmidt, Lutz wrote:
>>      > Hi David,
>>      > thank you for looking at this. You are right, the comment is a 
>> useless leftover -> removed in if and else branch.
>>      Looks fine. Just need a second reviewer. Do you need a sponsor to 
>> test
>>      on additional platforms? Otherwise what platforms have you tested?
>>      > With the "\n" handling, I believe we are on the safe side. If a 
>> newline character is detected in the parameter string, it is replaced 
>> by a st->cr() call. That call does the expected on any platform, I 
>> would hope. Flag::print_as_flag() (not in the scope of the change) 
>> uses a similar handling.
>>      >
>>      > The newlines are contained in string literals in C code (e.g. 
>> default values for parameters) or stem from ccstrlist concatenations. 
>> That is all under VM control. So I do not see a risk here. You can try 
>> yourself on any platform with the -XX:DisableIntrinsic=test1 parameter 
>> multiple times.
>>      >
>>      > If a user manages to specify a parameter string with platform 
>> (windows) specific line terminators and hopes for correct (\n-like) 
>> handling, he or she will be disappointed. I would assume the 
>> PrintFlags formatting isn't the only place that's impacted.
>>      Sorry I mistakenly thought you had modified the newline handling, 
>> when
>>      you hadn't. If there is an issue it would be preexisting. I was
>>      wondering how you would get a multi-line ccstr value. If you 
>> entered it
>>      on the command-line e.g:
>>      java -XX:OnError="Line 1
>>      Line2"
>>      then I would expect to find the platform line separator within the
>>      string. In testing this with the existing PrintFlagsFinal Linux 
>> does:
>>      ccstrlist OnError                                  = Line 1
>>             OnError                             += Line 2
>>                     {product} {command line}
>>      but testing on Windows is a problem. The regular cmd shell can't 
>> take
>>      multi-line arguments. If you use the ^ escape trick it actually 
>> strips
>>      the newline and passes the arg as one line. So I guess the issue is
>>      somewhat moot. :)
>>      One further nit:
>>        551           st->print("%s", "+=");
>>      should just be:
>>        551           st->print("+=");
>>      Thanks,
>>      David
>>      > I have updated the webrev in-place with the comments removed.
>>      >
>>      > Thanks again, Lutz
>>      >
>>      >
>>      > On 28.02.18, 23:26, "David Holmes" <david.holmes at oracle.com> 
>> wrote:
>>      >
>>      >      Hi Lutz,
>>      >
>>      >      On 24/02/2018 2:48 AM, Schmidt, Lutz wrote:
>>      >      > Dear all,
>>      >      >
>>      >      > may I please request reviews for this small enhancement:
>>      >      >
>>      >      > Bug:     https://bugs.openjdk.java.net/browse/JDK-8198608
>>      >      > Webrev:  
>> http://cr.openjdk.java.net/~lucy/webrevs/8198608.00/
>>      >      >
>>      >      > The code in Flag::print_on() so far wasn’t very easy to 
>> understand. Changing the layout of what was printed required some deep 
>> thinking. I hope that, with my changes, future modifications will be 
>> easier.
>>      >      >
>>      >      > The before/after output of -XX:+PrintFlagsFinal is 
>> identical, except for those argument names which are longer than 
>> expected. In that case, the new version prints one space less, which 
>> is by intention.
>>      >
>>      >      This all seems okay - and easier to modify further if needed.
>>      >
>>      >      Two minor comments:
>>      >
>>      >        576     // Flag::print_on(...) redesign (!print_ranges)
>>      >
>>      >      Isn't this the print_ranges case? But in any case not sure 
>> a comment
>>      >      with "redesign" in it is that meaningful given you can't 
>> see the old design.
>>      >
>>      >      Does the ccstr newline handling work on all platforms (ie 
>> Windows) - I'm
>>      >      never sure when it suffices to check for '\n' and when we 
>> have to check
>>      >      for the platform specific line terminators.
>>      >
>>      >      Thanks,
>>      >      David
>>      >
>>      >      > Thank you!
>>      >      > Lutz
>>      >      >
>>      >      >
>>      >      >
>>      >      > Dr. Lutz Schmidt | SAP JVM | PI  SAP CP Core | T: +49 
>> (6227) 7-42834
>>      >      >
>>      >
>>      >
>>