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

[Stefan Karlsson]

On 2014-06-30 17:40, Coleen Phillimore 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.

This is based on jdk9/hs/hotspot:
"Compare against:    http://hg.openjdk.java.net/jdk9/hs/hotspot"

so feel free to sponsor this change.

thanks,
StefanK

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