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

[Staffan Larsen]

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
>