Merging the MacOS X Port into HotSpot Express 23 (7098194)

[Paul Hohensee]

Just getting to this (was on vacation last Th thru Sun and have been
wading through email).

I actually think that directly using C/C++ integral types whose sizes are
implementation-defined isn't a good idea, though others disagree.  If
it were up to me, we wouldn't use 'int' or 'unsigned', but rather one of
uint32_t, etc.  Longs vary more across implementations than int, but
even ints can be different sizes on different platforms.  Hotspot has
built in the assumption that ints are 32-bit everywhere.

Anyway, that's why I said there shouldn't be direct uses of printf format
specifiers in Hotspot code.  I definitely misspoke wrt %s, but I'd like %d
to go away too.  In this case, the other reviewers are right about not using
INT32_FORMAT, because doing so assumes that int's are 32 bits, which
it shouldn't.

Paul

On 10/13/11 5:53 PM, David Holmes wrote:
> On 14/10/2011 3:34 AM, Daniel D. Daugherty wrote:
>> On 10/13/11 11:32 AM, Tom Rodriguez wrote:
>>> On Oct 13, 2011, at 7:07 AM, Daniel D. Daugherty wrote:
>>>
>>>> Changing the format string from "%d" to INT32_FORMAT was done
>>>> because Paul pointed out that HotSpot isn't supposed to use
>>>> format specifiers like "%d" directly.
>>> I don't think that's true. %d is the format for the int type and is
>>> broadly used in hotspot. Replacing %d with INT32_FORMAT seems more
>>> obfuscating than anything. The _FORMAT types are mainly there to deal
>>> with the inconsistent handling of longs and pointer sized values in
>>> format strings not to hide all formats. Personally I would eradicate
>>> INT32_FORMAT from the source since it doesn't add value and is rarely
>>> used.
>>
>> I'll let you and Paul hash that out when he gets back from vacation.
>
> Paul mis-spoke when he said "There should be no direct uses of
> printf format specifiers in Hotspot code," as the code is absolutely 
> full of them, as you would expect. We should always use %d for int and 
> jint types, %ld for longs, and should only need macros for typedef'd 
> types (pointers, jlongs, potentially unsigned values) that might be 
> different on different platforms/compilers..
>
> On a different note, there was no runtime snapshot this week as there 
> were no changes. Next snapshot will be next week as per the regular 
> schedule.
>
> David
> -----
>
>> Dan
>>
>>
>>> tom
>>>
>>>> That is different than changing the output from a decimal to
>>>> string which I think is beyond the scope of this changeset. It
>>>> does raise interesting questions about error diagnostics and,
>>>> if memory serves me correctly, there is an open bug for improving
>>>> error diagnostics...
>>>>
>>>> And, yes, I'll have to check the equivalent Linux code to see if
>>>> the format strings need to be fixed there also. I suspect they do,
>>>> but I already have plans (and a script) to make a pass at sync'ing
>>>> BSD improvements back to Linux.
>>>>
>>>>
>>>>> src/share/vm/prims/jni.cpp + thread.*
>>>>>
>>>>> ! thread->set_done_attaching_via_jni();
>>>>>
>>>>> I think the naming has gotten overly verbose here. The only way to
>>>>> attach is by definition via JNI. I don't think there was anything
>>>>> wrong with the existing _is_attaching logic (yes I added it! ;-) )
>>>>> and my only complaint was that the new _is_attached should really be
>>>>> _was_attached. Perhaps a better/simpler/cleaer approach here would
>>>>> have been to have a field: can_set_native_thread_name, and only set
>>>>> it to true in the non-attaching case. Another cleanup RFE perhaps.
>>>> On several occasions I have fielded questions about the difference
>>>> between "attach on demand" and "JNI attach" so there is definitely
>>>> confusion out there. I think the renaming adds clarity and I had
>>>> thought you would like the reduction in number of fields. I also
>>>> think the new JNIAttachStates is cleaner and more clear. I think
>>>> that adding a field called can_set_native_thread_name is going to
>>>> cause some confusion with JVM/TI capabilities since all of those
>>>> are named in the style of can_do_this and can_do_that.
>>>>
>>>>
>>>>> src/share/vm/utilities/debug.cpp
>>>>>
>>>>> Normally if we propose to move something into a product build there
>>>>> is a discussion about what we are moving, why, and what the impact
>>>>> is on build size and runtime performance. I don't see any of that
>>>>> here. :(
>>>> I've only recently joined the MacOS Port project so I have no idea
>>>> what was discussed before I arrived. I have difficulty imagining
>>>> that the changes to debug.cpp will add much to the size and I don't
>>>> think they will affect runtime performance unless they are used.
>>>> Compared to what I just added via Full Debug Symbols, this has got
>>>> to be a drop in the bucket :-)
>>>>
>>>>
>>>>> - set_native_thread_name:
>>>>>
>>>>> Ok so I have to go back to earlier emails here. Makes sense that
>>>>> Thread.setName is augmented to call JVM_SetNativeThreadName. However
>>>>> I don't like the semantics of this particular addition. setName has
>>>>> no usage restrictions on it, but set_native_thread_name is a no-op
>>>>> unless called by the current thread. By restricting it to the
>>>>> current thread it simplifies synchronization logic, but it seems a
>>>>> somewhat arbitrary restriction in terms of the functionality, given
>>>>> the more general nature of the setName API. I guess this is Yet
>>>>> Another RFE: set_native_thread_name should not be restricted to only
>>>>> the current thread.
>>>> We definitely need to get some clarity about the addition of
>>>> JVM_SetNativeThreadName(). I'm not fond of APIs without return
>>>> values so that's one thing to log in the new bug. The "must be
>>>> current thread" semantic should also be hashed out/clarified.
>>>>
>>>> Like any API between the JDK and HotSpot, changing it will be a
>>>> pain and require coordination.
>>>>
>>>>> BTW: are the JDK changes associated with this also being pushed at
>>>>> some stage?
>>>> Yes, but the exact process has not been finalized. Since there
>>>> are changes being made/coordinated by multiple teams, I'm not
>>>> sure which JDK8 sub-repos will be used.
>>>>
>>>>
>>>>> If so are the hotspot changes being pushed first?
>>>> I'm planning to push this changeset to RT_Baseline today. I don't
>>>> know if there is an HSX-B23-B02 snapshot this week, but I think
>>>> I've missed the window by one day. Normally changes have to hit
>>>> RT_Baseline by Wednesday nightly cut-off to be included in the
>>>> RT_Baseline -> Main_Baseline push for a given week...
>>>>
>>>> However, I don't know your plans for gatekeeping this week so
>>>> maybe you aren't planning to push until Friday morning.
>>>>
>>>> Strangely enough, because I'm pushing to HSX-23 via RT_Baseline,
>>>> my repo coordination life is much easier. HSX-23 will get to
>>>> JDK7u via a bulk update so I don't have to juggle too much...
>>>>
>>>> Dan
>>>>
>>>>
>>>>
>>>>
>>>>> Cheers,
>>>>> David
>>>>>
>>>>> On 13/10/2011 10:56 AM, Daniel D. Daugherty wrote:
>>>>>> Greetings,
>>>>>>
>>>>>> I've closed out Code Review Round 0 and I'm posting new webrevs
>>>>>> for Code Review Round 1:
>>>>>>
>>>>>> Here's the webrev for just the deltas between Code Review Round 0
>>>>>> and Code Review Round 1:
>>>>>>
>>>>>> http://cr.openjdk.java.net/~dcubed/mac_os_x_port/7098194-webrev/1-deltas-only 
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> Here's the webrev for all the changes in one shot:
>>>>>>
>>>>>> http://cr.openjdk.java.net/~dcubed/mac_os_x_port/7098194-webrev/1-all/ 
>>>>>>
>>>>>>
>>>>>> And here are the same subset webrevs:
>>>>>>
>>>>>> http://cr.openjdk.java.net/~dcubed/mac_os_x_port/7098194-webrev/1-bsd-resync/ 
>>>>>>
>>>>>>
>>>>>>
>>>>>> http://cr.openjdk.java.net/~dcubed/mac_os_x_port/7098194-webrev/1-macosx-other-code/ 
>>>>>>
>>>>>>
>>>>>>
>>>>>> http://cr.openjdk.java.net/~dcubed/mac_os_x_port/7098194-webrev/1-macosx-dtrace-code/ 
>>>>>>
>>>>>>
>>>>>>
>>>>>> http://cr.openjdk.java.net/~dcubed/mac_os_x_port/7098194-webrev/1-macosx-infra/ 
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> Here's my current commit message:
>>>>>>
>>>>>>> 7098194: integrate macosx-port changes
>>>>>>> Summary: Integrate macosx-port/hotspot changes as of 2011.09.29.
>>>>>>> Reviewed-by: kvn, dholmes, never, phh
>>>>>>> Contributed-by: Christos Zoulas<christos at zoulas.com>, Greg Lewis
>>>>>>> <glewis at eyesbeyond.com>, Kurt Miller<kurt at intricatesoftware.com>,
>>>>>>> Alexander Strange<astrange at apple.com>, Mike Swingler
>>>>>>> <swingler at apple.com>, Roger Hoover<rhoover at apple.com>
>>>>>> Thanks, in advance, for any final feedback for this changeset.
>>>>>>
>>>>>> Dan
>>>>>>
>>>>>>
>>>>>>> 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
>>>>>>>
>>>>>>>