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

[Mikael Vidstedt]

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