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 MISC logging was enabled by
default. All jdwp, jdb, and jdi tests were run in this way and
passed.
thanks,
Chris
