Request for review: 7102489: RFE: cleanup jlong typedef on __APPLE__ and _LP64 systems

[Coleen Phillimore]

I would really be worried about changing the definition in hotspot of 
jlong.  That seems very risky.  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.

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/attachments/20130115/4acf7e13/attachment-0001.html