RFR(M): 8236913: debug agent's jdwp command logging should include the command set name and command name
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Tue Jan 14 18:47:18 UTC 2020
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.spitsyn at 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 at 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 at 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
>>>>>>
>>>>
>>>>
>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20200114/eae79908/attachment-0001.htm>
More information about the serviceability-dev
mailing list