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

[Chris Plummer]

Hi,

Here's a new webrev:

http://cr.openjdk.java.net/~cjplummer/8236913/webrev.01/

Since the last webrev:
  - debugDispatch.c is and the header files (other than debugDispatch.h) 
are unchanged other
    than renaming from XXX_Cmds to XXX_CmdSets
  - debugDispatch.h has a minor change to CommandSet.cmds to make it a 
pointer type,
    and added the DEBUG_DISPATCH_DEFINE_CMDSET macro

Command sets are now defined using the following form:

     Command ArrayReference_Commands[] = {
         {length, "Length"},
         {getValues, "GetValues"},
         {setValues, "SetValues"}
     };

     DEBUG_DISPATCH_DEFINE_CMDSET(ArrayReference)

There is no need to specify the size of the array anymore. 
DEBUG_DISPATCH_DEFINE_CMDSET in its expanded form would be:

CommandSet ArrayReference_Commands_CmdSet = {
     sizeof(ArrayReference_Commands) / sizeof(Command),
     "ArrayReference",
     ArrayReference_Commands
};

Since there are 4 references to ArrayReference, plus the size 
calculation, I thought it was worth using a macro here. I considered 
also passing the initialization of the Commands array as an argument, 
but I dislike macros that take code as an argument, and I didn't see as 
much value in it. I could still be persuaded though if you think it's a 
good idea.

I had to special case FieldImpl because it is zero length. When I 
discovered the Solaris issues, I also found out that Windows didn't like 
initializing an empty array. At the time I fixed it by adding a {NULL, 
NULL} entry, but eventually decided to just special case it, especially 
since I would no longer be able to cheat and say the array was length 0 
even though it had an entry.

thanks,

Chris

On 1/15/20 2:31 PM, Chris Plummer wrote:
> Didn't think of that. It should work because it's a static array, not 
> a static struct with an embedded array. I'll give it a try. I should 
> also be able to use sizeof() rather than hard code the size.
>
> thanks,
>
> Chris
>
> On 1/15/20 2:05 PM, Alex Menkov wrote:
>> Hi Chris,
>>
>> I'd prefer to not separate command handlers and names.
>>
>> maybe something like
>>
>> static Command ArrayReference_Commands[] = {
>>     {length, "Length"},
>>     {getValues, "GetValues"},
>>     {setValues, "SetValues"}
>> };
>>
>> CommandSet ArrayReference_CommandSet = {
>>     3, "ArrayReference", &ArrayReference_Commands
>> };
>>
>> --alex
>>
>> On 01/15/2020 13:09, Chris Plummer wrote:
>>> Hi,
>>>
>>> Unfortunately I'm going to have to redo this fix. I ran into 
>>> compilation problems on Solaris:
>>>
>>> error: too many struct/union initializers 
>>> (E_TOO_MANY_STRUCT_UNION_INIT)
>>>
>>> This turns up on the first initializer of the cmds[] array:
>>>
>>> CommandSet ArrayReference_Cmds = {
>>>      3, "ArrayReference",
>>>      {
>>>          {length, "Length"},   <----------
>>>          {getValues, "GetValues"},
>>>          {setValues, "SetValues"}
>>>      }
>>> };
>>>
>>> It turns out that statically initializing a variable length array 
>>> that is a field of a struct is not valid C syntax. You can 
>>> statically initialize a variable length array, which is what the 
>>> code was originally doing, but not a variable length array within a 
>>> struct.
>>>
>>> I can fix this issue by giving the array a fixed size. For example:
>>>
>>> typedef struct CommandSet {
>>>      int num_cmds;
>>>      const char *cmd_set_name;
>>>      const Command cmds[20];
>>> } CommandSet;
>>>
>>> The catch here is that the array size needs to be at least as big as 
>>> the largest use, and for all the other static uses extra space will 
>>> be allocated but never used. In other words, all the arrays would be 
>>> size 20, even those that initialize fewer than 20 elements.
>>>
>>> So the choice here pretty much keep what I have, but waste some 
>>> space with the fixed array size, or use parallel arrays to store the 
>>> function pointers and command names separately. We used to have:
>>>
>>> void *ArrayReference_Cmds[] = { (void *)0x3
>>>      ,(void *)length
>>>      ,(void *)getValues
>>>      ,(void *)setValues};
>>>
>>> I could just keep this as-is and add:
>>>
>>> char *ArrayReference_CmdNames[] = {
>>>      "Length",
>>>      "GetValues",
>>>      "SetValues"
>>> };
>>>
>>> A further improvement might be to change to original array to be:
>>>
>>> const CommandHandler ArrayReference_Cmds[] = {
>>>      length,
>>>      getValues,
>>>      setValues
>>> };
>>>
>>> And then I can add a #define for the array size:
>>>
>>> #define ArrayReference_NumCmds (sizeof(ArrayReference_Cmds) / 
>>> sizeof(CommandHandler))
>>>
>>> char *ArrayReference_CmdNames[ArrayReference_NumCmds] = {
>>>      "Length",
>>>      "GetValues",
>>>      "SetValues"
>>> };
>>>
>>> Then I can either access these arrays in parallel, meaning instead of:
>>>
>>>      cmdSetsArray[JDWP_COMMAND_SET(ArrayReference)] = 
>>> &ArrayReference_Cmds;
>>>
>>> I would have (not I need an array for the sizes also for the second 
>>> option abov):
>>>
>>>      cmdSetsCmdsArraySize[JDWP_COMMAND_SET(ArrayReference)] = 
>>> ArrayReference_NumCmds;
>>>      cmdSetsCmdsArray[JDWP_COMMAND_SET(ArrayReference)] = 
>>> &ArrayReference_Cmds;
>>>      cmdSetsCmdNamesArray[JDWP_COMMAND_SET(ArrayReference)] = 
>>> &ArrayReference_CmdNames;
>>>
>>> Or I could change the CommandSet definition to be:
>>>
>>> typedef struct CommandSet {
>>>      int num_cmds;
>>>      const char *cmd_set_name;
>>>      CommandHandler cmd_handler[];
>>>      const char *cmd_name[];
>>> } CommandSet;
>>>
>>> And then add:
>>>
>>> const CommandSet ArrayReference_CommandSet = {
>>>      ArrayReference_NumCmds,
>>>      "ArrayReference",
>>>      &ArrayReference_Cmds,
>>>      &ArrayReference_CmdNames
>>> }
>>>
>>> Then I would just have the array of CommandSets rather than 3 arrays 
>>> to deal with.
>>>
>>> Lasty, I could use a macro that declares a new type for each 
>>> CommandSet, and then when the array of CommandSets is initialized, I 
>>> would cast that type to a CommandSet. I think the invocation of the 
>>> macro would look something like:
>>>
>>> DEFINE_COMMAND_SET (3, ArrayReference,
>>>          {length, "Length"},
>>>          {getValues, "GetValues"},
>>>          {setValues, "SetValues"}
>>> )
>>>
>>> However, I'm a bit unsure of this since I haven't made any attempt 
>>> to implement it yet. There might be more issues that pop up with 
>>> this one, where-as doing the parallel arrays is pretty straight 
>>> forward (although not pretty).
>>>
>>> thoughts?
>>>
>>> Chris
>>>
>>>
>>> On 1/10/20 11:27 AM, 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
>>>>
>>>
>>>
>
>