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

[Staffan Larsen]

New webrev: http://cr.openjdk.java.net/~sla/7132070/webrev.01/

I've tried not to break the bsd code. The code is less than readable with all the #ifdefs in there, but hopefully we can clean that up eventually. I'm a little unsure about the changes in vmStructs_bsd_x86.hpp - please look carefully.

Thanks,
/Staffan


On 15 feb 2012, at 20:40, Paul Hohensee wrote:

> Thanks!
> 
> Paul
> 
> On 2/15/12 2:25 PM, Staffan Larsen wrote:
>> On 15 feb 2012, at 20:03, Paul Hohensee wrote:
>> 
>>> On 2/15/12 1:58 PM, Staffan Larsen wrote:
>>>> 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.
>>> globalDefinitions_gcc.hpp is where these sorts of #includes currently live,
>>> so I'd go with present practice rather than doing something new.  If
>>> you want to put the include of mach.h in os_bsd.inline.hpp, then you
>>> should do something similar with all the platform-dependent includes
>>> in globalDefinitions_gcc.hpp.
>> I'd actually like to have it in osThread_bsd.hpp, but that isn't possible. I'll put in gloabalDefinitions_gcc.hpp.
>> 
>> 
>>>>> 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.
>>> Even so, if we ever need to make hotspot work on non-osx bsd platforms, we'll
>>> at least have markers to guide the work.
>> OK - I'll try to fix things up.
>> 
>> /Staffan
>> 
>>> Paul
>>> 
>>>> /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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20120216/a079c041/attachment-0001.html