JDK-7102489 : RFE: cleanup jlong typedef on __APPLE__ and _LP64 systems
  • Type: Enhancement
  • Component: hotspot
  • Sub-Component: runtime
  • Affected Version: hs23
  • Priority: P3
  • Status: Closed
  • Resolution: Fixed
  • OS: os_x
  • CPU: generic
  • Submitted: 2011-10-18
  • Updated: 2017-05-24
  • Resolved: 2013-01-17
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 8 Other Other
8Fixed hs25Fixed hs25,openjdk7uFixed
Related Reports
Duplicate :  
Relates :  
Description
The MacOS X Port project used the following changeset for the initial
push from macosx-port/hotspot -> HSX-23:

    http://hg.openjdk.java.net/hsx/hotspot-rt/hotspot/rev/436b4a3231bf

    7098194: integrate macosx-port changes

and that changeset includes a platform specific typedef for jlong.


David H made the following code review comment:

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


Here is my reply to David H:

On 10/12/11 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... 


Here is Roger H's reply to David H's comment:

On 10/12/11 9:59 AM, roger h wrote:
> 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.
>
> roger


Here is a comment from Paul H on the thread:

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


My reply to Paul H:

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


Paul's reply to me:

> Ok.  Another RFE. 


A comment from me to David H about formats:

>> src/os_cpu/bsd_zero/vm/os_bsd_zero.cpp (and probably elsewhere)
>> 
>> fatal(err_msg("pthread_attr_init failed with err = " INT32_FORMAT, rslt));
>> 
>> It is far more informative to provide the error string "strerror(rslt)"
>> than the imal value of the error code. (We're probably guilty of this
>> in many places but lets not propagate bad habits.)
> 
> 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.


A reply to my comment from Tom R:

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


My reply ro Tom R:

> I'll let you and Paul hash that out when he gets back from vacation.


Roger H's reply on this part of the thread:

> The Apple problem with macros like INT32_FORMAT is that they were
> being used to print all 32-bit values without respect to the
> underlying type.  For example, on 32-bit builds, long int was
> being printed with INT32_format.  Printing a 32-bit long int with
> "%d" instead of "%ld" produces a warning on Apple compilers.


David H's reply to my reply:

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


John R's comment on this thread:

> I agree about %d, %s, etc.
>
> But let's make a distinction about %ld.  Naked use of the C type
> "long" is a portability trap, and should not be in our code unless
> there is strong requirement.  (The old style pages say something
> about this, FWIW.)
>
> In Hotspot code there are approximately 30 uses of print format
> strings of the form %ld.  That qualifies as a rarity, compared
> with the corresponding 2000 uses of the string %d.  (See below.)
> Let's continue to make "long" and "%ld" be rare in our source base.
>
> -- John
>
> --------
> grep -n 'print.*%ld' $(hg loc -I src) | wc
>       34     255    4004
> --------
> grep -n 'print.*%d' $(hg loc -I src) | wc
>     2086   15753  236821
> --------
>


Reply from David H to John R:

> Yes you are absolutely right. We shouldn't be using
> "long" types in the first place. 


And finally, Paul H is back...

On 10/18/11 10: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.
>
> 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.
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.

Comments
The jni_x86.h code needs changing, but first, we need to fix the %ld and %lld format statements to be more precise and use the appropriate macros, and ensure that they work correctly on all of our supported platforms.
16-11-2012