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

[Chris Plummer]

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
>