RFR(S): 8074354: tests that intentionally crash the VM can timeout creating a core file
Thomas Stüfe
thomas.stuefe at gmail.com
Wed Apr 8 14:39:56 UTC 2015
Hi Daniel, Yumin,
sorry for following up so late.
About os::abort() on Windows:
My proposal would be to pre-open the minidump file in
os::check_dump_limit() like this:
static HANDLE theDumpFile = NULL;
void os::check_dump_limit(char* buffer, size_t bufferSize) {
// - build up filename in buffer
// - open file using CreateFile. On error, call
VMError:.report_coredump_status(..)
// - store handle in global variable dumpFile
}
void os::abort() {
// call MiniDumpWriteDump( with theDumpFile )
// write a short message to stderr like UNIX implementations of abort(3)
do
}
Advantages would be
1) by pre-opening the minidump file in os::check_dump_limits(), we will
know if the file can be created and if not can write a meaningful message
into the error log - which at this point is not written yet.
2) in os::check_dump_limits(), we have a buffer to use, no need to create a
buffer on the stack
3) in os::abort(), we now can get rid of the buffers on the stack: the core
file is already open, no need to construct the file name again.
Using a global static variable would be ok, because we will never run into
this case multihreaded (see VMError::report_and_die())
In os::abort(), I also would not use VMError::out_stream() as an output
handle, but simple stderr - because every other abort(3) on non-windows
platforms behaves the same way. Using plain stderr means you avoid any
problems with VMError::out_stream() not initialized.
In os::abort(), I also would write less information to stderr - I do not
think you need to output the name of the core file again (which now we
would see in the hs-err log file). Again, other abort(3) implementations
are not really verbose, they print "aborted" or "aborted (core dumped)".
I think, done this way, the code in os::abort() could become simpler and
some of the issues Dan mentioned would disappear.
Regards, Thomas
On Tue, Apr 7, 2015 at 6:23 PM, Daniel D. Daugherty <
daniel.daugherty at oracle.com> wrote:
> On 4/1/15 10:57 AM, Yumin Qi wrote:
>
> Hi, Dan and Thomas,
>
> I have a new webrev at
> http://cr.openjdk.java.net/~minqi/8074354/webrev06/
>
>
>
> src/share/vm/runtime/globals.hpp
> Sorry, I didn't notice this before:
>
> L939: product(bool, CreateCoredumpOnCrash,
> true, \
> L940: "Create minidump on VM fatal
> error") \
>
> The "minidump" on L940 should change also. Don't know if you
> want to say "core/mini dump" or something else.
>
>
> src/share/vm/runtime/os.hpp
> L719: // On Linux/Solaris it will simply check core dump limits
> while on Windows will do nothing.
> grammar: 'on Windows will do nothing'
> -> 'on Windows it will do nothing'
>
>
> src/share/vm/runtime/arguments.cpp
> No comments.
>
> src/share/vm/utilities/vmError.hpp
> No comments.
>
> src/share/vm/utilities/vmError.cpp
> No comments.
>
> 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
> L92: jio_snprintf(buffer, bufferSize, "%s\\hs_err_pid%u.mdmp",
> get_current_directory(NULL, 0), current_process_id());
> Other non-Windows calls to get_current_directory() do a NULL
> check before using the return value. The original Windows
> code that called get_current_directory() did not make this
> check, but I think it's better to be consistent and safe.
>
> L1007: static char buffer[O_BUFLEN];
> L1008: char filename[FILENAME_MAX];
> Two more potentially large stack variables here. Will we
> run into stack overflow on Win* more frequently when what
> we really wanted was the reason for the abort() call?
>
> The reason the original code used 'buffer' to construct the
> dump file name was to save space. The original code was also
> careful to not need the file name in any error messages after
> the point at which the dump file was successfully created.
>
> It would be better (more resilient) if you could pass in
> an already existing buffer to abort() like the original
> code passed into os::check_or_create_dump(). Yes, that
> would require visiting all existing calls to abort().
>
> L1015: assert(out->is_open(), "defaultStream should be open");
> Is it possible for os::abort() to be called early enough on
> Windows such that VMError::out is not actually setup? Yes,
> this would have to be absolutely catastrophic...
>
> See the comment in os_solaris.cpp L1520-1522.
>
> L1018: jio_snprintf(buffer, sizeof(buffer), "%s", "Either
> CreateCoredumpOnCrash is disabled from command"
> L1019 " line or coredump is not available");
> When you are using jio_snprintf() as a replacement for:
>
> strncpy(buffer, "my string", sizeof(buffer));
> buffer[sizeof(buffer) - 1] = '\0';
>
> you don't need the "%s" format string. So this code would be:
>
> jio_snprintf(buffer, sizeof(buffer),
> "Either CreateCoredumpOnCrash is disabled from "
> "command line or coredump is not available");
>
> Same comment for: L1026, L1039
>
> L1020: goto done;
> The use of 'goto' makes me cringe. You can refactor much
> of this logic into an abort_internal() function that
> abort() calls and keep all the original "return" logic
> to bail out.
>
> L1045: // Older versions of dbghelp.h doesn't contain all
> grammar: 'doesn't' -> 'do not'
>
> L1052: cwd = get_current_directory(NULL, 0);
> L1053: jio_snprintf(filename, sizeof(filename),
> "%s\\hs_err_pid%u.mdmp", cwd, current_process_id());
> Other non-Windows calls to get_current_directory() do a NULL
> check before using the return value. The original Windows
> code that called get_current_directory() did not make this
> check, but I think it's better to be consistent and safe.
>
> L1092: // well done.
> Like a overcooked steak? :-) Not sure what you're trying
> to say here...
>
> 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
> test/runtime/Safepoint/AssertSafepointCheckConsistency2.java
> test/runtime/Safepoint/AssertSafepointCheckConsistency3.java
> test/runtime/Safepoint/AssertSafepointCheckConsistency4.java
> No comments on these four.
>
>
> Dan
>
>
>
> What changes:
>
> 1) os_windows.cpp, os::abort(bool ....):
> a) Since when abort() is called, the last function called before
> exit process, VMError::log already closed, which is the file logging.
> defaultStream still available to use, so use it for writing error or
> success messages. VMError::out is the defaultStream, for using it, added a
> function out_stream() in VMError.
> b) Delete the codes which decide to write core based on
> client/server version with debug. The new code is if CreateCoredumpOnCrash
> set or dump_core is on, the coredump will be tried no matter which version
> it is. There is no special reason not to dump core with client version I
> think. Any concern for this change?
> c) Use of 'goto' in this function. I tried not to use it, but using
> it makes code clear. I remember somewhere suggest not using 'goto' in
> cplusplus.
>
> 2) changed comments to make them consistent as indicated by Dan.
>
> 3) Added/modified head version for files.
>
> Tests: JPRT, manual test on local Windows.
>
> Thanks
> Yumin
>
>
> On 3/31/2015 1:25 AM, Thomas Stüfe wrote:
>
> Hi Daniel, Yumin,
>
> sorry, a lot of Yumins changes were based on suggestions from me, so
> here more background:
>
> Yumin reversed the order of error log writing and core dumping. The
> reason for that are explained in detail at the start of the mail thread, in
> short:
>
> - on Windows core file dumping may hang - its rare but it happens -
> preventing error logs. Depending on who you ask, error logs are more
> important than minidumps, so it makes sense to first write the erorr log.
> - This also brings windows core paths closer to UNIX code paths. See below.
>
> About writing to stderr in os::abort():
>
> After this change, calling VmError::report_coredump_status() in
> os::abort() will not do anything because the error log is already written
> at that point. I suggested to Yumin writing to stderr instead in
> os::abort() because that mimicks UNIX behaviour: On UNIX, you call
> abort(3), which writes a core and aborts. It writes a short message to
> stderr "Aborted (core dumped)" or just "Aborted".
>
> After Yumins change, on Windows os::abort also writes the core, then
> aborts. It made sense to me that this function should behave the same way
> as on UNIX: if core writing fails, write to stderr. There is no way
> otherwise to communicate a core dump writing failure. After writing the
> core, process stops.
>
> ---
>
> About the control flows:
>
> Before Yumins change:
>
> On Windows
> 1) os::check_or_create_dump(): we wrote the core dump and then information
> about the dump (file location, success or error) was handed to the misnamed
> VmError::report_coredump_status(), which just stored that information.
> 2) VmError::report(): Error log was written and in step 63 information
> about the core (already written at that point) was added to error log.
> 3) os::abort() just did an exit(3)
>
> On Unix,
> 1) os::check_or_create_dump(): checks the limits to guess whether core
> dumping later will succeed. Again, that information is handed to
> VmError::report_coredump_status()
> 2) VmError::report(): Error log is written and in step 63, information
> about the core's probable future location is added to error log. Here,
> messages use past tense, which is misleading, because the core is not yet
> written.
> 3) os::abort() calls abort(3), which stops and writes a core.
>
> Yumin pushed core file writing on Windows down to os::abort(). This
> brings the control flow much closer to Unix:
>
> (1) call os::check_dump_limit() to check the prerequisites for core file
> writing and gather a bit information to put in the error log: On Unix,
> limit checks, on windows, so far nothing much but precalculating the error
> file name.
> (2) VmError::report(): Error log is written and information about the
> core's probable location is added to error log.
> (3) os::abort() is called, which on all platforms:
> - dumps the core
> - if failing, writes a one-liner to stderr
> - stops the process.
>
> I think, if one agrees that reversing order of core dumping and error
> log writing on windows is a good thing, this change looks ok to me. Some
> improvements could make it clearer:
>
> - maybe rename "os::check_dump_limit()" (was my suggestion, sorry) to
> "os::check_core_prerequisites()" - that leaves room to flesh it out a bit
> more on Windows, where we do not have UNIX limits.
> - make the messages in VMError::report() future sense: "Will write core
> file to location.....".
> - Make the messages written in os::abort() on Windows clearer, and
> shorter, and we also do not need the buffer to be static here. Actually, if
> we want to be as on UNIX, just dumping "core file writing succeeded" or
> "failed" - maybe with an error number from GetLastError() - will be enough
> because the file location is already printed in the error log. Like on UNIX.
>
> Kind Regards, Thomas
>
>
> On Mon, Mar 30, 2015 at 9:51 PM, Daniel D. Daugherty <
> daniel.daugherty at oracle.com> wrote:
>
>> Re: report_coredump_status() is really record_coredump_status()
>>
>> The report_coredump_status() function is designed to record two
>> things about core dumps for later reporting by the code that
>> generates the hs_err_pid file. That's why the original
>> report_coredump_status() didn't output anything to stderr.
>>
>> By changing the Windows calls to report_coredump_status() into
>> jio_fprintf() calls you have:
>>
>> - put output onto stderr that should go to the hs_err_pid file
>> - made the windows code paths work differently than the non-windows
>> code paths
>>
>
>> Dan
>>
>>
>>
>> On 3/30/15 1:28 PM, Yumin Qi wrote:
>>
>>> 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