RFR(S): 8074354: tests that intentionally crash the VM can timeout creating a core file
Yumin Qi
yumin.qi at oracle.com
Mon Mar 30 19:28:23 UTC 2015
Thanks Dan
On 3/30/2015 11:25 AM, Daniel D. Daugherty wrote:
> 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.
>
Will convert them to be consistent.
> 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.
>
Will use version w/o tail.
> 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
>
I will consider your comments, then revise with a new version. I
remember here there is a case, if status not set, it will output an
inconsistent message. Will check again.
> 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?
The original buffer is static too. Carrying an buffer for abort seems
not right. This buffer in fact only for storing file name (based on pid)
here.
abort() will dump the core file, should we prevent multi-thread calling
to this function? I did not check this part, will check if it is MT-safe.
>
> 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.
>
Changing to jio_fprintf is because report_coredump_status did not output
anything, it is just a logging as you indicated. It is reasonable we
change it to record_coredump_status instead. How about add printout to
report_coredump_status? So I do not need to use jio_printf here.
Other consideration of using jio_fprintf after call shutdown called, the
static output stream still exists, but not guaranteed to work as expected.
Thanks
Yumin
> 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