Request for review (M): 7132070: Use a mach_port_t as the OSThread thread_id rather than pthread_t on BSD/OSX

[Staffan Larsen]

On 15 feb 2012, at 19:49, Paul Hohensee wrote:

> Also,
> 
> You can move the #include of mach.h into globalDefinitions_gcc.hpp,
> in the #ifdef __APPLE__ section.  That way, it won't cause a compile-
> time error on non-osx platforms that don't have it.

I dislike adding more stuff to globalDefinitions since that gets pulled in all over the place. I'd prefer to have #includes only where they are needed.

> The declaration of _thread_id, thread_id() and set_thread_id() in os_bsd.hpp
> can be put under a #ifdef __APPLE__, vis.,
> 
> #ifdef _ALLBSD_SOURCE
> #ifdef __APPLE__
>  thread_t _thread_id;
> #else
>  pthread_t _thread_id;
> #endif
> #endif
> 
> The uses of mach_thread_self() in os_bsd.cpp can be similarly conditioned.
> 
> If there's a thread_t defined by non-osx bsd implementations, then
> you don't need predication in os_bsd.hpp, though you'd still need it
> in os_bsd.cpp.  I'm assuming there's no mach_thread_self() on non-osx
> bsd platforms.

I could do that, but the question is how much work we should put into not trying to break something that we can't test anyway. There's no way, even if I do this that I can verify that it works.

/Staffan

> 
> Thanks,
> 
> Paul
> 
> On 2/15/12 1:33 PM, Paul Hohensee wrote:
>> Imo we should at least try to make non-osx bsd builds work, since
>> the original code did work for non-osx builds.  This change doesn't
>> do that.
>> 
>> In globalDefinitions_gcc.hpp, if you keep the lines
>> 
>> #undef ELF_AC
>> #undef EFL_ID
>> 
>> then you don't have to change vm_version_x86.hpp.
>> 
>> Paul
>> 
>> On 2/15/12 10:16 AM, Daniel D. Daugherty wrote:
>>> The _ALLBSD_SOURCE symbol is defined by the HotSpot Makefile infrastructure.
>>> It is used to identify code specific to the BSD family of OSes.
>>> The __APPLE__ symbol is defined by the Apple compiler(s) and it is used to
>>> identify code specific to MacOS X.
>>> 
>>> Typically you'll see something like:
>>> 
>>> #ifdef _ALLBSD_SOURCE
>>> 
>>> <code that works on all BSDs>
>>> 
>>> #ifdef __APPLE__
>>> <code specific to MacOS X>
>>> #else
>>> <code for other BSDs>
>>> #endif // __APPLE__
>>> #endif // _ALLBSD_SOURCE
>>> 
>>> As for building on non-MacOS X BSDs, that would be nice, but we
>>> don't have the infrastructure to do it.
>>> 
>>> Dan
>>> 
>>> On 2/15/12 6:57 AM, Mikael Gerdin wrote:
>>>> Hi Staffan,
>>>> 
>>>> It looks like you're adding Mac-specific stuff like thread_t and calls to ::mach_thread_self() inside _ALLBSD_SOURCE #ifdefs, are you sure this won't break BSD builds?
>>>> Does the OSX compiler define _ALLBSD_SOURCE or is that for (free|net|open)bsd?
>>>> It's too bad we don't do regular builds on any of the BSDs, otherwise this would have been easier to figure out.
>>>> 
>>>> /Mikael
>>>> 
>>>> 
>>>> On 2012-02-15 11:29, Staffan Larsen wrote:
>>>>> Please review the following change:
>>>>> 
>>>>> Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7132070
>>>>> 
>>>>> Webrev: http://cr.openjdk.java.net/~sla/7132070/webrev.00/
>>>>> 
>>>>> This changes the value returned by OSThread::thread_id() and
>>>>> os::current_thread_id() on macosx to return the mach thread_t instead of
>>>>> pthread_t. There is a separate method OSThread:pthread_id() that returns
>>>>> the pthread_t.
>>>>> 
>>>>> The reason for this change is both that JFR would like a 4 byte value
>>>>> for thread id, and that SA requires access to the thread_t.
>>>>> 
>>>>> Thanks,
>>>>> /Staffan
>>>>