RFR (S): 8008081: Print outs do not have matching arguments

[David Holmes]

Good to go - thanks Mikael.

Perhaps make a mention in bug report about the dead code removal for 
print_frame_layout. (Did -Wunused find that ?? :) )

David

On 26/02/2013 6:38 AM, Mikael Vidstedt wrote:
>
> On 2013-02-17 15:27, David Holmes wrote:
>> On 16/02/2013 11:10 AM, Mikael Vidstedt wrote:
>>>
>>> On 2/15/2013 12:08 AM, David Holmes wrote:
>>>> On 13/02/2013 12:27 PM, Mikael Vidstedt wrote:
>>>>> Please review the following change:
>>>>>
>>>>> http://cr.openjdk.java.net/~mikael/webrevs/8008081/webrev.00/webrev
>>>>>
>>>>> Background:
>>>>>
>>>>> I found a few occurrences of calls to print type functions where the
>>>>> arguments do not match the format string. I hope that I have added the
>>>>> right/intended arguments in the respective places but could use some
>>>>> help to verify that that is indeed the case.
>>>>
>>>> In c1_FrameMap:
>>>>
>>>>  316   if( _num_monitors > 0) {
>>>>  317     tty->print_cr("monitor [0]:%d | [%2d]:%d",
>>>>  318 in_bytes(sp_offset_for_monitor_base(0)),
>>>>  319 in_bytes(sp_offset_for_monitor_base(_num_monitors)),
>>>>  320                   _num_monitors);
>>>>  321   }
>>>>
>>>> I think the last two args need swapping and should it be
>>>> _num_monitors-1 ie the "index" ? And does that make
>>>> sp_offset_for_monitor_base(_num_monitors) wrong ??
>>>
>>> Agreed, they should indeed be swapped. Thanks Staffan/David for pointing
>>> this out!
>>>
>>> Whether or not it should be _num_monitors-1 is indeed a good question.
>>> The code below uses num_spills-1 and prints things slightly differently
>>> if there is only one spill, so for symmetry reasons maybe this code
>>> should do the same thing? I'm counting on a C1 expert to provide
>>> guidance.
>>
>> Not a C1 expert but this:
>>
>> void check_monitor_index (int monitor_index) const {
>>    assert(monitor_index >= 0 && monitor_index < _num_monitors, "bad
>> index"); }
>>
>> indicates that we shouldn't be using
>> sp_offset_for_monitor_base(_num_monitors)
>
> I asked on hotspot-compiler-dev (now also cross posted) and the
> conclusion is that the print_frame_layout function can be removed all
> together. That makes the webrev look like this:
>
> http://cr.openjdk.java.net/~mikael/webrevs/8008081/webrev.02/webrev/
>
> Only c1_FrameMap.cpp and c1_FrameMap.hpp have been changed (from
> webrev.01).
>
> Good?
>
> Cheers,
> Mikael
>
>>
>> Cheers,
>> David
>> -----
>>
>>
>>> Meanwhile I've updated the webrev to add _num_monitors in the right
>>> place:
>>>
>>> http://cr.openjdk.java.net/~mikael/webrevs/8008081/webrev.01/webrev/
>>>
>>>> Everything else seems reasonable - some obvious, some less so - but
>>>> all 1000% better than before ;-)
>>>
>>> Wow, that sure is a lot of % :)
>>>
>>> Cheers,
>>> Mikael
>>>
>>>>
>>>> David
>>>> -----
>>>>
>>>>
>>>>> One special case is in this webrev is jvmtiEnter.xls. Since it is used
>>>>> to generate the jvmtiEnterTrace.cpp file and therefore is a bit less
>>>>> obvious to review I have prepared a pair of before/after files for
>>>>> your
>>>>> convenience:
>>>>>
>>>>> Before:
>>>>> http://cr.openjdk.java.net/~mikael/webrevs/8008081/jvmtiEnterTrace.cpp.orig
>>>>>
>>>>>
>>>>> After:
>>>>> http://cr.openjdk.java.net/~mikael/webrevs/8008081/jvmtiEnterTrace.cpp.fixed
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> I also prepared a diff -u of before vs after:
>>>>>
>>>>> http://cr.openjdk.java.net/~mikael/webrevs/8008081/jvmtiEnterTrace.cpp.diff
>>>>>
>>>>>
>>>>>
>>>>> Passes JPRT.
>>>>>
>>>>> Cheers,
>>>>> Mikael
>>>
>