RFR (S): 8151322: Implement os::set_native_thread_name() on Solaris
kim.barrett at oracle.com
Mon Mar 21 21:36:08 UTC 2016
> 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:
>>> 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
> 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:
> 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
However, it seems that some compilers (including the Solaris versions
we've been using) allow direct void* -> function pointer conversions.
More information about the hotspot-runtime-dev