RFR(S): 8074354: tests that intentionally crash the VM can timeout creating a core file
Yumin Qi
yumin.qi at oracle.com
Tue Apr 14 14:42:55 UTC 2015
Dan,
http://cr.openjdk.java.net/~minqi/8074354/webrev08/
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