RFE: cleanup jlong typedef on __APPLE__ and _LP64 systems

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.