[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR: JDK-8235846: Improve WindbgDebuggerLocal implementation
From: serguei.spitsyn () oracle ! com
Date: 2020-01-14 21:10:14
Message-ID: 2a7a52f8-aaea-2124-faaf-d371d8bcbb8e () oracle ! com
[Download RAW message or body]
Hi Alex,
Thank you for the update!
It looks good.
Still incorrect indent:
103 ~AutoJavaString() { 104 if (m_buf) { 105
m_env->ReleaseStringUTFChars(m_str, m_buf); 106 } 107 } 108 109 operator
const char* () const { 110 return m_buf; 111 } ... 133 void
setReleaseMode(jint mode) {
134 releaseMode = mode;
135 }
136
137 operator jbyte* () const {
138 return bytePtr;
139 }
The comment shout start from a capital letter:
177 // verifies COM call result is S_OK, throws DebuggerException and
exits otherwise.
No need for another webrev.
Thanks,
Serguei
On 1/14/20 12:39 PM, Alex Menkov wrote:
> Hi Serguei,
>
> Thank you for the review.
> updated webrev:
> http://cr.openjdk.java.net/~amenkov/jdk15/WinDbg_improve/webrev.02/
>
> On 01/13/2020 16:39, serguei.spitsyn@oracle.com wrote:
>> Hi Alex,
>>
>> It looks pretty good.
>> Just some minor comments below.
>>
>> The class AutoCOMPtr has unfixed indents.
>> I guess, the function AutoArrayPtr.asPtr() is not used anymore and
>> can be deleted.
>
> fixed.
>
>>
>> I'd suggest to remove first level '()' brackets in the comment to
>> avoid brackets nesting: 74 // manage COM 'auto' pointers (to avoid
>> multiple Release 75 // calls at every early (exception) returns). =>
>> 74 // manage COM 'auto' pointers to avoid multiple Release
>> 75 // calls at every early (exception) returns.
>
> done.
>
>> I'd suggest to reformat it as it was originally formatted:
>>
>> 297 if (IsEqualIID(interfaceId, __uuidof(IUnknown))
>> 298 || IsEqualIID(interfaceId, __uuidof(IDebugOutputCallbacks))) { =>
>> 297 if (IsEqualIID(interfaceId, __uuidof(IUnknown)) ||
>> 298 IsEqualIID(interfaceId, __uuidof(IDebugOutputCallbacks))) {
>
> done.
>
>> The comment about S_FALSE is not that clear as S_FALSE is not really
>> used
>> (better to use consistently just one value: S_FALSE or false):
>>
>> 418 // S_FALSE means timeout
>> 419 COM_VERIFY_OK_(ptrIDebugControl->WaitForEvent(DEBUG_WAIT_DEFAULT,
>> INFINITE),
>> 420 "Windbg Error: WaitForEvent failed!", false);
>
> S_FALSE is a possible result of ptrIDebugControl->WaitForEvent call.
> In the case we wait infinitely, so S_FALSE is not possible and we
> don't handle it separately.
> I removed the comment.
>
>
>> NULL is used in some placesand nullptr in others.
>
> There is some mess with 0/NULL/nullptr in the file
> I fixed all NULLs and some (most likely not all) 0s.
> BTW it immediately discovered misuse of NULL/0:
>
> - if (ptrIDebugSymbols->GetModuleParameters(loaded, 0, NULL,
> params.asPtr()) != S_OK) {
> - THROW_NEW_DEBUGGER_EXCEPTION_("Windbg Error: GetModuleParameters
> failed!", false);
> - }
> + COM_VERIFY_OK_(ptrIDebugSymbols->GetModuleParameters(loaded,
> nullptr, 0, params),
> + "Windbg Error: GetModuleParameters failed!", false);
>
> 2nd arg of GetModuleParameters is a pointer and 3rd is ulong
>
> --alex
>>
>> Thanks,
>> Serguei
>>
>>
>> On 12/19/19 15:34, Alex Menkov wrote:
>>> Hi all,
>>>
>>> Please review a fix for
>>> https://bugs.openjdk.java.net/browse/JDK-8235846
>>> webrev:
>>> http://cr.openjdk.java.net/~amenkov/jdk15/WinDbg_improve/webrev.01/
>>>
>>> Main goal of the change is to improve error reporting (we have
>>> several bugs and need at least COM error codes for WinDbg calls).
>>> Also the fix improves/rearranges this quite old code.
>>>
>>> --alex
>>
[Attachment #3 (text/html)]
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
Hi Alex,<br>
<br>
Thank you for the update!<br>
It looks good.<br>
<br>
Still incorrect indent:<br>
<pre><span class="new"> 103 ~AutoJavaString() {
<span class="new"> 104 if (m_buf) {</span>
105 m_env->ReleaseStringUTFChars(m_str, m_buf);
106 }
<span class="new"> 107 }</span>
108
<span class="changed"> 109 operator const char* () const {</span>
110 return m_buf;
111 }
...
133 void setReleaseMode(jint mode) {</span>
<span class="new"> 134 releaseMode = mode;</span>
<span class="new"> 135 }</span>
<span class="new"> 136 </span>
<span class="new"> 137 operator jbyte* () const {</span>
<span class="new"> 138 return bytePtr;</span>
<span class="new"> 139 }</span></pre>
The comment shout start from a capital letter:<br>
<pre><span class="new"> 177 // verifies COM call result is S_OK, throws \
DebuggerException and exits otherwise.</span></pre> <br>
No need for another webrev.<br>
<br>
Thanks,<br>
Serguei<br>
<br>
<br>
<br>
<div class="moz-cite-prefix">On 1/14/20 12:39 PM, Alex Menkov wrote:<br>
</div>
<blockquote type="cite"
cite="mid:2bac872a-e538-83e9-1448-dcd788de2cee@oracle.com">Hi
Serguei,
<br>
<br>
Thank you for the review.
<br>
updated webrev:
<br>
<a class="moz-txt-link-freetext" \
href="http://cr.openjdk.java.net/~amenkov/jdk15/WinDbg_improve/webrev.02/">http://cr.openjdk.java.net/~amenkov/jdk15/WinDbg_improve/webrev.02/</a>
<br>
<br>
On 01/13/2020 16:39, <a class="moz-txt-link-abbreviated" \
href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a> wrote: <br>
<blockquote type="cite">Hi Alex,
<br>
<br>
It looks pretty good.
<br>
Just some minor comments below.
<br>
<br>
The class AutoCOMPtr has unfixed indents.
<br>
I guess, the function AutoArrayPtr.asPtr() is not used anymore
and can be deleted.
<br>
</blockquote>
<br>
fixed.
<br>
<br>
<blockquote type="cite">
<br>
I'd suggest to remove first level '()' brackets in the comment
to avoid brackets nesting: 74 // manage COM 'auto' pointers (to
avoid multiple Release 75 // calls at every early (exception)
returns). => 74 // manage COM 'auto' pointers to avoid
multiple Release
<br>
75 // calls at every early (exception) returns.
<br>
</blockquote>
<br>
done.
<br>
<br>
<blockquote type="cite">I'd suggest to reformat it as it was
originally formatted:
<br>
<br>
297 if (IsEqualIID(interfaceId, __uuidof(IUnknown))
<br>
298 || IsEqualIID(interfaceId, __uuidof(IDebugOutputCallbacks)))
{ => 297 if (IsEqualIID(interfaceId, __uuidof(IUnknown)) ||
<br>
298 IsEqualIID(interfaceId, __uuidof(IDebugOutputCallbacks))) {
<br>
</blockquote>
<br>
done.
<br>
<br>
<blockquote type="cite">The comment about S_FALSE is not that
clear as S_FALSE is not really used
<br>
(better to use consistently just one value: S_FALSE or false):
<br>
<br>
418 // S_FALSE means timeout
<br>
419
COM_VERIFY_OK_(ptrIDebugControl->WaitForEvent(DEBUG_WAIT_DEFAULT,
INFINITE),
<br>
420 "Windbg Error: WaitForEvent failed!", false);
<br>
</blockquote>
<br>
S_FALSE is a possible result of ptrIDebugControl->WaitForEvent
call.
<br>
In the case we wait infinitely, so S_FALSE is not possible and we
don't handle it separately.
<br>
I removed the comment.
<br>
<br>
<br>
<blockquote type="cite">NULL is used in some placesand nullptr in
others.
<br>
</blockquote>
<br>
There is some mess with 0/NULL/nullptr in the file
<br>
I fixed all NULLs and some (most likely not all) 0s.
<br>
BTW it immediately discovered misuse of NULL/0:
<br>
<br>
- if (ptrIDebugSymbols->GetModuleParameters(loaded, 0, NULL,
params.asPtr()) != S_OK) {
<br>
- THROW_NEW_DEBUGGER_EXCEPTION_("Windbg Error:
GetModuleParameters failed!", false);
<br>
- }
<br>
+ COM_VERIFY_OK_(ptrIDebugSymbols->GetModuleParameters(loaded,
nullptr, 0, params),
<br>
+ "Windbg Error: GetModuleParameters failed!",
false);
<br>
<br>
2nd arg of GetModuleParameters is a pointer and 3rd is ulong
<br>
<br>
--alex
<br>
<blockquote type="cite">
<br>
Thanks,
<br>
Serguei
<br>
<br>
<br>
On 12/19/19 15:34, Alex Menkov wrote:
<br>
<blockquote type="cite">Hi all,
<br>
<br>
Please review a fix for
<br>
<a class="moz-txt-link-freetext" \
href="https://bugs.openjdk.java.net/browse/JDK-8235846">https://bugs.openjdk.java.net/browse/JDK-8235846</a>
<br>
webrev:
<br>
<a class="moz-txt-link-freetext" \
href="http://cr.openjdk.java.net/~amenkov/jdk15/WinDbg_improve/webrev.01/">http://cr.openjdk.java.net/~amenkov/jdk15/WinDbg_improve/webrev.01/</a>
<br>
<br>
Main goal of the change is to improve error reporting (we have
several bugs and need at least COM error codes for WinDbg
calls).
<br>
Also the fix improves/rearranges this quite old code.
<br>
<br>
--alex
<br>
</blockquote>
<br>
</blockquote>
</blockquote>
<br>
</body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic