Merging the MacOS X Port into HotSpot Express 23 (7098194)
Daniel D. Daugherty
daniel.daugherty at oracle.com
Wed Oct 12 15:33:03 PDT 2011
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.
> 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...
> 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