Uploaded image for project: 'JDK'
  1. JDK
  2. JDK-7102489

RFE: cleanup jlong typedef on __APPLE__ and _LP64 systems

    Details

    • Subcomponent:
    • Resolved In Build:
      b17
    • CPU:
      generic
    • OS:
      os_x

      Backports

        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.

          Issue Links

            Activity

            Hide
            acorn Karen Kinnear added a comment -
            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.
            Show
            acorn Karen Kinnear added a comment - 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.
            Hide
            hgupdate HG Updates added a comment -
            URL: http://hg.openjdk.java.net/hsx/hotspot-rt/hotspot/rev/203f64878aab
            User: hseigel
            Date: 2013-01-17 18:49:45 +0000
            Show
            hgupdate HG Updates added a comment - URL: http://hg.openjdk.java.net/hsx/hotspot-rt/hotspot/rev/203f64878aab User: hseigel Date: 2013-01-17 18:49:45 +0000
            Hide
            hgupdate HG Updates added a comment -
            URL: http://hg.openjdk.java.net/hsx/hsx25/hotspot/rev/203f64878aab
            User: amurillo
            Date: 2013-01-25 20:42:57 +0000
            Show
            hgupdate HG Updates added a comment - URL: http://hg.openjdk.java.net/hsx/hsx25/hotspot/rev/203f64878aab User: amurillo Date: 2013-01-25 20:42:57 +0000

              People

              • Assignee:
                hseigel Harold Seigel
                Reporter:
                dcubed Daniel Daugherty
              • Votes:
                0 Vote for this issue
                Watchers:
                6 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Imported:
                  Indexed: