[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR(M): 8236913: debug agent's jdwp command logging should include the command set name and comm
From: Alex Menkov <alexey.menkov () oracle ! com>
Date: 2020-01-22 19:11:08
Message-ID: 6f0c2237-8f53-c44e-9ff0-541261c9d7bf () oracle ! com
[Download RAW message or body]
LGTM
--alex
On 01/16/2020 11:41, Chris Plummer wrote:
> 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@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@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
>>>>>
>>>>
>>>>
>>
>>
>
>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic