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

[Daniel D. Daugherty]

On 2/16/12 7:09 AM, Staffan Larsen wrote:
> New webrev: http://cr.openjdk.java.net/~sla/7132070/webrev.01/ 
> <http://cr.openjdk.java.net/%7Esla/7132070/webrev.01/>

Thumbs up!


src/cpu/x86/vm/vm_version_x86.cpp
     No comments.

src/os/bsd/vm/osThread_bsd.cpp
     No comments.

src/os/bsd/vm/osThread_bsd.hpp
     No comments.

src/os/bsd/vm/os_bsd.cpp
     line 5151 - it might be more clear to rename the local 'mach_thread'
         to something else. Yes, the underlying type is a "mach thread",
         but at this point, you've already abstracted to a thread_t.

     No other comments.

src/os_cpu/bsd_x86/vm/vmStructs_bsd_x86.hpp
     No comments.

src/share/vm/utilities/globalDefinitions_gcc.hpp
Please update copyright year to 2012.
     No other comments.


>
> 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/ 
>>>>>>>>>> <http://cr.openjdk.java.net/%7Esla/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
>