Hi Chris,
http://cr.openjdk.java.net/~cjplummer/8236913/webrev.00/src/jdk.jdwp.agent/share/native/libjdwp/MethodImpl.c.udiff.html
+CommandSet Method_Cmds = {
+ 5, "Method",
+ {
+ {lineTable, "lineTable"},
+ {variableTable, "variableTable"},
+ {bytecodes, "bytecodes"},
+ {isObsolete, "isObsolete"},
+ {variableTableWithGenerics, "variableTableWithGenerics"}
+ }
String names should start from a capital letter.
http://cr.openjdk.java.net/~cjplummer/8236913/webrev.00/src/jdk.jdwp.agent/share/native/libjdwp/debugDispatch.c.udiff.html
+ CommandSet *cmd_set;
+ *cmdSetName_p = "<Invalid CommandSet>";
+ *cmdName_p = "<Unkown Command>";
I'd suggest to replace Unknown with Invalid.
Then the *cmdName_p = "<Invalid Command>" line below can be removed:
+ *cmdSetName_p = cmd_set->cmd_set_name;
+ if (cmdNum > cmd_set->num_cmds) {
+ *cmdName_p = "<Invalid Command>";
+ return NULL;
Otherwise, it looks good.
Thanks,
Serguei
On 1/16/20 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
|