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
