Request for review: 7102489: RFE: cleanup jlong typedef on __APPLE__ and _LP64 systems
David Holmes
david.holmes at oracle.com
Thu Jan 17 03:11:00 PST 2013
On 17/01/2013 12:46 PM, Coleen Phillimore wrote:
>
> Hi,
>
> I really think Harold's change is good. It takes the APPLE conditional
> compilation out of the definition of jlong and moves APPLE specific to
> the definition of JLONG_FORMAT. The jlong typedef for macosx now
> matches linux and solaris on x86, and it matches the typedef that's in
> the JDK in jni_md.h. Changing this vs. cleaning up the conditional
> code and printing is a much bigger undertaking and might need a CCC
> request. This shouldn't prevent the cleanup.
I agree. While this raises all sorts of interesting questions about how
this all hangs together on different platforms it is going way beyond
what the CR intended to do.
Apologies for the hold up Harold :)
David
> Coleen
>
> On 1/16/2013 4:51 PM, 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.
>>
>> Thanks, Harold
>>
>> On 1/16/2013 1:53 AM, David Holmes wrote:
>>> On 16/01/2013 9:12 AM, Coleen Phillimore wrote:
>>>>
>>>> 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
>>
>> 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