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

[Alex Menkov]

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
>