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

[David Holmes]

Hi Paul,

On 19/10/2011 2:17 AM, Paul Hohensee wrote:
> 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.

The important thing is that the types and format specifiers always 
match. So given we have (and should have in my view) int's and "long 
long"'s then we should use %d and %lld respectively as the correct 
format specifiers. If we were to have int32_t, int64_t etc then we would 
need INT32_T_FORMAT etc.

I think it would be wrong to change from using the basic C int type to 
using int32_t for example as that would cause problems trying to 
build/run on ILP64 systems for example. I think the current code assumes 
a well-defined execution environment such as ILP32 (or more strictly 
ILP32LL), LP64 or even ILP64 (though the latter is untested AFAIK).

Any platform that had an int type smaller than 32-bits would require a 
complete rewrite of the VM in my view due to the fact that Java's int 
type must be 32-bits and must be accessed atomically.

David
-----


> 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
>>>>>>>>
>>>>>>>>