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

Wed Mar 14 16:29:48 UTC 2018

Thanks for adding the much needed comments. How about adding an example of what a “typical”, actual formatted string looks like?

Cheers,
Mikael

> On Mar 14, 2018, at 1:34 AM, Schmidt, Lutz <lutz.schmidt at sap.com> wrote:
> 
> Hi David,
> sorry this took so long. Gaining insight into what happened and getting rid of the effects was cumbersome. Here is what I achieved:
> 
> The ResourceMark issue was embarrassingly easy to resolve. I just removed the "detour" via stringStream. 
> 
> The failing test suffered from three facts:
> #1 (that's my "fault"): I changed the print layout (omitting whitespace) in cases when the expected field width is exceeded. 
> #2 (test inherent): the regex used to parse each print_flag() line requires whitespace at places where they are not really needed.
> #3 (test inherent): Some tests contain an exact copy of what they expect as output as string literal. Those literals had to be adapted. 
> 
> I have adjusted the layout such that the regex matches ok. What remains is a "bad gutt feeling" because
> - the regex is very specific with respect to the print layout. Minor changes may cause it to break (again).
> - the test, though being a jvmci test, verifies a locally assumed format, for all flags. Is that the right place?
> - I have no insight into why this verification is necessary. Is there another place relying on the verified format?
> - I regard relying on an exact char by char layout of the test output as sub-par testing style. 
> 
> Anyway, there is a new webrev at http://cr.openjdk.java.net/~lucy/webrevs/8198608.01/ 
> The bug can be found here: https://bugs.openjdk.java.net/browse/JDK-8198608
> 
> It has undergone all testing we run @SAP. No issues were detected. May I please request you to have another look? Of course, anybody else is welcome to comment as well!
> 
> Thank you,
> Lutz
> 
> On 03.03.18, 18:04, "Schmidt, Lutz" <lutz.schmidt at sap.com> wrote:
> 
>    Hi David, 
> 
>    I guess I have to apologize! There must be something wrong with my test coverage.
> 
>    As of now, I don't have an idea why a flag value would not be printed correctly.
> 
>    I will look into this, and the ResourceMark issue as well,  asap.
> 
>    Regards,
>    Lutz
> 
>    On 03.03.18, 10:30, "David Holmes" <david.holmes at oracle.com> wrote:
> 
>        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
>>>>> 
>>>> 
>>>> 
>>> 
> 
> 
> 
> 