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

[Coleen Phillimore]

I replied to the wrong RFR.    This one looks fine also.   Let me warn 
Alejandro that this would go straight to hs main (which I think is what 
we are deciding).

Coleen

On 6/30/14, 11:54 AM, Volker Simonis wrote:
> On Mon, Jun 30, 2014 at 5:50 PM, Stefan Karlsson
> <stefan.karlsson at oracle.com> wrote:
>> 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"
>>
> The initial, offending change was in hs-gc but because it has already
> propagated to all the other hs repos I intentionally did this fix-up
> change in jdk9/hs/hotspot to get it into the other ones as fast as
> possible.
>
>> so feel free to sponsor this change.
>>
> Would be great.
>
> Thanks,
> Volker
>
>> 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
>>>>>>