RFR(M): 8236913: debug agent's jdwp command logging should include the command set name and command name

[Alex Menkov]

Hi Chris,

On 01/13/2020 20:11, Chris Plummer wrote:
> Hi Alex,
> 
> Are you ok with the _p arguments?

Yes, LGTM.

--alex

> 
> Also, can I get a second reviewer please.
> 
> thanks,
> 
> Chris
> 
> On 1/10/20 3:00 PM, Chris Plummer wrote:
>> Hi Alex,
>>
>> I'll fix the mistakes in MethodImpl.c and ReferenceTypeImpl.c. As for 
>> the "_p" suffix, it means the argument is a pointer type that a value 
>> will be returned in. I've seen this used elsewhere in hotspot. For 
>> example VM_RedefineClasses::merge_constant_pools() and 
>> ObjectSynchronizer::deflate_monitor_list().
>>
>> bool VM_RedefineClasses::merge_constant_pools(const 
>> constantPoolHandle& old_cp,
>>        const constantPoolHandle& scratch_cp, constantPoolHandle 
>> *merge_cp_p,
>>        int *merge_cp_length_p, TRAPS) {
>>
>> int ObjectSynchronizer::deflate_monitor_list(ObjectMonitor** list_p,
>>                                              ObjectMonitor** free_head_p,
>>                                              ObjectMonitor** 
>> free_tail_p) {
>>
>> thanks,
>>
>> Chris
>>
>> On 1/10/20 2:12 PM, Alex Menkov wrote:
>>> Hi Chris,
>>>
>>> Thanks for making the code more "typed" (this "void*" arrays are 
>>> error prone).
>>> Looks good in general, some minor comments:
>>>
>>> MethodImpl.c
>>> - command names starts with lower case letters
>>>
>>>
>>> ReferenceTypeImpl.c
>>> - please fix indentation for command definitions
>>>
>>>
>>> debugDispatch.h/.c
>>>
>>> +debugDispatch_getHandler(int cmdSetNum, int cmdNum, const char 
>>> **cmdSetName_p, const char **cmdName_p)
>>>
>>> What are the "_p" suffixes for? to show that this are pointers?
>>> To me this doesn't make much sense.
>>>
>>> --alex
>>>
>>> On 01/10/2020 11:27, Chris Plummer wrote:
>>>> Hello,
>>>>
>>>> Please review the following
>>>>
>>>> https://bugs.openjdk.java.net/browse/JDK-8236913
>>>> http://cr.openjdk.java.net/~cjplummer/8236913/webrev.00/
>>>>
>>>> The debug agent has logging support that will trace all jdwp 
>>>> commands coming in. Currently all it traces is the command set 
>>>> number and the command number within that command set. So you see 
>>>> something like:
>>>>
>>>> [#|10.01.2020 06:27:24.366 
>>>> GMT|FINEST|J2SE1.5|jdwp|LOC=MISC:"debugLoop.c":240;;PID=12719;THR=t at 915490560|:Command 
>>>> set 1, command 9|#]
>>>>
>>>> I've added support for including the name of the command set and 
>>>> command, so now you see:
>>>>
>>>> [#|10.01.2020 06:27:24.366 
>>>> GMT|FINEST|J2SE1.5|jdwp|LOC=MISC:"debugLoop.c":240;;PID=12719;THR=t at 915490560|:Command 
>>>> set VirtualMachine(1), command Resume(9)|#]
>>>>
>>>> So in this case command set 1 represents VirtualMachine and command 
>>>> 9 is the Resume command.
>>>>
>>>> I was initially going to leverage jdwp.spec which is already 
>>>> processed by build.tools.jdwpgen.Main to produce JDWP.java and 
>>>> JDWPCommands.h. However, I could see it was more of a challenge than 
>>>> I initially hoped. Also, the main advantage would have been not 
>>>> having to hard code arrays of command names, but we already have 
>>>> harded coded arrays of function pointers to handle the various jdwp 
>>>> commands, so I just replaced these with a more specialized arrays 
>>>> that also include the names of the commands. As an example, we used 
>>>> to have:
>>>>
>>>> void *ArrayReference_Cmds[] = { (void *)0x3
>>>>      ,(void *)length
>>>>      ,(void *)getValues
>>>>      ,(void *)setValues};
>>>>
>>>> Now we have:
>>>>
>>>> CommandSet ArrayReference_Cmds = {
>>>>      3, "ArrayReference",
>>>>      {
>>>>          {length, "Length"},
>>>>          {getValues, "GetValues"},
>>>>          {setValues, "SetValues"}
>>>>      }
>>>> };
>>>>
>>>> So no worse w.r.t. hard coding things that could be generated off 
>>>> the spec, and it cleans up some ugly casts also. The CommandSet 
>>>> typedef can be found in debugDispatch.h.
>>>>
>>>> All the header files except for debugDispatch.h have the same 
>>>> pattern for changes, so they are pretty easy to review
>>>>
>>>> All .c files except debugDispatch.c and debugLoop.c also have the 
>>>> same pattern. Note some command handler function names are not the 
>>>> same as the command name. If you want to double check command set 
>>>> names and command names, you can find the spec here:
>>>>
>>>> https://docs.oracle.com/javase/9/docs/specs/jdwp/jdwp-protocol.html
>>>>
>>>> In ReferenceTypeImpl.c I fixed a typo in the method() prototype. It 
>>>> had an extra argument which I think was a very old copy-n-paste bug 
>>>> from the method1() prototype. This was caught because the command 
>>>> handler functions are now directly assigned to a CommandHandler type 
>>>> rather than cast. The cast was hiding this bug.
>>>>
>>>> I tested by doing a test run where MISC logging was enabled by 
>>>> default. All jdwp, jdb, and jdi tests were run in this way and passed.
>>>>
>>>> thanks,
>>>>
>>>> Chris
>>>>
>>
>>
> 
>