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

[Mikael Vidstedt]

On 2/25/2013 9:24 PM, David Holmes wrote:
> 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 ?? :) )

Thanks Staffan, David! I added a comment to the bug report regarding the 
dead function and I'll keep a background process going to see if there 
are things we can do to find more of them!

Cheers,
Mikael

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