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

[David Holmes]

Thanks Staffan!

David

On 21/03/2016 6:26 PM, Staffan Larsen wrote:
> Still ok with me!
>
> /Staffan
>
>> On 20 mars 2016, at 22:02, 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));
>>
>>> I forgot to mention:
>>>
>>>   445   if (Solaris::_pthread_setname_np) {
>>>
>>> violates the no implicit bools rule from the style guide.
>>
>> Fixed (also copied from Linux version).
>>
>>> Functionality looks ok to me.
>>
>> Thanks.
>>
>> David
>>
>