PING: RFR: JDK-8235846: Improve WindbgDebuggerLocal implementation

[Alex Menkov]

Need 2nd reviewer.

--alex

On 01/14/2020 13:10, serguei.spitsyn at oracle.com wrote:
> 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
>>>
>