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
