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

List:       openjdk-serviceability-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-23 22:57:57
Message-ID: 562ABB75.3060507 () oracle ! com
[Download RAW message or body]

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]