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

[David Holmes]

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)

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
>