PING: RFR: JDK-8235846: Improve WindbgDebuggerLocal implementation

Chris Plummer chris.plummer at oracle.com
Thu Jan 23 21:56:35 UTC 2020 

On 1/23/20 1:16 PM, Alex Menkov wrote:
>
>
> 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?
Either is fine.

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