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

[Chris Plummer]

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