|
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};
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
|
