RFR(M/S): 8048169: Change 8037816 breaks HS build on PPC64 and CPP-Interpreter platforms

[Coleen Phillimore]

My first impulse has been to use INTPTR_FORMAT unless the compiler 
complains.  David Chase and others are trying to make this sane though, 
I am hoping it turns out something I can remember not to break. :)

Thanks,
Coleen

On 6/30/14, 12:18 PM, Volker Simonis wrote:
> Hi Coleen,
>
> thanks for the review. As far as I have found out, these are macros
> for printf and scanf format specifiers as defined by C99. I don't know
> why we have both, INTPTR_FORMAT and PTR_FORMAT defined to the same
> format specifier. Currently, I found 1001 occurences of INTPTR_FORMAT
> and 845 occurences of PTR_FORMAT. They could be probably unified to
> one macro as they already are the same now anyway.
>
> As far as I can see, we had:
>
> #ifdef  _LP64
> #define PTR_FORMAT    PTR64_FORMAT
> #else   // !_LP64
> #define PTR_FORMAT    PTR32_FORMAT
> #endif  // _LP64
>
> #define INTPTR_FORMAT PTR_FORMAT
>
> before the big "7089790: integrate bsd-port changes" change which
> changed this to the current version. Seems like INTPTR_FORMAT has
> always just been a short cut (which is actually longer!) for
> PTR_FORMAT:)
>
> Regards,
> Volker
>
> On Mon, Jun 30, 2014 at 5:40 PM, Coleen Phillimore
> <coleen.phillimore at oracle.com> wrote:
>> Volker,
>> This is a nice cleanup!  Thank you.  I've reviewed it.  Since it's based on
>> hs-gc, I think someone from the gc group should sponsor it.  Otherwise, I'd
>> be happy to.
>>
>> One small comment below.
>>
>>
>> On 6/30/14, 10:25 AM, Volker Simonis wrote:
>>> Hi David,
>>>
>>> thanks for looking at the change. Please find my answers inline
>>>
>>> On Mon, Jun 30, 2014 at 9:53 AM, David Holmes <david.holmes at oracle.com>
>>> wrote:
>>>> Hi Volker,
>>>>
>>>> Two minor queries:
>>>>
>>>> src/share/vm/interpreter/bytecodeInterpreter.cpp
>>>>
>>>> +                    (int)(istate->bcp() - METHOD->code_base()),
>>>>
>>>> Not clear that this is an int result - what type are the operands?
>>> The operands are both 'address' (aka 'char*') so the actual result
>>> type would be 'ptrdiff_t'. But at this place, we subtract the code
>>> base from the current byte code pointer the result of which should be
>>> a short integer. I therefore think the cast should be OK.
>>>
>>>> +  tty->print_cr("native_mirror: " INTPTR_FORMAT, (uintptr_t)
>>>> this->_oop_temp);
>>>>
>>>> uintptr_t or intptr_t ? (Yes I see the existing code has the same
>>>> mismatch
>>>> but I don't understand why.)
>>> This code is there since the first OpenJDK checkin and as fas as I can
>>> see it was added with 6u10.
>>>
>>> I think it's just the name INTPTR_FORMAT which is a little misleading.
>>> Actually INTPTR_FORMAT is defined as:
>>>
>>> #ifdef  _LP64
>>> #define INTPTR_FORMAT "0x%016" PRIxPTR
>>> #define PTR_FORMAT    "0x%016" PRIxPTR
>>> #else   // !_LP64
>>> #define INTPTR_FORMAT "0x%08"  PRIxPTR
>>> #define PTR_FORMAT    "0x%08"  PRIxPTR
>>> #endif  // _LP64
>>>
>>> in globalDefinitions.hpp and "PRIxPTR" is the specifier for printing
>>> an "uintptr_t" (as defined in inttypes.h by C99).
>>>
>>> Strictly speaking, I think INTPTR_FORMAT should really be defined to
>>> PRIiPTR (or PRIdPTR) which could then be used to output signed values.
>>> On the other hand PTR_FORMAT should be used in the code only for
>>> printing unsigned hex values. But there are currently hundreds of
>>> places where INTPTR_FORMAT is used for printing unsigned hex values so
>>> that would be another XXL change.
>>
>> This PRIxPTR symbolically doesn't mean anything to me (PRI means "print"?)
>> and I'm not anxious to review another XXL change like this.   We should
>> decide on some sort of migration plan for this that is easily followed.  I
>> thought INTPTR_FORMAT was supposed to print unsigned hex values because
>> pointers are unsigned.
>>
>> Thanks,
>> Coleen
>>
>>
>>
>>> Regards,
>>> Volker
>>>
>>>> David
>>>> -----
>>>>
>>>>
>>>> On 28/06/2014 2:20 AM, Volker Simonis wrote:
>>>>> Hi,
>>>>>
>>>>> this is a follow-up for change "8037816: Fix for 8036122 breaks build
>>>>> with Xcode5/clang". It fixes the matching of format string parameter
>>>>> types to the actual argument types for the PPC64 and CPP-Interpreter
>>>>> files in the same way as 8037816 already did it for all the other
>>>>> files (so mainly insertions of 'p2i()' calls and some manual casts).
>>>>>
>>>>> http://cr.openjdk.java.net/~simonis/webrevs/8048169/
>>>>> https://bugs.openjdk.java.net/browse/JDK-8048169
>>>>>
>>>>> This change only touches PPC64 and the two CPP-Interpreter files
>>>>> src/share/vm/interpreter/bytecodeInterpreter.cpp and
>>>>> src/share/vm/interpreter/bytecodeInterpreterProfiling.hpp
>>>>>
>>>>> Please review and sponsor.
>>>>>
>>>>> Thank you and best regards,
>>>>> Volker
>>>>>