Request for review: 7102489: RFE: cleanup jlong typedef on __APPLE__ and _LP64 systems
Mikael Vidstedt
mikael.vidstedt at oracle.com
Thu Jan 17 17:14:50 PST 2013
On 2013-01-17 03:11, David Holmes wrote:
> 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 :)
I echo what David said - sorry for the hold up, thanks for bearing with
us :)
Cheers,
Mikael
>
> 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