RFR: JDK-8235846: Improve WindbgDebuggerLocal implementation

[Alex Menkov]

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
>