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

[Volker Simonis]

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