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

[David Holmes]

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