[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-runtime-dev
Subject:    Re: RFR (S): 8139801: Error message from validation check has wrong order on Windows
From:       "sangheon.kim" <sangheon.kim () oracle ! com>
Date:       2015-10-26 15:25:44
Message-ID: 562E45F8.1070604 () oracle ! com
[Download RAW message or body]

Hi Coleen,

Thank you for reviewing this.

Sangheon


On 10/23/2015 05:36 PM, Coleen Phillimore wrote:
> Sangheon, this change looks good.
> Coleen
>
> On 10/23/15 6:57 PM, sangheon.kim wrote:
>> Hi all,
>>
>> I'm waiting some reviews from (R)eviewer.
>> And here's a new webrev which includes Chris' comments.
>> - Move fflush() outside of 'if' statement at jni.cpp.
>> - Remove #ifdef _WIN32
>>
>> webrev:
>> http://cr.openjdk.java.net/~sangheki/8139801/webrev.02/
>>
>> Thanks,
>> Sangheon
>>
>>
>> On 10/20/2015 05:17 PM, sangheon.kim wrote:
>>> Hi Chris,
>>>
>>> Thank you for reviewing this.
>>> My answers are below.
>>>
>>> On 10/20/2015 04:35 PM, Chris Plummer wrote:
>>>> Hi Sangheon,
>>>>
>>>> Thanks for doing this. A few comments/suggestions:
>>>>
>>>> I don't think the #ifdef __WIN32__ should be needed. Although we 
>>>> are doing the fflush mainly to address issues we only currently see 
>>>> on win32, there's no reason why the fflush can't be done on all 
>>>> platforms. I think that will help keep the code cleaner.
>>> From cleaner code point of view, I totally agree with you.
>>> Let me hear others opinion on this.
>>>
>>>>
>>>> In jni.cpp, is there a reason not to flush unconditionally (outside 
>>>> the if/else)?
>>> Hmm.
>>> I thought we will not have strings to be flushed from the 
>>> create_vm() success case.
>>> But I think I have to move outside of 'if statement'.
>>>
>>>>
>>>> Please make sure your commit comment references all 3 CRs this fix 
>>>> is addressing:
>>>>
>>>> 8139374 TlabSize.java argument test fails
>>>> 8078328 AppCDS jtreg test result missing expected error message in 
>>>> stderr
>>>> 8139801 Error message from validation check has wrong order on Windows
>>> Okay.
>>>
>>> Thanks,
>>> Sangheon
>>>
>>>
>>>>
>>>> I'm not a "R"eviewer.
>>>>
>>>> thanks,
>>>>
>>>> Chris
>>>>
>>>> On 10/20/15 3:03 PM, sangheon.kim wrote:
>>>>> Hi all,
>>>>>
>>>>> Initially I only tested validation error messages but later I 
>>>>> noticed that other error messages also have same problem on 
>>>>> Windows. (thanks to Chris Plummer for the discussion)
>>>>>
>>>>> When we fail to create VM, we end with 1) returning to the caller 
>>>>> or 2) call vm_abort().
>>>>> For the 1) case, all error messages have wrong order on Windows.
>>>>>
>>>>> eg) an error message on Windows. (this is NG)
>>>>> $ java -XX:VMOptionsFile=foo -XX:VMOptionsFile=bar
>>>>> Error: Could not create the Java Virtual Machine.
>>>>> Error: A fatal exception has occurred. Program will exit.
>>>>> Only one VM Options file is supported on the command line <- this 
>>>>> should be printed first.
>>>>>
>>>>> We can see easily this wrong order on Windows but the problem is 
>>>>> there's no guarantee that 'stdout/stderr' will be flushed on that 
>>>>> situation.
>>>>>
>>>>> As a solution I added fflush(stdout) and fflush(stderr) for above 
>>>>> two cases.
>>>>>
>>>>> CR: https://bugs.openjdk.java.net/browse/JDK-8139801
>>>>> Webrev: http://cr.openjdk.java.net/%7Esangheki/8139801/webrev.01/
>>>>> Testing: JPRT
>>>>>
>>>>> Thanks,
>>>>> Sangheon
>>>>>
>>>>>
>>>>> On 10/19/2015 01:00 PM, sangheon.kim wrote:
>>>>>> Hi all again,
>>>>>>
>>>>>> Let me cancel this patch as this kind of problem seems general on 
>>>>>> Windows.
>>>>>>
>>>>>> Thanks,
>>>>>> Sangheon
>>>>>>
>>>>>>
>>>>>> On 10/19/2015 08:56 AM, sangheon.kim wrote:
>>>>>>> Hi all,
>>>>>>>
>>>>>>> Can I get some reviews for this change of adding 'fflush()' on 
>>>>>>> validation message print?
>>>>>>>
>>>>>>> The order of 'validation error message' and 'vm exit message' is 
>>>>>>> wrong on Windows.
>>>>>>> - Windows is buffering the validation error message which is 
>>>>>>> sent to 'stderr'.
>>>>>>> - 'stderr' from 'jvm.dll' is flushed later than 'stderr' from 
>>>>>>> the caller of 'jvm.dll'.
>>>>>>>
>>>>>>> eg) Expected message order (all other platforms except Windows)
>>>>>>> $ java -XX:MinTLABSize=1 -version
>>>>>>> 1) MinTLABSize (1) must be greater than or equal to reserved 
>>>>>>> area in TLAB (16)
>>>>>>> 2) Error: Could not create the Java Virtual Machine. \n Error: A 
>>>>>>> fatal exception has occurred. Program will exit.
>>>>>>>
>>>>>>> On Windows:
>>>>>>> $ java -XX:MinTLABSize=1 -version
>>>>>>> 2) Error: Could not create the Java Virtual Machine. \n Error: A 
>>>>>>> fatal exception has occurred. Program will exit.
>>>>>>> 1) MinTLABSize (1) must be greater than or equal to reserved 
>>>>>>> area in TLAB (16)
>>>>>>>
>>>>>>> As a solution, I added 'fflush()' on print function of 
>>>>>>> validation check (CommandLineError::print).
>>>>>>> And any other functions that are printing to 'stderr' would have 
>>>>>>> similar fix for Windows.
>>>>>>>
>>>>>>> CR: https://bugs.openjdk.java.net/browse/JDK-8139801
>>>>>>> Webrev: http://cr.openjdk.java.net/~sangheki/8139801/webrev.00/
>>>>>>> Testing: JPRT
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Sangheon
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>

[prev in list] [next in list] [prev in thread] [next in thread]