RFR(S): 8074354: tests that intentionally crash the VM can timeout creating a core file
Thomas Stüfe
thomas.stuefe at gmail.com
Fri Mar 20 18:55:01 UTC 2015
Hi Yumin,
I think you meant to post webrev.04?
I looked at 04, and it looks nice. Thank you!
(still only reviewer with r)
Thomas
On Mar 20, 2015 6:20 PM, "Yumin Qi" <yumin.qi at oracle.com> wrote:
> Hi, All
>
> http://cr.openjdk.java.net/~minqi/8074354/webrev03/
>
> This version is based on Thomas' suggestion.
> Tests passed vm.runtime.quick.testlist, JPRT and manual tests.
>
> Thanks
> Yumin
>
> On 3/18/2015 10:06 AM, Yumin Qi wrote:
>
>>
>> On 3/18/2015 4:26 AM, Thomas Stüfe wrote:
>>
>>> 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?
>>>
>>> Sure.
>>
>>> - 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.
>>>
>>> Yes, no need to carry those two parameters. I tried to keep less
>> changes, but decided to change now.
>>
>> - 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.
>>>
>>> Sure, will direct output to stderr.
>>
>>> - 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.
>>>
>>> OK
>>
>> Thanks
>> Yumin
>>
>>> Thanks for your work!
>>>
>>> Thomas
>>>
>>>
>>> On Tue, Mar 17, 2015 at 2:04 AM, Yumin Qi <yumin.qi at oracle.com <mailto:
>>> yumin.qi at oracle.com>> wrote:
>>>
>>> Hi, Dan and all
>>>
>>> I have updated webrev at:
>>> http://cr.openjdk.java.net/~minqi/8074354/webrev03/
>>> <http://cr.openjdk.java.net/%7Eminqi/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/ <
>>> http://cr.openjdk.java.net/%7Eminqi/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>
>>> <mailto: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/>
>>> <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
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>
>
More information about the hotspot-runtime-dev
mailing list