RFR(S): 8074354: tests that intentionally crash the VM can timeout creating a core file
Yumin Qi
yumin.qi at oracle.com
Wed Apr 15 18:08:18 UTC 2015
On 4/15/2015 11:05 AM, Yumin Qi wrote:
> I need one more reviewer for push --- Thomas Stufe is not a reviewer.
>
In fact I am confused here --- Thomas is not a reviewer and not a
commit-er either, so could I count him in review list.
> Thanks
> 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