RFR(S): 8074354: tests that intentionally crash the VM can timeout creating a core file
Thomas Stüfe
thomas.stuefe at gmail.com
Thu Apr 16 06:44:11 UTC 2015
Hi all,
sorry for dropping off the last days. The last changes look fine for me. As
David said, my census name is "stuefe"
Best Regards, and @Yumin thanks for your work and perseverance!
On Thu, Apr 16, 2015 at 4:33 AM, David Holmes <david.holmes at oracle.com>
wrote:
> On 16/04/2015 4:18 AM, Daniel D. Daugherty wrote:
>
>> Yumin,
>>
>> I think Thomas made enough contributions during the review
>> cycle to count him as a (r)eviewer.
>>
>> However, it does not look like Thomas has an OpenJDK user
>>
>
> Yes he does:
>
> http://openjdk.java.net/census#stuefe
>
> David
>
>
> name so it's more difficult.What I do in cases like that is
>> add something to this line:
>>
>> Summary: <my usual summary>; Also reviewed by thomas.stuefe at gmail.com.
>>
>> You may also choose to add Thomas to the contributed by line which
>> would look like this:
>>
>> Contributed-by: yumin.qi at oracle.com, thomas.stuefe at gmail.com
>>
>> Dan
>>
>>
>> On 4/15/15 12:08 PM, Yumin Qi wrote:
>>
>>> 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