Request for review: 7102489: RFE: cleanup jlong typedef on __APPLE__ and _LP64 systems
harold seigel
harold.seigel at oracle.com
Fri Jan 18 04:46:46 PST 2013
No apologies necessary. The issues you both brought up were valid and
worth discussing.
Thanks, Harold
On 1/17/2013 8:14 PM, Mikael Vidstedt wrote:
> 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