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

[Volker Simonis]

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