RFR(S): 8074354: tests that intentionally crash the VM can timeout creating a core file
Daniel D. Daugherty
daniel.daugherty at oracle.com
Mon Mar 30 18:25:37 UTC 2015
On 3/25/15 12:11 PM, Yumin Qi wrote:
> Hi, all
>
> I updated the webrev with a new change to test case:
> test/runtime/Unsafe/RangeCheck.java
>
> Add -XX:-CreateCoredumpOnCrash to test, no dump needed on this case.
>
> new webrev: http://cr.openjdk.java.net/~minqi/8074354/webrev05/
General nit - please update copyright lines to 2015 as needed
src/share/vm/runtime/globals.hpp
No comments.
src/share/vm/runtime/os.hpp
line 720: // On Windows this will create an actual minidump, on
Linux/Solaris it will simply check core dump limits
line 721: static void check_dump_limit(char* buffer, size_t
bufferSize);
The comment on line 720 no longer matches with the function
rename from check_or_create_dump() to check_dump_limit().
You have this comment in vmError.cpp:
943 // check core dump limits on Linux/Solaris, nothing on
Windows
944 os::check_dump_limit(buffer, sizeof(buffer));
so the two comments are out of sync.
src/share/vm/runtime/arguments.cpp
line 3252: } else if (match_option(option,
"-XX:+CreateMinidumpOnCrash", &tail)) {
line 3256: } else if (match_option(option,
"-XX:-CreateMinidumpOnCrash", &tail)) {
These two checks should use the match_option() version
without a '&tail' parameter.
src/share/vm/utilities/vmError.cpp
line 217: bool VMError::coredump_status = true; // presume we
can dump core file first
I don't think you should init this to true. It confuses
things with VMError::report_coredump_status(). It will also
confuse this code:
526 STEP(63, "(printing core file information)")
529 if (coredump_status) {
530 st->print("Core dump written. Default location:
%s", coredump_message);
because coredump_status won't accurately reflect whether
the coredump_message field has been set.
line 943: // check core dump limits on Linux/Solaris, nothing on
Windows
This updated comment doesn't match the unmodified
comment in os.hpp:
720: // On Windows this will create an actual minidump, on
Linux/Solaris it will simply check core dump limits
src/os/aix/vm/os_aix.cpp
No comments.
src/os/bsd/vm/os_bsd.cpp
No comments.
src/os/linux/vm/os_linux.cpp
No comments.
src/os/posix/vm/os_posix.cpp
No comments.
src/os/solaris/vm/os_solaris.cpp
No comments.
src/os/windows/vm/os_windows.cpp
line 1008: static char buffer[O_BUFLEN];
Why switch from a buffer parameter to a static buffer?
What happens with parallel calls to abort()? Will the static
buffer get stomped by the competing threads?
line 1015: // If running on a client version of Windows and user
has not explicitly enabled dumping
line 1016: if (!os::win32::is_windows_server() &&
!CreateCoredumpOnCrash) {
The default for CreateCoredumpOnCrash is now 'true' so the
comment and logic are no longer correct here. The Windows
Client VM will generate minidumps by default.
old line 1007: VMError::report_coredump_status("Minidumps are not
enabled by default on client versions of Windows", false);
new line 1017: jio_fprintf(stderr, "Minidumps are not enabled by
default on client versions of Windows.\n");
There are a number of places where we change from
VMError::report_coredump_status() to jio_fprintf()
and I'm not quite seeing why this was done.
Update: VMError::report_coredump_status() is misnamed. It doesn't
report anything in the sense that it doesn't print anything. It
does record two pieces of information about core dumps so maybe
it should be VMError::record_coredump_status().
line 1100: jio_fprintf(stderr, "%s.\n", buffer);
At this point, buffer contains the path to the
mini-dump file so the above line simply prints
that on stderr. Why?
Yes, I see that the old code did something similar:
1090 VMError::report_coredump_status(buffer, true);
but report_coredump_status() didn't actually print
anything. It just squirreled away these in vmError.cpp:
217 bool VMError::coredump_status;
218 char VMError::coredump_message[O_BUFLEN];
See comments above about how report_coredump_status()
is misnamed.
At this point, I'm convinced that all the changes from
VMError::report_coredump_status() to jio_fprintf() are
a bad idea.
test/runtime/ErrorHandling/ProblematicFrameTest.java
No comments.
test/runtime/ErrorHandling/SafeFetchInErrorHandlingTest.java
No comments.
test/runtime/ErrorHandling/SecondaryErrorTest.java
No comments.
test/runtime/Safepoint/AssertSafepointCheckConsistency1.java
No comments.
test/runtime/Safepoint/AssertSafepointCheckConsistency2.java
No comments.
test/runtime/Safepoint/AssertSafepointCheckConsistency3.java
No comments.
test/runtime/Safepoint/AssertSafepointCheckConsistency4.java
No comments.
test/runtime/Unsafe/RangeCheck.java
No comments.
test/runtime/memory/ReadFromNoaccessArea.java
No comments.
>
> Thanks
> Yumin
>
>
> On 3/23/2015 11:48 AM, Yumin Qi wrote:
>> Since Thomas is not a reviewer in open jdk, I need two volunteers (at
>> least one "R"eviewer).
>>
>> Dan, since you already reviewed previous version, could you have a look?
>>
>> Thanks
>> Yumin
>>
>> On 3/20/2015 3:20 PM, Yumin Qi wrote:
>>> Thomas,
>>>
>>> Thanks. Yes, it is webrev04. Also, I have updated webrev04 to add
>>> another test case change: test/runtime/memory/ReadFromNoaccessArea.java
>>> (Thanks Dan's update)
>>>
>>> Thanks
>>> Yumin
>>>
>>> On 3/20/2015 11:55 AM, Thomas Stüfe wrote:
>>>>
>>>> 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
>>>> <mailto:yumin.qi at oracle.com>> wrote:
>>>>
>>>> Hi, All
>>>>
>>>> http://cr.openjdk.java.net/~minqi/8074354/webrev03/
>>>> <http://cr.openjdk.java.net/%7Eminqi/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>
>>>> <mailto: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/>
>>>> <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/>
>>>> <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>>
>>>> <mailto: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/>
>>>> <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