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
