PING: RFR: JDK-8235846: Improve WindbgDebuggerLocal implementation

Thu Jan 23 03:21:30 UTC 2020

On 1/22/20 12:23 PM, Alex Menkov wrote:
> 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.
Ok. This is probably acceptable C++, but TBH not something I really care 
for. It's confusing to someone who is not fluent in this use of cast 
operators. It reads funny to me because you have a local variable that 
is an instance of an object (not a pointer to an object), yet are 
comparing it to null.
>
>> 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;
> }
>
I'm not sure what standard you are referring to. The only other uses of 
E_NOINTERFACE I see are 5 uses in awt that all put the AddRef() in the 
if/else blocks, not outside. Can you please explain what this standard is?

thanks

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