RFR(S): 8074354: tests that intentionally crash the VM can timeout creating a core file
Thomas Stüfe
thomas.stuefe at gmail.com
Mon Apr 13 07:30:15 UTC 2015
Hi Yumin,
some small things:
os_windows.cpp:1012
+ if (dumpFile == INVALID_HANDLE_VALUE) {
+ jio_snprintf(buffer, buffsz, "Failed to create coredump file");
+ status = false;
+ }
Please print out the error number too:
+ jio_snprintf(buffer, buffsz, "Failed to create coredump file
(0x%x).", ::GetLastError());
os_windows.cpp:1036
+ assert(dumpFile != NULL, "dumpFile should not be NULL");
Please remove this assert. I would not do any asserts at this stage, we are
dying anyway and a recursive assert would just confuse things. Moreover,
CreateFile may have failed for "normal" reasons.
Instead, if dumpFile is NULL, just skip the whole minidump stuff in
os::abort; just behave the same way as for os::abort(false):
+ if (!dump_core || !dumpFile) {
+ win32::exit_process_or_thread(win32::EPT_PROCESS, 1);
}
os_windows.cpp: 1076
I would just skip the whole "FormatMessage" stuff. It is just sugar, but it
will allocate buffers, which we may not want at this stage. I think the
plain error code from GetLastError() is sufficient here. So I would just:
1076 // Older versions of dbghelp.dll (the one shipped with Win2003 for
example) may not support all
1077 // the dump types we really want. If first call fails, lets fall
back to just use MiniDumpWithFullMemory then.
1078 if (_MiniDumpWriteDump(hProcess, processId, dumpFile, dumpType,
pmei, NULL, NULL) == false &&
1079 _MiniDumpWriteDump(hProcess, processId, dumpFile,
(MINIDUMP_TYPE)MiniDumpWithFullMemory, pmei, NULL, NULL) == false) {
jio_fprintf(stderr, "Call to MiniDumpWriteDump() failed (Error
0x%x)\n", ::GetLastError());
1089 } else {
1090 ....
Rest is fine.
Thomas
On Mon, Apr 13, 2015 at 7:59 AM, Yumin Qi <yumin.qi at oracle.com> wrote:
> Thomas and Dan,
>
> Please review the changes at
> http://cr.openjdk.java.net/~minqi/8074354/webrev08/
>
> Thanks
> Yumin
>
> On 4/10/2015 10:14 PM, Yumin Qi wrote:
>
>> Thomas,
>>
>> Thanks for review again.
>>
>> On 4/10/2015 4:20 AM, Thomas Stüfe wrote:
>>
>>> Hi Yumin,
>>>
>>> Hi Yumin,
>>>
>>> this looks mostly fine for me. I only was able to test it on Linux, but
>>> will test it on other OSes next week.
>>>
>>> Here two remaining small points:
>>>
>>> - in os::check_dump_limit, maybe a comment would be helpful, because the
>>> contract for this function is a bit difficult to understand. Proposal:
>>>
>>> "Check or prepare a core dump to be taken at a later point in the same
>>> thread in os::abort(). Use the caller provided buffer as a scratch buffer.
>>> Output status message via VMError::report_cordedump_status() (on
>>> success, the core dump file location, on error, a short error message) -
>>> this status message which will be written into the error log."
>>>
>>> I will take the comments, new comments:
>>
>> // ON Posix compatible OS it will simply check core dump limits while
>> on Windows
>> // it will check if dump file can be created. Check or prepare a core
>> dump to be
>> // taken at a later point in the same thread in os::abort(). Use the
>> caller
>> // provided buffer as a scratch buffer. The status message which will
>> be written
>> // into the error log either is file location or a short error message,
>> depends
>> // on checking result.
>> static void check_dump_limit(char* buffer, size_t bufferSize);
>>
>> - It would make sense to improve the output in VMError.report() in
>>> STEP(63) a bit:
>>>
>>> Problem 1: we use past-tense, which is misleading ("Core dump written",
>>> "Failed to write core"). We did not generate the core dump yet, we just
>>> plan to do it, and it still may fail for various reasons (e.g. even if the
>>> limits are fine, the current directory may be write protected. There is no
>>> way to catch all errors beforehand). If the user reads "Core dump written",
>>> he assumes the core dump was written successfully. If abort(3) later fails
>>> to write the core dump, user will be confused.
>>>
>>> Will change the message to
>> if (coredump_status) {
>> st->print("Core dump will be written. %s", coredump_message);
>> } else {
>> st->print("No core dump will be written. %s", coredump_message);
>> }
>>
>> as your suggested.
>> Whether the coredump can be successfully created depends on following
>> core file generation procedure. User can trace error messages if failed to
>> create.
>>
>>> Problem 2: this is more obscure: on Linux core dumps may not be written
>>> to the file system at all but be processed by some other program. On my
>>> Ubuntu 14.4 machine I see this output:
>>>
>>> #
>>> # Core dump written. Default location: Core dumps may be processed with
>>> "/usr/share/apport/apport %p %s %c %P" (or dumping to
>>> /shared/projects/openjdk/jdk9-hs-rt/output/core.26261)
>>> #
>>>
>>> This output is a bit jarring.
>>>
>>> I have not tested on Ubuntu so no comments.
>>
>> I will post a new webrev based on your and Dan's comments. Thanks for
>> your patience.
>>
>> Thanks
>> Yumin
>>
>>> My suggestion for better texts would be:
>>>
>>> "Core dump will be written. %s"
>>> "No core dump will be written. %s"
>>>
>>> which hopefully fits all cases better. But I don't know, other
>>> suggestions are welcome :-)
>>>
>>> Otherwise, this looks fine.
>>>
>>> Thanks for your work!
>>>
>>> Thomas
>>>
>>>
>>> On Fri, Apr 10, 2015 at 5:18 AM, Yumin Qi <yumin.qi at oracle.com <mailto:
>>> yumin.qi at oracle.com>> wrote:
>>>
>>> Hi, Thomas and Dan
>>>
>>> http://cr.openjdk.java.net/~minqi/8074354/webrev07/
>>> <http://cr.openjdk.java.net/%7Eminqi/8074354/webrev07/>
>>>
>>> On 4/9/2015 7:51 PM, Yumin Qi wrote:
>>>
>>> Please hold on, I missed the test cases:
>>>
>>> *test/runtime/Unsafe/RangeCheck.java
>>> *
>>> *test/runtime/memory/ReadFromNoaccessArea.java
>>>
>>> Added.
>>>
>>> Also some file heads need to be updated with this year.
>>>
>>> Changed
>>>
>>> Thanks
>>> Yumin
>>>
>>> Thanks
>>> Yumin
>>>
>>>
>>> *
>>> On 4/9/2015 7:07 PM, Yumin Qi wrote:
>>>
>>> Hi, Thomas and Dan
>>>
>>> The same URL is updated with new version.
>>>
>>> http://cr.openjdk.java.net/~minqi/8074354/webrev07/
>>> <http://cr.openjdk.java.net/%7Eminqi/8074354/webrev07/>
>>>
>>> I remove the public access to VMError::out,
>>> VMError::coredump_message and the size of
>>> VMError::coredump_message.
>>> Added private static HANDLE in os_windows.cpp for
>>> dumpFile as suggested by Thomas.
>>>
>>> Tests passes JPRT and manual tests.
>>>
>>> Thanks
>>> Yumin
>>>
>>>
>>> On 4/9/2015 1:32 AM, Thomas Stüfe wrote:
>>>
>>> Hi Yumin,
>>>
>>> I do not think exposing VMError::coredump_status
>>> buffer just for the sake of using it as a scratch
>>> buffer in os::abort() is a good idea. It is very
>>> confusing and has nothing to do with the purpose of
>>> VMerror::coredump_status.
>>>
>>> Also, VMerror::coredump_status is static, so you do
>>> not even gain anything over just using a global or
>>> function scope static buffer. Either we are worried
>>> about thread safety in os::abort(), or we aren't. If
>>> we aren't, just use a local static buffer, it is much
>>> clearer and no need to expose VMError::coredump_status
>>> like this.
>>>
>>> Apart from that, see my remarks in yesterdays mail.
>>>
>>> Cheers, Thomas
>>>
>>>
>>>
>>>
>>> On Thu, Apr 9, 2015 at 5:16 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
>>>
>>> New webrev at
>>> http://cr.openjdk.java.net/~minqi/8074354/webrev07/
>>> <http://cr.openjdk.java.net/%7Eminqi/8074354/webrev07/>
>>> <http://cr.openjdk.java.net/%7Eminqi/8074354/webrev07/>
>>>
>>> To make code clean, add os::abort_internal which
>>> is only used
>>> for Windows.
>>> Since passing VMError::coredump_message and its
>>> size as
>>> parameters across multiple functions makes
>>> function have a bigger
>>> signature, I change the way to access them -- add
>>> two functions in
>>> VMError to access them: coredump_msg_buffer() and
>>> coredump_msg_buffer_size(). They are static
>>> members, so can save
>>> stack space for passing around.
>>>
>>> in os_windows.cpp, abort(bool, void*, void*):
>>> if error encountered, exit with error messages.
>>> If no error, VMError::coredump_message contains
>>> the dump
>>> path/name. I just use them so no need to have
>>> extra space for name
>>> allocation.
>>>
>>> Passed JPRT and manual tests.
>>>
>>> Thanks
>>> Yumin
>>>
>>>
>>> On 4/7/2015 9:23 AM, Daniel D. Daugherty 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/
>>> <http://cr.openjdk.java.net/%7Eminqi/8074354/webrev06/>
>>> <http://cr.openjdk.java.net/%7Eminqi/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
>>> <mailto:daniel.daugherty at oracle.com>
>>> <mailto:daniel.daugherty at oracle.com
>>> <mailto: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/
>>> <http://cr.openjdk.java.net/%7Eminqi/8074354/webrev05/>
>>> <http://cr.openjdk.java.net/%7Eminqi/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,
>>
>>
More information about the hotspot-runtime-dev
mailing list