RFR(S): 8074354: tests that intentionally crash the VM can timeout creating a core file

[Thomas Stüfe]

Hi Yumin,

- just aesthetics, but on all implementations of os_abort(), at least on
the UNIX variants, maybe we could consistently rename the parameters to be
"siginfo" and "context" instead of using Windows terms?

- On check_create_dump_limit(), you do not need exceptionRecord,
contextRecord parameters anymore. The only parameters now needed are buffer
and buffer size, which is not even an output parameter, just a scratch
buffer for this function to use for printf. For output, it calls
VMError::report_coredump_status(...) at the end to provide information
about the core file.

- I would rename that function from check_create_dump_limit() to
check_dump_limit() - nothing is created anymore.

- on Windows, in os::abort(), there is no point anymore in calling
VMError::report_coredump_status(...) because that information is only used
during error log writing which already happened. It only makes sense to
call this function in check_create_dump_limit(), which happens before error
log writing.
Instead, maybe error messages like "Call to MiniDumpWriteDump() failed"
should just be written to stderr? This would be consistent with Unix, where
the OS writes a short message on stderr if core file writing fails.

- there is now a new test,
test/runtime/ErrorHandling/SafeFetchInErrorHandlingTest.java - could you
please also add "-XX:-CreateCoredumpOnCrash" ? I just added the test and
did not want to add the option before it was available.

Thanks for your work!

Thomas


On Tue, Mar 17, 2015 at 2:04 AM, Yumin Qi <yumin.qi at oracle.com> wrote:

> Hi, Dan and all
>
>   I have updated webrev at:
>   http://cr.openjdk.java.net/~minqi/8074354/webrev03/
>
>   1) bug changed synopsis
>        8074354: Make CreateMinidumpOnCrash a new name and available on all
> platforms
>   2) tests: JPRT, vm.runtime.quick.testlist (on Windows), jtreg on Windows
> and Linux/Unix.
>
>   Thanks
>   Yumin
>
>
> On 3/12/2015 12:59 PM, Daniel D. Daugherty wrote:
>
>> I believe there are both JavaTest/JTREG tests in the hotspot repo
>> and there are VM/NSK tests from back when we did phone home...
>>
>> Check with Christian or Misha...
>>
>> Dan
>>
>>
>> On 3/12/15 1:50 PM, Yumin Qi wrote:
>>
>>> Thanks, Dan
>>>
>>> Will look at and run those tests. Are they under  nsk?
>>>
>>> Yumin
>>>
>>> On 3/12/2015 12:29 PM, Daniel D. Daugherty wrote:
>>>
>>>> Yumin,
>>>>
>>>> There are some error handler tests. You should find those and make
>>>> sure that you run them on Windows since you're changing the order
>>>> there...
>>>>
>>>> Dan
>>>>
>>>>
>>>> On 3/12/15 12:42 PM, Yumin Qi wrote:
>>>>
>>>>> Hi, Thomas and all
>>>>>
>>>>>    The second version of the change:
>>>>>    webrev: http://cr.openjdk.java.net/~minqi/8074354/webrev02/
>>>>>
>>>>>    In this version:
>>>>>    1) switch order on Windows to first print log file as other
>>>>> platform, then dump core file if needed. When VMError created after crash,
>>>>> siginfo and context which are ExceptionRecord and ContextRecord on Windows
>>>>> are recorded, mini (or full, due to 'or' used so type will be mini anyway)
>>>>> dump creates dump file based on those two as before.
>>>>>    2) to make os::abort to get above two pointers, added two more
>>>>> fields to the function parameters:
>>>>>    3)  changes for test/runtime/ErrorHandling/SecondaryError.java ---
>>>>> added "-XX:-CreateCoredumpOnCrash"
>>>>>
>>>>>     -  static void abort(bool dump_core = true);
>>>>>    +  static void abort(bool dump_core = true, void *siginfo = NULL,
>>>>> void *context = NULL);
>>>>>
>>>>>    Tests: JPRT and manually.
>>>>>
>>>>> Thanks
>>>>> Yumin
>>>>>
>>>>> On 3/11/2015 3:47 AM, Thomas Stüfe wrote:
>>>>>
>>>>>> Hi Yumin,
>>>>>>
>>>>>> There is also test/runtime/ErrorHandling/SecondaryErrorTest.java -
>>>>>> could you please add "-XX:-CreateCoredumpOnCrash" ? Thank you!
>>>>>>
>>>>>> Beyond that, as I wrote in the bug report comments:
>>>>>>
>>>>>> "This is also a problem on Windows - MiniDumpWriteDump() may hang
>>>>>> infinitly. And on Windows this is worse than under UNIX because we create
>>>>>> the Dump before writing the hs-err file, so if the Dump hangs, we get no
>>>>>> error log. I would like to revert the order: create the minidump after
>>>>>> writing the error log, the same way Unix does it. We did this in our JVM
>>>>>> (SAP) because for us, error logs are more useful than minidumps. "
>>>>>>
>>>>>> So, I would like to see os::abort on Windows call
>>>>>> MiniDumpWriteDump(), and thus the mini dump writing moved after the error
>>>>>> log writing. This would also make the code way cleaner because the control
>>>>>> flow would be the same on all platforms.
>>>>>>
>>>>>> I understand that this may be out of scope for your change, but I
>>>>>> would like to know what others think about this.
>>>>>>
>>>>>> Kind regards, Thomas
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Wed, Mar 11, 2015 at 8:02 AM, Yumin Qi <yumin.qi at oracle.com
>>>>>> <mailto:yumin.qi at oracle.com>> wrote:
>>>>>>
>>>>>>     Please review:
>>>>>>
>>>>>>     bug: https://bugs.openjdk.java.net/browse/JDK-8074354
>>>>>>     webrev: http://cr.openjdk.java.net/~minqi/8074354/webrev01/
>>>>>> <http://cr.openjdk.java.net/%7Eminqi/8074354/webrev01/>
>>>>>>
>>>>>>     Summary: Tests timed out when VM crashes and dumping core file
>>>>>>     which in the test case is not needed. To make core not created,
>>>>>>     the fix changed CreateMinidumpOnCrash to CreateCoredumpOnCrash,
>>>>>>     the former is only used on Windows and the latter for all
>>>>>>     platforms. When VM crashes on non Windows, core file generated as
>>>>>>     default if OS sets core dump allowed. Default value of
>>>>>>     CreateCoredumpOnCrash set to 'true' to keep same behavior on non
>>>>>>     Windows platforms, but changed for Windows --- original is false,
>>>>>>     not create minidump on Windows. With CreateCoredumpOnCrash turned
>>>>>>     off, no core file will be generated. CreateMinidumpOnCrash still
>>>>>>     can be used on commandline but only as alias for the new flag.
>>>>>>
>>>>>>     Tests: JPRT, manual tests setting CreateMinidumpOnCrash on
>>>>>>     commandline to verify flag change as alias.
>>>>>>
>>>>>>     Thanks
>>>>>>     Yumin
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>