Request for review: 7102489: RFE: cleanup jlong typedef on __APPLE__ and _LP64 systems
David Holmes
david.holmes at oracle.com
Thu Jan 17 03:14:41 PST 2013
On 17/01/2013 7:51 AM, harold seigel wrote:
> Hi David,
>
> Thanks for your comments. Please see my interspersed comments.
>
> Also, I posted a modified webrev at
> http://cr.openjdk.java.net/~hseigel/bug_7102489_2/
> <http://cr.openjdk.java.net/%7Ehseigel/bug_7102489_2/> that has 32 and
> 64 definitions of JLONG_FORMAT in .../launcher/java_md.h and 2013
> copyrights.
So ... I don't see where java_md.c actually needs the format specifier
that was defined in java_md.h ??
>>> I would really be worried about changing the definition in hotspot of
>>> jlong. That seems very risky.
>>
>> No risk. jlong is a 64-bit signed value regardless. And int64_t will
>> be either long or "long long" as appropriate.
> I tried changing the type of jlong to int64_t and encountered
> compilation errors on 64 bit Linux, because I had added "#include
> <stdint.h>" to jni_x86.h. The header file was needed because it
> contains the definition of int64_t. An example error was:
>
> .../src/share/vm/memory/allocation.hpp:
> In member function void Arena::check_for_overflow(size_t, const char*) const:
> .../src/share/vm/memory/allocation.hpp:340:
> error: UINTPTR_MAX was not declared in this scope
Interesting - but more likely a side effect of the new include than an
inherent problem with the change. The resulting type must be a signed
64-bit value no matter how it gets named.
But as per my response to Coleen, let's move on.
Thanks,
David
-----
> Type jlong is defined as long in the jni_md.h file that we ship on Mac
> OS. If we changed jlong to int64_t everywhere, then users may suddenly
> encounter compilation errors, also.
>>
>>> I have to admit that this change looks
>>> very clean to me. If the type of something to print is jlong, you use
>>> JLONG_FORMAT, if it's size_t, you use SIZE_FORMAT. This change allows
>>> macosx to compile and gets the expected result format. I'm not sure
>>> what the purpose of having extra %xyzzy things is. This appears to be a
>>> clean change.
>>
>> Clean except for the need to still special case _APPLE_. And I confess
>> I've lost track of why that is necessary. I can see that because of
>> the definition of int64_t on _APPLE_ you can't set JLONG_FORMAT ==
>> INT64_T_FORMAT. But I think if we have to special case something it
>> should be the format specifier not the definition of jlong (which is
>> what we now have). That said moving the "problem" from the jlong
>> typedef to the JLONG_FORMAT definition isn't really any cleaner -
>> still have an ugly _APPLE_ check in shared code. (Though all the
>> jlong_format_specifier() removal is much cleaner.)
> The _APPLE_ check is now in 'semi' shared code,
> globalDefintions_gcc.hpp. That file is full of Solaris, Sparc, Linux,
> APPLE, etc. specific code. It appears to be an appropriate dumping
> ground for ugly code.
>>
>> If we go with Mikael's suggestion of using int64_t for jlong we move
>> all the special casing to Windows. But that's because the compiler
>> there does not support C99.
> My view of the problem reported by this bug is that the typedef for
> jlong on Mac OS should be changed to match the other 64-bit platforms.
> This fix does this. Whether or not int64_t is a better definition for
> jlong is a different issue. I can file an RFE to investigate this.
>
>>
>> David
>> -----
>>
>>>
>>> Thanks,
>>> Coleen
>>>
>>>
>>> On 01/15/2013 05:57 PM, Mikael Vidstedt wrote:
>>>>
>>>> On 1/15/2013 11:02 AM, harold seigel wrote:
>>>>> Hi,
>>>>>
>>>>> Thank you for the comments. I think there are two remaining minor
>>>>> issues. Let me know if I missed anything.
>>>>>
>>>>> 1. Use int64_t, instead of long, to define jlong?
>>>>>
>>>>> I prefer using 'long' to define 'jlong', rather than 'int64_t',
>>>>> because 'long' is a predefined C++ language type. Type 'int64_t'
>>>>> is a Unix operating system defined type. This would
>>>>> unnecessarily complicate things. For example, defining 'jlong'
>>>>> as 'int64_t' would require moving the definition of 'jlong' from
>>>>> src/cpu/x86/vm/jni_x86.h to files in the src/os_cpu/ directories.
>>>>>
>>>>> Would it be useful to file a new bug to investigate using
>>>>> 'int64_t' to define 'jlong' ?
>>>>>
>>>>
>>>> int64_t is part of the c99 standard, so it's not really an operating
>>>> system defined type per se, but I believe you're right in the sense
>>>> that it's not available in any of the standard header files on
>>>> Windows. But as I said I don't really have a problem defining jlong
>>>> based on long/long long if that's easier.
>>>>
>>>> I do think it'd be a useful exercise to see what it would take to use
>>>> int64_t to define jlong, but I'm fine with doing it as a separate
>>>> project.
>>>>
>>>>> 2. Define 32-bit and 64-bit variants of JLONG_FORMAT in
>>>>> src/os/posix/launcher/java_md.h ?
>>>>>
>>>>> Would it be better to define JLONG_FORMAT as %lld for 32-bit and
>>>>> %ld for 64-bit for the posix variant, in file java_md.h? I'm
>>>>> unclear what the Windows variant of "%I64d" would be.
>>>>>
>>>>
>>>> Maybe I'm missing something, but I'd say we should define jlong to be
>>>> the exact same (derived) type as int64_t, and JLONG_FORMAT should be
>>>> exactly the same as INT64_FORMAT/PRId64. For all the posix platforms I
>>>> think that should be trivial, and I'd even argue that the easiest way
>>>> to do it would be to use int64_t/PRId64 directly assuming all the
>>>> posix platforms we support have stdint.h/inttypes.h. For Windows,
>>>> judging from globalDefinitions_visCPP.hpp, it looks like "signed
>>>> __int64" and "%I64d" is the way to go regardless of 32/64. Does that
>>>> make sense?
>>>>
>>>> Cheers,
>>>> Mikael
>>>>
>>>>> Thanks, Harold
>>>>>
>>>>> On 1/14/2013 2:10 PM, Mikael Vidstedt wrote:
>>>>>> On 2013-01-12 15:05, David Holmes wrote:
>>>>>>> Sorry Harold I didn't see this before my other reply. Now I
>>>>>>> understand your problem. We either have to:
>>>>>>>
>>>>>>> a) typedef long long jlong on all platforms; or
>>>>>>> b) special case the typedef for jlong on Apple; or
>>>>>>> c) special case the typedef for JLONG_FORMAT on Apple
>>>>>>>
>>>>>>> But even if we do (a) any platform that defines int64_t differently
>>>>>>> to our jlong will mean we still need distinct format specifiers.
>>>>>>> Further unless we know how int64_t is defined on a given platform
>>>>>>> we don't know what format specifier to use (%ld or %lld).
>>>>>>>
>>>>>>> Do compilers that provide things like int64_t also define a format
>>>>>>> specifier macro?
>>>>>>
>>>>>> It's part of the C99 standard (not sure about c++) - the types have
>>>>>> corresponding format specifier macros called PRI*, defined in
>>>>>> inttypes.h. For example PRId64, PRIu64 or PRIx64 can be used to
>>>>>> print the int64_t/uint64_t equivalents of %d, %u and %x
>>>>>> respectively. Our own internal format macros (SIZE_FORMAT,
>>>>>> INTPTR_FORMAT etc) are defined as derivatives of these.
>>>>>>
>>>>>> In general "long" tends to be a mess... :(
>>>>>>
>>>>>> /Mikael
>>>>>>
>>>>>>>
>>>>>>> David
>>>>>>> -----
>>>>>>>
>>>>>>> On 12/01/2013 12:36 AM, harold seigel wrote:
>>>>>>>> Hi Vladimir,
>>>>>>>>
>>>>>>>> Thank you for your comments. Mac OS defines int64_t as 'long
>>>>>>>> long'.
>>>>>>>> So, int64_t needs a different format specifier than jlong, which
>>>>>>>> this
>>>>>>>> fix now defines as 'long'. This is because, as shown below, the
>>>>>>>> Mac OS
>>>>>>>> C++ compiler is picky about format specifiers for values of types
>>>>>>>> 'long
>>>>>>>> long' and 'long'.
>>>>>>>>
>>>>>>>> $ gcc lld.cpp
>>>>>>>> lld.cpp: In function int main(int, char**):
>>>>>>>> lld.cpp:8: warning: format %lld expects type long long int, but
>>>>>>>> argument 2 has type long int
>>>>>>>> lld.cpp:9: warning: format %ld expects type long int, but
>>>>>>>> argument 2
>>>>>>>> has type int64_t
>>>>>>>>
>>>>>>>> $ cat lld.cpp
>>>>>>>> #include <stdio.h>
>>>>>>>> #include <stdint.h>
>>>>>>>>
>>>>>>>> int main(int argc, char * argv[]) {
>>>>>>>> long long_val = 5;
>>>>>>>> int64_t int64_val = 8;
>>>>>>>> printf("long_val: %ld\n", long_val);
>>>>>>>> printf("long_val: %lld\n", long_val); <---- Line 8
>>>>>>>> printf("int64_val: %ld\n", int64_val); <--- Line 9
>>>>>>>> printf("int64_val: %lld\n", int64_val);
>>>>>>>> return 0;
>>>>>>>> }
>>>>>>>>
>>>>>>>> That is why I added JLONG_FORMAT.
>>>>>>>>
>>>>>>>> Thanks, Harold
>>>>>>>>
>>>>>>>> On 1/10/2013 9:46 PM, Vladimir Kozlov wrote:
>>>>>>>>> Can we just define INT64_FORMAT as platform specific and use it
>>>>>>>>> instead of adding new JLONG_FORMAT?
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Vladimir
>>>>>>>>>
>>>>>>>>> On 1/10/13 10:39 AM, harold seigel wrote:
>>>>>>>>>> Hi,
>>>>>>>>>>
>>>>>>>>>> Please review the following changes to fix bug 7102489.
>>>>>>>>>>
>>>>>>>>>> Summary:
>>>>>>>>>> The definition of type jlong differed on Mac OS from the other
>>>>>>>>>> 64 bit
>>>>>>>>>> platforms. This fix makes it consistent. In order to do this,
>>>>>>>>>> this fix
>>>>>>>>>> defines new macros, JLONG_FORMAT and JULONG_FORMAT, for printing
>>>>>>>>>> and
>>>>>>>>>> scanning jlongs and julongs.
>>>>>>>>>>
>>>>>>>>>> This fix also does some cleanup. Methods
>>>>>>>>>> jlong_format_specifier() and
>>>>>>>>>> julong_format_specifer() were removed and some format specifiers
>>>>>>>>>> were
>>>>>>>>>> replaced with appropriate macros.
>>>>>>>>>>
>>>>>>>>>> Open webrev at http://cr.openjdk.java.net/~hseigel/bug_7102489/
>>>>>>>>>> <http://cr.openjdk.java.net/%7Ehseigel/bug_7102489/>
>>>>>>>>>> Bug link at http://bugs.sun.com/view_bug.do?bug_id=7102489
>>>>>>>>>>
>>>>>>>>>> Thank you,
>>>>>>>>>> Harold
>>>>>>
>>>>
>>>
More information about the hotspot-runtime-dev
mailing list