PING: RFR: JDK-8235846: Improve WindbgDebuggerLocal implementation

[Alex Menkov]

Hi Chris,

On 01/17/2020 14:36, Chris Plummer wrote:
> 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.

Yes, you are right.
errmsg is an object and nullptr is a pointer, so compiler tries to find 
a way to cast types.
This cast operator is the only way to cast errmsg to pointer (char *), 
so compiler uses it.
You can read more about cast operators:
https://en.cppreference.com/w/cpp/language/cast_operator
They are quite convenient.

> 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();

I updated COM object implementation (SAOutputCallbacks) to follow 
"standard way".
- standard implementation for QueryInterface looks like

   *ppInterface = nullptr;
   if (IsEqualIID(interfaceId, __uuidof(IUnknown)) ||
         IsEqualIID(interfaceId, __uuidof(MyInterface1))) {
     *ppInterface = static_cast<MyInterface1*>(this);
   } else if (IsEqualIID(interfaceId, __uuidof(MyInterface2))) {
     *ppInterface = static_cast<MyInterface2*>(this);
   } else if (IsEqualIID(interfaceId, __uuidof(MyInterface3))) {
     *ppInterface = static_cast<MyInterface3*>(this);
...
   } else {
     return E_NOINTERFACE;
   }
   AddRef();
   return S_OK;
}

- standard handling of ref. counter sets it to 1 during object creation 
(so callers don't have to call AddRef after each creation)
So I did:
-  SAOutputCallbacks() : m_refCount(0), m_msgBuffer(0) {
+  SAOutputCallbacks() : m_refCount(1), m_msgBuffer(nullptr) {

and removed unnecessary AddRef:
    SAOutputCallbacks* ptrIDebugOutputCallbacks = new SAOutputCallbacks();
-  ptrIDebugOutputCallbacks->AddRef();


--alex

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