Re: RFR(M): 8236913: debug agent's jdwp command logging should include the command set name and command name
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", _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)] =
_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)] =
_Cmds;
cmdSetsCmdNamesArray[JDWP_COMMAND_SET(ArrayReference)] =
_CmdNames;
Or I could change the CommandSet definition to be:
typedef struct CommandSet {
int num_cmds;
const char
Re: RFR(M): 8236913: debug agent's jdwp command logging should include the command set name and command name
Okay, thanks!
Serguei
On 1/17/20 14:58, Chris Plummer wrote:
Hi Serguei,
I'll make that change.
thanks,
Chris
On 1/17/20 2:33 PM, serguei.spit...@oracle.com wrote:
Hi Chris,
It looks good.
Just one nit below.
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 = "";
+*cmdName_p = "";
I'd suggest to replace Unknown with Invalid.
Then the *cmdName_p = "" line below can be removed:
+*cmdSetName_p = cmd_set->cmd_set_name;
+if (cmdNum > cmd_set->num_cmds) {
+*cmdName_p = "";
+return NULL;
No need in another webrev if you you decide to fix the above.
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", _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"}
Re: RFR(M): 8236913: debug agent's jdwp command logging should include the command set name and command name
Hi Serguei,
I'll make that change.
thanks,
Chris
On 1/17/20 2:33 PM, serguei.spit...@oracle.com wrote:
Hi Chris,
It looks good.
Just one nit below.
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 = "";
+*cmdName_p = "";
I'd suggest to replace Unknown with Invalid.
Then the *cmdName_p = "" line below can be removed:
+*cmdSetName_p = cmd_set->cmd_set_name;
+if (cmdNum > cmd_set->num_cmds) {
+*cmdName_p = "";
+return NULL;
No need in another webrev if you you decide to fix the above.
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", _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
Re: RFR(M): 8236913: debug agent's jdwp command logging should include the command set name and command name
Hi Chris,
It looks good.
Just one nit below.
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 = "";
+*cmdName_p = "";
I'd suggest to replace Unknown with Invalid.
Then the *cmdName_p = "" line below can be removed:
+*cmdSetName_p = cmd_set->cmd_set_name;
+if (cmdNum > cmd_set->num_cmds) {
+*cmdName_p = "";
+return NULL;
No need in another webrev if you you decide to fix the above.
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", _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,
Re: RFR(M): 8236913: debug agent's jdwp command logging should include the command set name and command name
Chris,
Please, ignore this (will resend my review).
In a part of my review I've looked at the old webrev.
Thanks,
Serguei
On 1/17/20 14:16, serguei.spit...@oracle.com wrote:
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 = "";
+*cmdName_p = "";
I'd suggest to replace Unknown with Invalid.
Then the *cmdName_p = "" line below can be removed:
+*cmdSetName_p = cmd_set->cmd_set_name;
+if (cmdNum > cmd_set->num_cmds) {
+*cmdName_p = "";
+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", _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"},
Re: RFR(M): 8236913: debug agent's jdwp command logging should include the command set name and command name
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 = "";
+*cmdName_p = "";
I'd suggest to replace Unknown with Invalid.
Then the *cmdName_p = "" line below can be removed:
+*cmdSetName_p = cmd_set->cmd_set_name;
+if (cmdNum > cmd_set->num_cmds) {
+*cmdName_p = "";
+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", _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"},
Re: RFR(M): 8236913: debug agent's jdwp command logging should include the command set name and command name
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", _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)] =
_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)] =
_Cmds;
cmdSetsCmdNamesArray[JDWP_COMMAND_SET(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
Re: RFR(M): 8236913: debug agent's jdwp command logging should include the command set name and command name
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", _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)] =
_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)] =
_Cmds;
cmdSetsCmdNamesArray[JDWP_COMMAND_SET(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",
_Cmds,
_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
Re: RFR(M): 8236913: debug agent's jdwp command logging should include the command set name and command name
Forgot to mention
you need to change CommandSet declaration:
typedef struct CommandSet {
int num_cmds;
const char *cmd_set_name;
-const Command cmds[];
+const Command *cmds;
} CommandSet;
--alex
On 01/15/2020 14:05, 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", _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)] =
_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)] =
_Cmds;
cmdSetsCmdNamesArray[JDWP_COMMAND_SET(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",
_Cmds,
_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
Re: RFR(M): 8236913: debug agent's jdwp command logging should include the command set name and command name
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", _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)] = _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)] =
_Cmds;
cmdSetsCmdNamesArray[JDWP_COMMAND_SET(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",
_Cmds,
_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
Re: RFR(M): 8236913: debug agent's jdwp command logging should include the command set name and command name
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)] = _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)] =
_Cmds;
cmdSetsCmdNamesArray[JDWP_COMMAND_SET(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",
_Cmds,
_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
Re: RFR(M): 8236913: debug agent's jdwp command logging should include the command set name and command name
Hi Chris,
Okay, thanks!
Serguei
On 1/14/20 10:39 AM, Chris Plummer wrote:
Hi Serguei,
I didn't want to return a Command because then the cmdSetNum and
cmdNum would need to be checked by the caller before calling
debugDispatch_getHandler()or have special handling for NULL being
returned.
Thanks for the review.
Chris
On 1/14/20 1:40 AM, serguei.spit...@oracle.com wrote:
Hi Chris,
It looks good to me modulo the comments from Alex.
I'm ok with the _p arguments.
Probably, you've already considered to return Command (instead
ofCommandHandler) from the debugDispatch_getHandler().
It allows to get rid of the cmdName_pargument but gives a
non-symmetry in getting names.
I think, it would not give any real advantage, so I'm ok with current
approach.
Thanks,
Serguei
On 1/13/20 20:11, Chris Plummer wrote:
Hi Alex,
Are you ok with the _p arguments?
Also, can I get a second reviewer please.
thanks,
Chris
On 1/10/20 3:00 PM, Chris Plummer wrote:
Hi Alex,
I'll fix the mistakes in MethodImpl.c and ReferenceTypeImpl.c. As
for the "_p" suffix, it means the argument is a pointer type that a
value will be returned in. I've seen this used elsewhere in
hotspot. For example VM_RedefineClasses::merge_constant_pools() and
ObjectSynchronizer::deflate_monitor_list().
bool VM_RedefineClasses::merge_constant_pools(const
constantPoolHandle& old_cp,
const constantPoolHandle& scratch_cp, constantPoolHandle
*merge_cp_p,
int *merge_cp_length_p, TRAPS) {
int ObjectSynchronizer::deflate_monitor_list(ObjectMonitor** list_p,
ObjectMonitor**
free_head_p,
ObjectMonitor**
free_tail_p) {
thanks,
Chris
On 1/10/20 2:12 PM, Alex Menkov wrote:
Hi Chris,
Thanks for making the code more "typed" (this "void*" arrays are
error prone).
Looks good in general, some minor comments:
MethodImpl.c
- command names starts with lower case letters
ReferenceTypeImpl.c
- please fix indentation for command definitions
debugDispatch.h/.c
+debugDispatch_getHandler(int cmdSetNum, int cmdNum, const char
**cmdSetName_p, const char **cmdName_p)
What are the "_p" suffixes for? to show that this are pointers?
To me this doesn't make much sense.
--alex
On 01/10/2020 11:27, 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
Re: RFR(M): 8236913: debug agent's jdwp command logging should include the command set name and command name
Hi Serguei,
I didn't want to return a Command because then the
cmdSetNum and cmdNum would need to be checked by the caller before
calling debugDispatch_getHandler() or have special handling for NULL being returned.
Thanks for the review.
Chris
On 1/14/20 1:40 AM, serguei.spit...@oracle.com wrote:
Hi Chris,
It looks good to me modulo the comments from Alex.
I'm ok with the _p arguments.
Probably, you've already considered to return Command (instead
of CommandHandler)
from the
debugDispatch_getHandler().
It allows to get rid of the cmdName_p argument but gives a
non-symmetry in getting names.
I think, it would not give any real advantage, so I'm ok with
current approach.
Thanks,
Serguei
On 1/13/20 20:11, Chris Plummer wrote:
Hi
Alex,
Are you ok with the _p arguments?
Also, can I get a second reviewer please.
thanks,
Chris
On 1/10/20 3:00 PM, Chris Plummer wrote:
Hi Alex,
I'll fix the mistakes in MethodImpl.c and ReferenceTypeImpl.c.
As for the "_p" suffix, it means the argument is a pointer
type that a value will be returned in. I've seen this used
elsewhere in hotspot. For example
VM_RedefineClasses::merge_constant_pools() and
ObjectSynchronizer::deflate_monitor_list().
bool VM_RedefineClasses::merge_constant_pools(const
constantPoolHandle& old_cp,
const constantPoolHandle& scratch_cp,
constantPoolHandle *merge_cp_p,
int *merge_cp_length_p, TRAPS) {
int ObjectSynchronizer::deflate_monitor_list(ObjectMonitor**
list_p,
ObjectMonitor**
free_head_p,
ObjectMonitor**
free_tail_p) {
thanks,
Chris
On 1/10/20 2:12 PM, Alex Menkov wrote:
Hi Chris,
Thanks for making the code more "typed" (this "void*" arrays
are error prone).
Looks good in general, some minor comments:
MethodImpl.c
- command names starts with lower case letters
ReferenceTypeImpl.c
- please fix indentation for command definitions
debugDispatch.h/.c
+debugDispatch_getHandler(int cmdSetNum, int cmdNum, const
char **cmdSetName_p, const char **cmdName_p)
What are the "_p" suffixes for? to show that this are
pointers?
To me this doesn't make much sense.
--alex
On 01/10/2020 11:27, 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
Re: RFR(M): 8236913: debug agent's jdwp command logging should include the command set name and command name
Hi Chris,
On 01/13/2020 20:11, Chris Plummer wrote:
Hi Alex,
Are you ok with the _p arguments?
Yes, LGTM.
--alex
Also, can I get a second reviewer please.
thanks,
Chris
On 1/10/20 3:00 PM, Chris Plummer wrote:
Hi Alex,
I'll fix the mistakes in MethodImpl.c and ReferenceTypeImpl.c. As for
the "_p" suffix, it means the argument is a pointer type that a value
will be returned in. I've seen this used elsewhere in hotspot. For
example VM_RedefineClasses::merge_constant_pools() and
ObjectSynchronizer::deflate_monitor_list().
bool VM_RedefineClasses::merge_constant_pools(const
constantPoolHandle& old_cp,
const constantPoolHandle& scratch_cp, constantPoolHandle
*merge_cp_p,
int *merge_cp_length_p, TRAPS) {
int ObjectSynchronizer::deflate_monitor_list(ObjectMonitor** list_p,
ObjectMonitor** free_head_p,
ObjectMonitor**
free_tail_p) {
thanks,
Chris
On 1/10/20 2:12 PM, Alex Menkov wrote:
Hi Chris,
Thanks for making the code more "typed" (this "void*" arrays are
error prone).
Looks good in general, some minor comments:
MethodImpl.c
- command names starts with lower case letters
ReferenceTypeImpl.c
- please fix indentation for command definitions
debugDispatch.h/.c
+debugDispatch_getHandler(int cmdSetNum, int cmdNum, const char
**cmdSetName_p, const char **cmdName_p)
What are the "_p" suffixes for? to show that this are pointers?
To me this doesn't make much sense.
--alex
On 01/10/2020 11:27, 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
Re: RFR(M): 8236913: debug agent's jdwp command logging should include the command set name and command name
Hi Chris,
It looks good to me modulo the comments from Alex.
I'm ok with the _p arguments.
Probably, you've already considered to return Command (instead of CommandHandler) from the
debugDispatch_getHandler().
It allows to get rid of the cmdName_p argument but gives a
non-symmetry in getting names.
I think, it would not give any real advantage, so I'm ok with
current approach.
Thanks,
Serguei
On 1/13/20 20:11, Chris Plummer wrote:
Hi
Alex,
Are you ok with the _p arguments?
Also, can I get a second reviewer please.
thanks,
Chris
On 1/10/20 3:00 PM, Chris Plummer wrote:
Hi Alex,
I'll fix the mistakes in MethodImpl.c and ReferenceTypeImpl.c.
As for the "_p" suffix, it means the argument is a pointer type
that a value will be returned in. I've seen this used elsewhere
in hotspot. For example
VM_RedefineClasses::merge_constant_pools() and
ObjectSynchronizer::deflate_monitor_list().
bool VM_RedefineClasses::merge_constant_pools(const
constantPoolHandle& old_cp,
const constantPoolHandle& scratch_cp,
constantPoolHandle *merge_cp_p,
int *merge_cp_length_p, TRAPS) {
int ObjectSynchronizer::deflate_monitor_list(ObjectMonitor**
list_p,
ObjectMonitor**
free_head_p,
ObjectMonitor**
free_tail_p) {
thanks,
Chris
On 1/10/20 2:12 PM, Alex Menkov wrote:
Hi Chris,
Thanks for making the code more "typed" (this "void*" arrays
are error prone).
Looks good in general, some minor comments:
MethodImpl.c
- command names starts with lower case letters
ReferenceTypeImpl.c
- please fix indentation for command definitions
debugDispatch.h/.c
+debugDispatch_getHandler(int cmdSetNum, int cmdNum, const
char **cmdSetName_p, const char **cmdName_p)
What are the "_p" suffixes for? to show that this are
pointers?
To me this doesn't make much sense.
--alex
On 01/10/2020 11:27, 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};
Re: RFR(M): 8236913: debug agent's jdwp command logging should include the command set name and command name
Hi Alex,
Are you ok with the _p arguments?
Also, can I get a second reviewer please.
thanks,
Chris
On 1/10/20 3:00 PM, Chris Plummer wrote:
Hi Alex,
I'll fix the mistakes in MethodImpl.c and ReferenceTypeImpl.c. As for
the "_p" suffix, it means the argument is a pointer type that a value
will be returned in. I've seen this used elsewhere in hotspot. For
example VM_RedefineClasses::merge_constant_pools() and
ObjectSynchronizer::deflate_monitor_list().
bool VM_RedefineClasses::merge_constant_pools(const
constantPoolHandle& old_cp,
const constantPoolHandle& scratch_cp, constantPoolHandle
*merge_cp_p,
int *merge_cp_length_p, TRAPS) {
int ObjectSynchronizer::deflate_monitor_list(ObjectMonitor** list_p,
ObjectMonitor** free_head_p,
ObjectMonitor**
free_tail_p) {
thanks,
Chris
On 1/10/20 2:12 PM, Alex Menkov wrote:
Hi Chris,
Thanks for making the code more "typed" (this "void*" arrays are
error prone).
Looks good in general, some minor comments:
MethodImpl.c
- command names starts with lower case letters
ReferenceTypeImpl.c
- please fix indentation for command definitions
debugDispatch.h/.c
+debugDispatch_getHandler(int cmdSetNum, int cmdNum, const char
**cmdSetName_p, const char **cmdName_p)
What are the "_p" suffixes for? to show that this are pointers?
To me this doesn't make much sense.
--alex
On 01/10/2020 11:27, 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
Re: RFR(M): 8236913: debug agent's jdwp command logging should include the command set name and command name
Hi Alex,
I'll fix the mistakes in MethodImpl.c and ReferenceTypeImpl.c. As for
the "_p" suffix, it means the argument is a pointer type that a value
will be returned in. I've seen this used elsewhere in hotspot. For
example VM_RedefineClasses::merge_constant_pools() and
ObjectSynchronizer::deflate_monitor_list().
bool VM_RedefineClasses::merge_constant_pools(const constantPoolHandle&
old_cp,
const constantPoolHandle& scratch_cp, constantPoolHandle
*merge_cp_p,
int *merge_cp_length_p, TRAPS) {
int ObjectSynchronizer::deflate_monitor_list(ObjectMonitor** list_p,
ObjectMonitor** free_head_p,
ObjectMonitor** free_tail_p) {
thanks,
Chris
On 1/10/20 2:12 PM, Alex Menkov wrote:
Hi Chris,
Thanks for making the code more "typed" (this "void*" arrays are error
prone).
Looks good in general, some minor comments:
MethodImpl.c
- command names starts with lower case letters
ReferenceTypeImpl.c
- please fix indentation for command definitions
debugDispatch.h/.c
+debugDispatch_getHandler(int cmdSetNum, int cmdNum, const char
**cmdSetName_p, const char **cmdName_p)
What are the "_p" suffixes for? to show that this are pointers?
To me this doesn't make much sense.
--alex
On 01/10/2020 11:27, 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
Re: RFR(M): 8236913: debug agent's jdwp command logging should include the command set name and command name
Hi Chris,
Thanks for making the code more "typed" (this "void*" arrays are error
prone).
Looks good in general, some minor comments:
MethodImpl.c
- command names starts with lower case letters
ReferenceTypeImpl.c
- please fix indentation for command definitions
debugDispatch.h/.c
+debugDispatch_getHandler(int cmdSetNum, int cmdNum, const char
**cmdSetName_p, const char **cmdName_p)
What are the "_p" suffixes for? to show that this are pointers?
To me this doesn't make much sense.
--alex
On 01/10/2020 11:27, 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
RFR(M): 8236913: debug agent's jdwp command logging should include the command set name and command name
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
