RFR(S): 8074354: tests that intentionally crash the VM can timeout creating a core file
Yumin Qi
yumin.qi at oracle.com
Tue Apr 14 19:54:50 UTC 2015
Thanks for many round reviews!
/Yumin
On 4/14/2015 12:40 PM, Daniel D. Daugherty wrote:
> On 4/14/15 8:42 AM, Yumin Qi wrote:
>> Dan,
>>
>> http://cr.openjdk.java.net/~minqi/8074354/webrev08/
>
> Thumbs up on this round.
>
>
> src/share/vm/runtime/globals.hpp
> No comments.
>
> src/share/vm/runtime/os.hpp
> No comments.
>
> 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
> src/os/bsd/vm/os_bsd.cpp
> src/os/linux/vm/os_linux.cpp
> src/os/posix/vm/os_posix.cpp
> src/os/solaris/vm/os_solaris.cpp
> No comments on the above five files.
>
> src/os/windows/vm/os_windows.cpp
> No comments.
>
> test/runtime/ErrorHandling/ProblematicFrameTest.java
> test/runtime/ErrorHandling/SafeFetchInErrorHandlingTest.java
> No comments on the above two files.
>
> 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
> test/runtime/Unsafe/RangeCheck.java
> test/runtime/memory/ReadFromNoaccessArea.java
> No comments on the above six files.
>
>
> Dan
>
>
>> Sorry missing the comment change. Now it is done.
>>
>> Thanks
>> Yumin
>>
>> On 4/14/2015 7:14 AM, Yumin Qi wrote:
>>> Dan,
>>>
>>>
>>> I missed your comments, so will change again. Sorry for that.
>>> Will update soon.
>>>
>>> Thanks
>>> Yumin
>>>
>>> On 4/13/2015 10:13 PM, Yumin Qi wrote:
>>>> Dan, Thanks.
>>>>
>>>> Changed to add CloseHandle(dumpFile) before all exit paths.
>>>> Please review at same link:
>>>>
>>>> http://cr.openjdk.java.net/~minqi/8074354/webrev08/
>>>>
>>>> Yumin
>>>>
>>>>
>>>> On 4/13/2015 7:04 PM, Daniel D. Daugherty wrote:
>>>>> Yumin,
>>>>>
>>>>> I would prefer that the handle is properly closed on all the
>>>>> exit paths. Not closing the handle might even trigger a future
>>>>> Microsoft sanity check; kind of like when Microsoft added checks
>>>>> for duplicate close() calls...
>>>>>
>>>>> Dan
>>>>>
>>>>>
>>>>> On 4/13/15 4:05 PM, Yumin Qi wrote:
>>>>>> Dan,
>>>>>>
>>>>>> On 4/13/2015 2:09 PM, Daniel D. Daugherty wrote:
>>>>>>> On 4/13/15 10:58 AM, Yumin Qi wrote:
>>>>>>>> Thomas,
>>>>>>>>
>>>>>>>> Answers embeded below. The new web is just the same link
>>>>>>>> updated (08):
>>>>>>>>
>>>>>>>> http://cr.openjdk.java.net/~minqi/8074354/webrev08/
>>>>>>>
>>>>>>> src/share/vm/runtime/globals.hpp
>>>>>>> No comments.
>>>>>>>
>>>>>>> src/share/vm/runtime/os.hpp
>>>>>>> L720: // ON Posix compatible OS it will
>>>>>>> typo: 'ON' -> 'On'
>>>>>>>
>>>>>>> L724: ... or a short error message, depends
>>>>>>> grammar: 'depends' -> 'depending'
>>>>>>>
>>>>>>> L725: // on checking result.
>>>>>>> grammar: 'on checking' -> 'on the checking'
>>>>>>>
>>>>>>> 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
>>>>>>> src/os/bsd/vm/os_bsd.cpp
>>>>>>> src/os/linux/vm/os_linux.cpp
>>>>>>> src/os/posix/vm/os_posix.cpp
>>>>>>> src/os/solaris/vm/os_solaris.cpp
>>>>>>> No comments on the above five files.
>>>>>>>
>>>>>>> src/os/windows/vm/os_windows.cpp
>>>>>>> L1010: jio_snprintf(buffer, buffsz, "Failed to create
>>>>>>> coredump file (0x%x).", GetLastError());
>>>>>>> Because this is a Win* specific file you can use "minidump"
>>>>>>> here instead of "coredump".
>>>>>>>
>>>>>>> I made a similar comment in the previous round.
>>>>>>>
>>>>>> Yes, I will change 'coredump' into 'minidump'
>>>>>>> L1028: if (!dump_core || !dumpFile) {
>>>>>>> The "!dumpFile" part is an implied boolean which we don't
>>>>>>> like to use in HotSpot. Perhaps: dumpFile == NULL
>>>>>>>
>>>>>> Will change to Hotspot style.
>>>>>>> L1078: win32::exit_process_or_thread(win32::EPT_PROCESS, 1);
>>>>>>> The "CloseHandle(dumpFile)" call is missing.
>>>>>>>
>>>>>> There is no need to call CloseHandle here explicitly, since we
>>>>>> will exit next and all resources will be reclaimed by OS. I
>>>>>> remove it at last since think it is not needed here. If you check
>>>>>> previous calls to
>>>>>> win32::exit_process_or_thread(win32::EPT_PROCESS, 1);
>>>>>> I did not place CloseHandle(dumpFile) either.
>>>>>> If needed, I think we should put the call before all exit entry.
>>>>>>> test/runtime/ErrorHandling/ProblematicFrameTest.java
>>>>>>> test/runtime/ErrorHandling/SafeFetchInErrorHandlingTest.java
>>>>>>> No comments on the above two files.
>>>>>>>
>>>>>>> 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
>>>>>>> test/runtime/Unsafe/RangeCheck.java
>>>>>>> test/runtime/memory/ReadFromNoaccessArea.java
>>>>>>> No comments on the above six files.
>>>>>> Thanks
>>>>>> Yumin
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Dan
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Thanks
>>>>>>>> Yumin
>>>>>>>>
>>>>>>>> On 4/13/2015 12:30 AM, Thomas Stüfe wrote:
>>>>>>>>> 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());
>>>>>>>>>
>>>>>>>>>
>>>>>>>> Changed as above. No :: global scope operator needed here.
>>>>>>>>>
>>>>>>>>> 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.
>>>>>>>>>
>>>>>>>> removed.
>>>>>>>>> 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);
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>>
>>>>>>>> changed as expected.
>>>>>>>>>
>>>>>>>>> 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 ....
>>>>>>>>>
>>>>>>>>>
>>>>>>>> Agree, this is the last step before exit, with output of last
>>>>>>>> error code, user can get what happened --- though a little
>>>>>>>> effort to research.
>>>>>>>>
>>>>>>>>> Rest is fine.
>>>>>>>>> Thomas
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Mon, Apr 13, 2015 at 7:59 AM, Yumin Qi <yumin.qi at oracle.com
>>>>>>>>> <mailto:yumin.qi at oracle.com>> wrote:
>>>>>>>>>
>>>>>>>>> Thomas and Dan,
>>>>>>>>>
>>>>>>>>> Please review the changes at
>>>>>>>>> http://cr.openjdk.java.net/~minqi/8074354/webrev08/
>>>>>>>>> <http://cr.openjdk.java.net/%7Eminqi/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>
>>>>>>>>> <mailto: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/>
>>>>>>>>> <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/>
>>>>>>>>> <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>>
>>>>>>>>> <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:
>>>>>>>>>
>>>>>>>>> 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/>
>>>>>>>>> <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/>
>>>>>>>>> <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>>
>>>>>>>>> <mailto: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/>
>>>>>>>>> <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