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

[Alex Menkov]

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