PING: RFR: JDK-8235846: Improve WindbgDebuggerLocal implementation

[Chris Plummer]

Hi Alex,

I assume that the following:

   65   operator T* () const {
   66     return m_ptr;
   67   }

Is used here:

  183       AutoArrayPtr<char> errmsg(new char[strlen(str) + 32]); \
  184       if (errmsg == nullptr) { \

I just don't understand how this works. Somehow it seems the "T*" 
operator applies to the "errmsg" reference here. This is a use of C++ 
operators I'm not familiar with. Can you please explain.

Why is the following not placed at the end if the "if" block:

  299   AddRef();
  300   return S_OK;

The following was removed. It's not clear to me why it was and what the 
impact is:

  283   ptrIDebugOutputCallbacks->AddRef();

thanks,

Chris

On 1/17/20 10:37 AM, Alex Menkov wrote:
> 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
>>>>
>>