RFR (S): 7129724: Fix location of core file in dump message on macosx

[keith mcguigan]

Still looks good to me! :)

--
- Keith

On 7/3/2012 1:33 PM, Mikael Vidstedt wrote:
>
> Thanks David!
>
> Can I please get one more review of this given the difference from the
> first webrev?
>
> Thanks,
> Mikael
>
>
> On 7/2/2012 7:32 PM, David Holmes wrote:
>> Thanks Mikael - looks good to go.
>>
>> David
>>
>> On 3/07/2012 2:57 AM, Mikael Vidstedt wrote:
>>>
>>> New version:
>>>
>>> http://cr.openjdk.java.net/~mikael/7129724/webrev.07/
>>>
>>> Now checks return value from get_current_directory and get_core_path has
>>> been hoisted.
>>>
>>> On 2012-06-27 14:50, David Holmes wrote:
>>>> On 28/06/2012 4:58 AM, Mikael Vidstedt wrote:
>>>>> You're right. I was set on maintaining the "correct" string, but
>>>>> relaxing that somewhat does make it easier.
>>>>>
>>>>> http://cr.openjdk.java.net/~mikael/7129724/webrev.02
>>>>
>>>> Thanks Mikael!
>>>>
>>>> One suggestion to address an existing latent bug in the code: check
>>>> the return value from os::_get_current_directory is not NULL. In a
>>>> generic sense we may have a path longer than the buffer being passed
>>>> to getcwd(). I'd just add an assert for that.
>>>>
>>>> Two possible small cleanups:
>>>>
>>>> src/os/bsd/vm/os_bsd.cpp
>>>>
>>>> How can n be greater than bufferSize? I would have thought you could
>>>> just do "return jio_snprintf(...);"
>>>
>>> snprintf (and therefore jio_snprintf) can return a value larger than
>>> bufferSize if the resulting string would have been longer than the
>>> supplied buffer. Or put differently, if the resulting string has been
>>> truncated jio_snprintf will still return the length of the full string.
>>>
>>> Cheers,
>>> Mikael
>>>
>>>> src/os/posix/vm/os_posix.cpp
>>>>
>>>> You could hoist out the get_core_path call to the start and so only
>>>> write it once:
>>>>
>>>> 35 void os::check_or_create_dump(void* exceptionRecord, void*
>>>> contextRecord, char* buffer, size_t bufferSize) {
>>>> 37 struct rlimit rlim;
>>>> 38 bool success;
>>>> 36 int n = get_core_path(buffer, bufferSize);
>>>> 39
>>>> 40 if (getrlimit(RLIMIT_CORE, &rlim) != 0) {
>>>> 42 jio_snprintf(buffer + n, bufferSize - n, "/core or core.%d (may not
>>>> exist)", current_process_id());
>>>> ...
>>>>
>>>> Cheers,
>>>> David
>>>>
>>>>>
>>>>> Thanks,
>>>>> Mikael
>>>>>
>>>>> On 6/26/2012 5:47 PM, David Holmes wrote:
>>>>>> Hi Mikael,
>>>>>>
>>>>>> On 27/06/2012 10:31 AM, Mikael Vidstedt wrote:
>>>>>>>
>>>>>>> David,
>>>>>>>
>>>>>>> Do you mean something like:
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~mikael/7129724/webrev.01
>>>>>>
>>>>>> Something like, but to simplify things I was thinking of just
>>>>>> returning the actual path component and have the check_or_create_dump
>>>>>> handle the "core or core.%d" part. No need to duplicate the pid
>>>>>> parsing logic then and the main part for linux/solaris is simply:
>>>>>>
>>>>>> int os::get_core_paths(char* buffer, size_t bufferSize) {
>>>>>> get_current_directory(buffer, bufferSize);
>>>>>> return strlen(buffer);
>>>>>> }
>>>>>>
>>>>>> (this is unchecked - I'm assuming things are properly nul-terminated
>>>>>> etc. Pity about the need for strlen, but then you could choose not to
>>>>>> return the length and instead do the strlen directly ie:
>>>>>>
>>>>>> ! os::get_core_paths(buffer, bufferSize);
>>>>>> n = strlen(buffer);
>>>>>> ! jio_snprintf(buffer + n, bufferSize - n, "...");
>>>>>>
>>>>>> either way works I guess)
>>>>>>
>>>>>> My suggestion does mean that the message is not as specific as it
>>>>>> might be if the name is always core.pid rather than core. But that
>>>>>> seems a very minor concern.
>>>>>>
>>>>>> BTW in current webrev this part needs fixing up:
>>>>>>
>>>>>> if (getrlimit(RLIMIT_CORE, &rlim) != 0) {
>>>>>> ! n = os::get_core_paths(buffer, bufferSize);
>>>>>> ! jio_snprintf(buffer + n, bufferSize - n, "%s/core or core.%d (may
>>>>>> not exist)");
>>>>>> success = true;
>>>>>>
>>>>>> You are no longer parsing anything for the %s and %d - I think you
>>>>>> really only want the "may not exist" part.
>>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>>
>>>>>>> Cheers,
>>>>>>> Mikael
>>>>>>>
>>>>>>> On 2012-06-25 23:31, David Holmes wrote:
>>>>>>>> Hi Mikael,
>>>>>>>>
>>>>>>>> On 26/06/2012 8:22 AM, Mikael Vidstedt wrote:
>>>>>>>>> If HotSpot crashes the dump message prints the default location
>>>>>>>>> of the
>>>>>>>>> generated core file. On other "posix" like OSes the path is
>>>>>>>>> normally
>>>>>>>>> <cwd>/core or core.<pid>, but on macosx the cores always end up in
>>>>>>>>> /cores/core.<pid>. This change aims to reflect that. Verified
>>>>>>>>> manually
>>>>>>>>> on Linux and macosx. Passes JPRT.
>>>>>>>>>
>>>>>>>>> Webrev: http://cr.openjdk.java.net/~mikael/7129724/webrev.00
>>>>>>>>
>>>>>>>> I don't like this - sorry. The whole point of introducing
>>>>>>>> os_posix.cpp/hpp was to capture common "posix" functionality. The
>>>>>>>> very
>>>>>>>> fact you need to add this change indicates this is not in fact
>>>>>>>> "common
>>>>>>>> posix functionality". Seems to me that the default core location
>>>>>>>> should be a platform specific value:
>>>>>>>>
>>>>>>>> os::get_corefile_path(char* buf, int buflen)
>>>>>>>>
>>>>>>>> and then each os_*.cpp can define it as needed, with Solaris and
>>>>>>>> Linux
>>>>>>>> simply delegating to the existing os::get_current_directory(buf,
>>>>>>>> len).
>>>>>>>>
>>>>>>>> That aside, it really bugs me that BSD/OSX things are
>>>>>>>> conditionalized
>>>>>>>> on __APPLE__ ! Should be BSD or OSX or our internally defined
>>>>>>>> values
>>>>>>>> like TARGET_OS_FAMILY_xxx
>>>>>>>>
>>>>>>>> Cheers,
>>>>>>>> David
>>>>>>>> ----
>>>>>>>
>>>>>>>
>>>
>>>