RFR(M): 8236913: debug agent's jdwp command logging should include the command set name and command name
Chris Plummer
chris.plummer at oracle.com
Wed Jan 15 22:31:28 UTC 2020
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
>>>
>>
>>
More information about the serviceability-dev
mailing list