Merging the MacOS X Port into HotSpot Express 23 (7098194)
Paul Hohensee
paul.hohensee at oracle.com
Wed Oct 12 15:47:36 PDT 2011
On 10/12/11 6:33 PM, Daniel D. Daugherty wrote:
> Paul,
>
> Thanks for the thorough review. Replies embedded below.
>
> On 10/12/11 2:56 PM, Paul Hohensee wrote:
>> The printf format thing is handled in globalDefinitions.hpp via
>> system-dependent
>> macros named INTX_FORMAT, PTR_FORMAT, etc. There should be no direct
>> uses of
>> printf format specifiers in Hotspot code, so the change to jni_x86.h
>> should go
>> away and any uses of %lld and %ld should be replaced by the
>> appropriate FORMAT
>> macro. If needed, you can use the platform-specific extensions to
>> globalDefinitions.hpp
>> in the cpu and utilities directories.
>
> Based on what Roger said, I suspect that if I remove the change to
> jni_x86.h (typedef long long jlong on __APPLE__), then I'll have
> some non-trivial amount of warnings fallout that I'll need to
> deal with. I'm willing to do that, but not right now and not with
> this changeset. The clock is ticking on getting this changeset
> pushed and I don't consider this a showstopper.
Ok. Another RFE.
>
>
>> If set_native_thread_name() is an implementation of an extension to
>> Thread,
>> then I'd recommend leaving it and the supporting code out (and
>> there's a lot of
>> it, vis jvm.cpp as you point out) until the core libs people figure
>> out how
>> they want to handle it. Maybe file an RFE for the work.
>
> If I remove that support, then I won't be able to use the resulting
> VM with the rest of the macosx-port forest, right? Part of my
> bring up strategy relies on my being able to use the HSX-23 port
> bits with the rest of the macosx-port built bits...
True. But it may not be used in it's current form.
Thanks,
Paul
>
>
>> In debug.cpp, I'd make Debugging a product flag if you're going to
>> make Command
>> a product class. That way you don't need the #ifndef PRODUCT around
>> its uses.
>
> Done.
>
>
>> There's a use of "%#p" at line 481 in the Old file (487 in the New
>> one) that should
>> be replaced by PTR_FORMAT.
>
> Fixed.
>
>
>> The comment on line 496 of the New version should
>> be "!PRODUCT" rather than "PRODUCT".
>
> Fixed there and in other places in the file.
>
>
>> You might change pss() to read
>>
>> 577 extern "C" void pss() { // print all stacks
>> 578 if (!Thread::current()) return;
>> 579 Command c("pss");
>> 581 Threads::print(true, PRODUCT_ONLY(false) NOT_PRODUCT(true));
>> 585 }
>>
>>
>> It's perhaps a bit neater.
>
> Fixed the print() call on line 581.
>
>
>> Re "!Thread::current()", the rule in hotspot is to make
>> comparisons to null explicit, so you could use instead
>> "Thread::current() != NULL",
>> because what NULL is varies by platform.
>
> Fixed both !current() uses in the file.
>
> Dan
>
>
>>
>> Paul
>>
>> On 10/12/11 2:34 PM, roger hoover wrote:
>>> As I promised Dan earlier this morning....replies embedded:
>>>
>>> On Oct 12, 2011, at 9:48 AM, Daniel D. Daugherty wrote:
>>>
>>>> David,
>>>>
>>>> Thanks for the thorough review (as always). Replies embedded below.
>>>>
>>>>
>>>> On 10/11/11 10:48 PM, David Holmes wrote:
>>>>> Hi Dan,
>>>>>
>>>>> src/cpu/x86/vm/jni_x86.h
>>>>>
>>>>> I don't understand why this change would be needed:
>>>>>
>>>>> ! #if defined(_LP64)&& !defined(__APPLE__)
>>>>> typedef long jlong;
>>>>> #else
>>>>> typedef long long jlong;
>>>>> #endif
>>>>>
>>>>> this is saying that on "apple" under _LP64 a long is not 64-bits.
>>>>> But the very definition of LP64 is that longs and pointers are
>>>>> 64-bits. ???
>>>> $ cat sizes.c
>>>> #include<stdio.h>
>>>>
>>>> int main() {
>>>> printf("Hello world! _LP64=");
>>>> #ifdef _LP64
>>>> printf("%d\n", _LP64);
>>>> #else
>>>> printf("undefined\n");
>>>> #endif
>>>> printf("sizeof(long) = %d\n", (int) sizeof(long));
>>>> printf("sizeof(long long) = %d\n", (int) sizeof(long long));
>>>> }
>>>>
>>>> Output on my MacOS 10.6.8 machine:
>>>>
>>>> Hello world! _LP64=1
>>>> sizeof(long) = 8
>>>> sizeof(long long) = 8
>>>>
>>>> Output on my Solaris X86 machine:
>>>>
>>>> Hello world! _LP64=undefined
>>>> sizeof(long) = 4
>>>> sizeof(long long) = 8
>>>>
>>>> Output on my Solaris X86 machine when built with '-m64':
>>>>
>>>> Hello world! _LP64=1
>>>> sizeof(long) = 8
>>>> sizeof(long long) = 8
>>>>
>>>> Since __APPLE__ machines have a 8 byte long, I don't understand
>>>> why the person thought that "long long jlong" was necessary...
>>>>
>>>> We need an Apple person to jump in here...
>>>>
>>> (answered before, but here it is again):
>>> This is likely due to the fprintf format usage mess. The hotspot
>>> code had assumed that any 64-bit fprintf format could be used with
>>> any 64-bit value. Apple compilers give warnings if you print a long
>>> with "%lld", and insists upon "%ld" for a clean compile even though
>>> both are 64-bit values. Changing the type to long long allows the
>>> format to remain unchanged.
>>>
>>> The correct way to fix this mess is to insure that the formats
>>> exactly match the types used. We couldn't pull this off at apple
>>> since we didn't have the capabilities to build under all of the
>>> other os platforms to test the changes.
>>>
>>>>> src/os/<os>/vm/os_<os>.cpp
>>>>>
>>>>> + void os::set_native_thread_name(const char *name) {
>>>>> + // Not yet implemented.
>>>>> + return;
>>>>> + }
>>>>>
>>>>> I hate seeing "shared" code that is not implemented on 3 out of 4
>>>>> supported platforms. Though it seems Linux will also support
>>>>> pthread_setname_np as of March 2010 (not sure which kernel or
>>>>> glibc versions needed). If not for the fact this also adds an
>>>>> exported JVM method to set the native thread name, we could
>>>>> otherwise bury this in the platform specific native thread
>>>>> creation code (which might also obviate the need for the new
>>>>> _is_attached logic - see next point). I also wonder what Java code
>>>>> is using this new JVM API?
>>>> I suspect that this new API is used by other pieces of the MacOS X
>>>> port project, but I don't know that for sure. Again, we need an
>>>> Apple person to jump in here.
>>> The native thread name addition was a huge win, both for us
>>> debugging and for our developer customers. The only complaint we
>>> had was from other projects whose code attached to Java and (in
>>> initial revisions) had their threads renamed. It is exported so
>>> that Thread.java can set the native thread name.
>>>
>>> I have no explanation for the lack of a return code. I can only
>>> surmise that it is an oversight that was not caught by the compiler.
>>>
>>> The other pieces are:
>>> macosx-port/jdk/src/share/native/java/lang/Thread.c:
>>> {"getThreads", "()[" THD, (void *)&JVM_GetAllThreads},
>>> {"dumpThreads", "([" THD ")[[" STE, (void
>>> *)&JVM_DumpThreads},
>>> --> {"setNativeName", "(" STR ")V", (void
>>> *)&JVM_SetNativeThreadName},
>>> };
>>> -and-
>>> macosx-port/jdk/src/share/classes/java/lang/Thread.java
>>> /**
>>> * Changes the name of this thread to be equal to the argument
>>> *<code>name</code>.
>>> *<p>
>>> * First the<code>checkAccess</code> method of this thread is
>>> called
>>> * with no arguments. This may result in throwing a
>>> *<code>SecurityException</code>.
>>> *
>>> * @param name the new name for this thread.
>>> * @exception SecurityException if the current thread cannot
>>> modify this
>>> * thread.
>>> * @see #getName
>>> * @see #checkAccess()
>>> */
>>> public final void setName(String name) {
>>> checkAccess();
>>> this.name = name.toCharArray();
>>> --> if (threadStatus != 0) {
>>> --> setNativeName(name);
>>> --> }
>>> }
>>>>
>>>>> Also wondering where the "current thread only" restriction came
>>>>> about?
>>>> If the API is restricted to the "current thread", then we don't have
>>>> to worry about races and locking. Also, I'm wondering why there is no
>>>> return code.
>>>>
>>>>
>>>>> src/share/vm/runtime/thread.hpp
>>>>>
>>>>> + // Remember whether or not we were attached
>>>>> + bool _is_attached;
>>>>>
>>>>> It took me a while to realize that _is_attached really means "was
>>>>> attached" ie that the Java thread came about due to a native
>>>>> thread attaching to the VM via JNI attachCurrentThread. Could I
>>>>> suggest this is renamed to was_attached?
>>>> I was taking another look at this. The key user of the new flag is:
>>>>
>>>> src/share/vm/prims/jvm.cpp
>>>>
>>>> JVM_ENTRY(void, JVM_SetNativeThreadName(JNIEnv* env, jobject
>>>> jthread, jstring n
>>>> ame))
>>>> JVMWrapper("JVM_SetNativeThreadName");
>>>> ResourceMark rm(THREAD);
>>>> oop java_thread = JNIHandles::resolve_non_null(jthread);
>>>> JavaThread* thr = java_lang_Thread::thread(java_thread);
>>>> // Thread naming only supported for the current thread, doesn't
>>>> work for
>>>> // target threads.
>>>> if (Thread::current() == thr&& !thr->is_attached()) {
>>>> // we don't set the name of an attached thread to avoid stepping
>>>> // on other programs
>>>> const char *thread_name =
>>>> java_lang_String::as_utf8_string(JNIHandles::reso
>>>> lve_non_null(name));
>>>> os::set_native_thread_name(thread_name);
>>>> }
>>>> JVM_END
>>>>
>>>> which is coded to not set the name of a thread that has been
>>>> attached. In the JavaThread(bool) constructor:
>>>>
>>>> _is_attached = _is_attaching = is_attaching;
>>>>
>>>> we set both flags to the "is_attaching" parameter (default false)
>>>> so if we're coming down the JNI attach path, that parameter
>>>> should be true so we set both flags to true. After the thread
>>>> has been properly attached, we call set_attached() which sets
>>>> the _is_attaching flag to false. Whew!
>>>>
>>>> In the JavaThread(ThreadFunction, size_t) constructor we set
>>>> both flags to false because we're not attaching... and we
>>>> didn't come down the JNI attach path.
>>>>
>>>> I don't like the "was attached" attribute and I no longer like
>>>> the "is attaching" and "is attached" attributes either. Sigh.
>>>> I think both fields need to be cleaned up and renamed. Perhaps
>>>> something like:
>>>>
>>>> is_attaching_via_jni
>>>> was_attached_via_jni
>>>>
>>>> would be more clear?
>>>>
>>>>
>>>>> src/share/vm/utilities/debug.cpp
>>>>>
>>>>> Why are we exposing this debug code in a product build?
>>>> We'll need an Apple person to jump in here...
>>>>
>>>>
>>> It has been an Apple tradition (from long before I joined the Java
>>> team in 2004) to include a basic set of debugging routines in
>>> product builds. We call them from gdb while debugging problems that
>>> don't show up in debug builds, and we've told our developer
>>> customers about them. The rest of the Apple port does not depend on
>>> these and whether or not to continue Apple's tradition is totally up
>>> to Oracle.
>>>
>>>>> src/share/vm/prims/jni.cpp
>>>>>
>>>>> The USDT2 ifdef changes are truly ugly especially in this file. Is
>>>>> there no way to hide USDT1 vs USDT2 at a lower level so that the
>>>>> top-level probe points are not affected? I know this is flagged
>>>>> for "future work" but history shows such cleans ups rarely if ever
>>>>> happen - and this current code really is pretty awful to look at.
>>>> Yes, the #ifdef'ing makes the current code awful. I don't think
>>>> anyone will dispute that observation.
>>>>
>>>> The better solution would be for Solaris to migrate to USDT2.
>>>> I'm discussing that with Keith McGuigan. However, I don't want
>>>> to take much of his time right now because he is hip deep in
>>>> another alligator swamp at the moment.
>>>>
>>>> The best I can offer at the moment is filing a new bug to track
>>>> the future work. Yes, I know that clean up type work rarely gets
>>>> done, but this merge with the MacOS X port project needs to get
>>>> into HSX-23 as soon as possible. Broken functionality in the
>>>> other platforms is a stopper, but ugly is not a stopper.
>>>>
>>>> Thanks again for the thorough review.
>>>>
>>>> Dan
>>>>
>>>>
>>>>> Cheers,
>>>>> David
>>>>>
>>>>> On 12/10/2011 3:34 AM, Daniel D. Daugherty wrote:
>>>>>> Greetings,
>>>>>>
>>>>>> Tom R shepherded the the BSD Port into HotSpot Express 23 using:
>>>>>>
>>>>>> 7089790 integrate bsd-port changes
>>>>>>
>>>>>> and I'm following up on that work using the following bug:
>>>>>>
>>>>>> 7098194 integrate macosx-port changes
>>>>>>
>>>>>> The synopsis is a bit off. In reality, the 7098194changeset will
>>>>>> include additional changes from the BSD Port in addition the deltas
>>>>>> made by the MacOS X Port. The bsd-port/hotspot tip changeset for
>>>>>> this resync is:
>>>>>>
>>>>>> changeset: 2729:f1a18ada5853
>>>>>> tag: tip
>>>>>> user: Greg Lewis<glewis at eyesbeyond.com>
>>>>>> date: Wed Sep 21 22:29:10 2011 -0700
>>>>>> summary: . Finish removing hsearch_r.
>>>>>>
>>>>>> The macosx-port/hotspot changeset for this merge is:
>>>>>>
>>>>>> changeset: 2756:69de8d34a370
>>>>>> tag: tip
>>>>>> user: swingler at apple.com
>>>>>> date: Thu Sep 29 18:10:16 2011 -0700
>>>>>> summary: Adding JAVA_LIBRARY_PATH for bundled app launching
>>>>>> (avoids stomping DYLD_LIBRARY_PATH)
>>>>>>
>>>>>> The focus of 7098194 is to get the MacOS X port into HSX-23 without
>>>>>> regressing the non-MacOS X platforms. In other words, we're trying
>>>>>> to get HSX-23 caught up with the BSD Port and MacOS X Port projects.
>>>>>> Shaking out the MacOS X Port itself will be done with other
>>>>>> changesets
>>>>>> on an as needed basis.
>>>>>>
>>>>>> Just to be clear, the push target for this changeset is the
>>>>>> RT_Baseline
>>>>>> sub-repo of HotSpot Express (currently HSX-23):
>>>>>>
>>>>>> http://hg.openjdk.java.net/hsx/hotspot-rt/hotspot
>>>>>>
>>>>>> Like what Tom did for 7089790, I'm posting multiple webrevs so folks
>>>>>> can review these changes in different ways. My primary focus here is
>>>>>> the common or shared code so I'm less worried about the BSD or
>>>>>> MacOS X
>>>>>> specific changes. Obviously, if you see something I messed up in
>>>>>> those
>>>>>> files, I'd like to know.
>>>>>>
>>>>>> Here's the webrev for all the changes in one shot:
>>>>>>
>>>>>> http://cr.openjdk.java.net/~dcubed/mac_os_x_port/7098194-webrev/0-all/
>>>>>>
>>>>>>
>>>>>> The order of the files in the above webrev is the same as for
>>>>>> for the subset webrevs below.
>>>>>>
>>>>>> Here's the webrevs for the changes in subsets:
>>>>>>
>>>>>> http://cr.openjdk.java.net/~dcubed/mac_os_x_port/7098194-webrev/0-bsd-resync/
>>>>>>
>>>>>>
>>>>>>
>>>>>> The above webrev has the changes to the bsd-port/hotspot repo made
>>>>>> after Tom R's work on 7089790 (the 2011.08.02 wiki snapshot).
>>>>>>
>>>>>> http://cr.openjdk.java.net/~dcubed/mac_os_x_port/7098194-webrev/0-macosx-other-code/
>>>>>>
>>>>>>
>>>>>>
>>>>>> The above webrev has the non-Dtrace and non-infrastructure changes
>>>>>> made for the MacOS X port. There's a couple of files that also
>>>>>> contain
>>>>>> Dtrace related changes, but I decided it was better to include those
>>>>>> files in this subset.
>>>>>>
>>>>>> http://cr.openjdk.java.net/~dcubed/mac_os_x_port/7098194-webrev/0-macosx-dtrace-code/
>>>>>>
>>>>>>
>>>>>>
>>>>>> The above webrev has almost all of the changes to enable Dtrace on
>>>>>> MacOS X (a couple of files are in the macosx-other-code subset).
>>>>>> On MacOS X, a newer version of Dtrace is implemented than on Solaris
>>>>>> which is why the code is #ifdef'ed. I had to change the original
>>>>>> #ifdef'ing because the original implementation had put some
>>>>>> non-Dtrace
>>>>>> code into #ifdef USDT1 ... #endif blocks so the code didn't build on
>>>>>> non-Solaris platforms. In order to be more clean with #ifdef'ing, I
>>>>>> redid all MacOS X Dtrace #ifdef'ing in the following forms:
>>>>>>
>>>>>> #ifndef USDT2
>>>>>> <older Dtrace implementation is in this block>
>>>>>> <some non-Dtrace code (macros) that call the older Dtrace>
>>>>>> <implementation are also in this block>
>>>>>> #else /* USDT2 */
>>>>>> <new Dtrace implementation for MacOS X in this block>
>>>>>> #endif /* USDT2 */
>>>>>>
>>>>>> #ifndef USDT2
>>>>>> <older Dtrace implementation is in this block>
>>>>>> <no #else because the newer Dtrace doesn't need/have the equivalent>
>>>>>> #endif /* ! USDT2 */
>>>>>>
>>>>>> Yes, I realize that the MacOS X Dtrace implementation does not
>>>>>> follow
>>>>>> HotSpot style guidelines very consistently, but I decided not to fix
>>>>>> that so I could diff against the macosx-port/hotspot more reliably.
>>>>>>
>>>>>> I have to consult with Keith McGuigan about how to migrate the
>>>>>> Solaris
>>>>>> Dtrace implementation to the newer version. However, that work
>>>>>> will be
>>>>>> done independently of this bug (7098194).
>>>>>>
>>>>>> http://cr.openjdk.java.net/~dcubed/mac_os_x_port/7098194-webrev/0-macosx-infra/
>>>>>>
>>>>>>
>>>>>>
>>>>>> The above webrev has all the infrastructure (e.g., Makefile) related
>>>>>> changes. Many/most folks aren't interested in Makefile stuff so I've
>>>>>> put these changes in their own subset. Of course, I need Kelly
>>>>>> O'Hair
>>>>>> to bless these changes...
>>>>>>
>>>>>> Builds done so far:
>>>>>>
>>>>>> - Solaris X86 builds of {Client, Server} VM * {product,
>>>>>> fastdebug, jvmg}
>>>>>> - WinXP builds of {Client, Server} VM * {product, fastdebug, debug}
>>>>>> - MacOS 10.6.8/Xcode 3.2.6 build of the macosx-port forest with
>>>>>> new HSX
>>>>>> repo
>>>>>> - MacOS 10.7/Xcode 4.1.1 build of the macosx-port forest with new
>>>>>> HSX repo
>>>>>>
>>>>>> Testing done so far:
>>>>>>
>>>>>> - Serviceability stack (25 subsuites):
>>>>>> VM/NSK: jvmti, jvmti_unit, jdwp, jdi, sa-jdi, heapdump, hprof, jdb,
>>>>>> logging, mm
>>>>>> SDK/JDK: jdi, jdi_closed, jli, logging, mm_csjmx, mm_csm, mm_jlm,
>>>>>> mm_jlm_closed, mm_jmx, mm_jmx_closed, mm_sm, mm_sm_closed,
>>>>>> misc_attach, misc_jvmstat, misc_tools
>>>>>> - Tested configs (8 configs)
>>>>>> {Solaris X86, WinXP} * {Client, Server} VM * {-Xmixed, -Xcomp}
>>>>>>
>>>>>> - MacOS X builds have only had minimal 'java -version' type testing,
>>>>>> i.e., did the build "work"?
>>>>>>
>>>>>> No regressions have been seen in the Solaris X86 testing and WinXP
>>>>>> testing should be finished later today.
>>>>>>
>>>>>> Things still to do (in no particular order):
>>>>>>
>>>>>> - gather the list of contributors for the changeset comment
>>>>>> - JPRT test jobs when the JPRT-hotspotwest queue settles down
>>>>>> - dtrace testing on Solaris X86
>>>>>> - code review
>>>>>> - start bring up of more formal dev testing on MacOS X
>>>>>>
>>>>>> Thanks, in advance, for any review comments.
>>>>>>
>>>>>> Dan
>>>>>>
>>>
More information about the hotspot-dev
mailing list