RFR: JDK-8235846: Improve WindbgDebuggerLocal implementation

[serguei.spitsyn at oracle.com]

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20200114/0f1627f9/attachment-0001.htm>