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