RFR (S): 8151322: Implement os::set_native_thread_name() on Solaris

[David Holmes]

On 22/03/2016 7:36 AM, Kim Barrett wrote:
>> On Mar 20, 2016, at 5:02 PM, David Holmes <david.holmes at oracle.com> wrote:
>>
>> Hi Kim,
>>
>> Thanks for taking a look at this.
>>
>> Updated webrev: http://cr.openjdk.java.net/~dholmes/8151322/webrev.v3/
>>
>> Comments below.
>>
>> On 20/03/2016 2:24 PM, Kim Barrett wrote:
>>>> On Mar 16, 2016, at 12:24 AM, David Holmes <david.holmes at oracle.com> wrote:
>>>>
>>>> cc'ing James as initial requestor for this.
>>>>
>>>> On 16/03/2016 7:49 AM, Gerard Ziemski wrote:
>>>>>
>>>>>> On Mar 15, 2016, at 4:31 PM, David Holmes <david.holmes at oracle.com> wrote:
>>>>>>
>>>>>>> Couldn’t we look it up on as needed basis in the implementation of "void os::set_native_thread_name(const char *name)” instead?
>>>>>>
>>>>>> AFAIK we never lookup anything as-needed but always handle it at VM initialization time. A quick grep will show that we are using RTLD_DEFAULT in a few places across different platforms. Elsewhere we know what library we have to search. I can try finding out which library it should be if you think that is preferable?
>>>>>
>>>>> Sure, either that or we find out the performance impact on the startup time, so then we can decide if it’s an issue or not.
>>>>
>>>> Some numbers in the bug report. It seems to me if we know the library that will contain the symbol then we should just open it. I filed a RFE:
>>>>
>>>> https://bugs.openjdk.java.net/browse/JDK-8151953
>>>>
>>>> to look at use of RTLD_DEFAULT in general.
>>>>
>>>> Updated webrev: http://cr.openjdk.java.net/~dholmes/8151322/webrev.v2/
>>>>
>>>> Not 100% sure whether dlopen should be also relying on the search path dlopen("libc.so.1",...) - or whether the absolute /usr/lib/libc.so.1 should be hard-wired? I'm not familiar enough with solaris library management to know whether we will always find libc on that exact path? We have one existing /usr/lib/libc.so.1 dlopen on Solaris x86, but most dlopens just take the base name.
>>>
>>> A couple of quibbles.
>>>
>>> Nearly all of the similar places nearby declare a typedef for the
>>> function type near the variable, and use that typedef in both the
>>> variable declaration and the assignment cast.  I found one place in
>>> os_solaris.cpp that didn't do that, but the variable declaration and
>>> the cast are right next to each other in that case, rather than far
>>> apart.  Of course, if the cast is wrong, the assignment will fail to
>>> compile.
>>
>> I added the typedef. Note that I copied this "pattern" from the os_linux.cpp version and on Linux we tend to have fewer dynamic lookups and fewer typedefs. Also I found a range of practices employed in this area on Solaris. For example os_solaris.cpp has this sure enough:
>>
>> typedef struct sigaction *(*get_signal_t)(int);
>> get_signal_t os::Solaris::get_signal_action = NULL;
>>
>> but the os_solaris.hpp header has this:
>>
>>   static struct sigaction *(*get_signal_action)(int);
>>
>> so the typedef seems somewhat misplaced.
>>
>>> Many (but I think not all) of the casts of a dlsym result to a
>>> function pointer use CAST_TO_FN_PTR.
>>
>> No not all by any means. There seems to be no rhyme or reason as to when CAST_TO_FN_PTR is used and when it is not. I also can not see what purpose it serves when used in conjunction with dlsym, which returns a void*. With CAST_TO_FN_PTR we go from void* -> uintptr_t -> real-func-ptr-type. I suspect there is some ancient history here. I chose not to change this as it seems pointless.
>>
>> BTW this, arguably, may be the more "correct" approach:
>>
>> ./os_cpu/solaris_sparc/vm/vm_version_solaris_sparc.cpp:
>>    func = reinterpret_cast<FuncType>(dlsym(_dl_handle, name));
>
> reinterpret_cast (and all the C-style casts involved are really
> reinterpret_casts) can be used to convert a function pointer to a
> different function pointer type.  It can also be used to convert
> between any pointer (including function pointers) and (appropriate)
> integer types.  No other conversions involving function pointers are
> allowed.  (C++11 adds "Converting a function pointer to an object
> pointer type or vice versa is conditionally-supported.")
>
> Since void* is not a function pointer type, there's no specified
> direct conversion from a void* to a function pointer.  That's the
> raison d'etre for CAST_TO_FN_PTR. (Although conversion to an integer
> then to some other type (not back to the original type) and then use
> is implementation-defined.)
>
> However, it seems that some compilers (including the Solaris versions
> we've been using) allow direct void* -> function pointer conversions.

I thought any pointer type could be converted to void* and back again?

David