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

[Mikael Vidstedt]

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