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

[David Holmes]

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
>>>>>> ----
>>>>>
>>>>>
>
>