PING: RFR: JDK-8235846: Improve WindbgDebuggerLocal implementation

Alex Menkov alexey.menkov at oracle.com
Thu Jan 23 21:16:47 UTC 2020 


On 01/23/2020 12:35, Chris Plummer wrote:
> On 1/23/20 11:21 AM, Alex Menkov wrote:

...skipped...

>>>>
>>>>> 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?
>>
>> Well, openjdk implements only 3 COM interfaces.
>> This "standard way" is from other projects I worked on (including jdk 
>> installer which implements a lot of COM objects).
>> I don't care much about this change and can revert it.
> OK.

What does the "OK" mean? :)
Do you prefer revert the change?

--alex

> 
> thanks,
> 
> Chris
>>
>> As for ref.counter - I never see ref.counter is initialized by 0 (AWT 
>> code initialize ref.counter by 1 in ctor too).
>>
>> --alex
>>
>>>
>>> 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
>>>>>>>>>
>>>>>>>
>>>>>
>>>>>
>>>
>>>
> 
>

More information about the serviceability-dev mailing list