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

[Coleen Phillimore]

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