'Re: PING: RFR: JDK-8235846: Improve WindbgDebuggerLocal implementation'

[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: PING: RFR: JDK-8235846: Improve WindbgDebuggerLocal implementation
From:       Chris Plummer <chris.plummer () oracle ! com>
Date:       2020-01-23 23:15:57
Message-ID: 73303452-4907-557a-b4f9-d3d07df7d43c () oracle ! com
[Download RAW message or body]

On 1/23/20 3:12 PM, Alex Menkov wrote:
>
> On 01/23/2020 13:56, Chris Plummer wrote:
>> 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...
>>>
>>>>> 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.
>
> Then I'll keep the change.
> May I count you as reviewer?
>
Yes.

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


[prev in list] [next in list] [prev in thread] [next in thread]
Configure | About | News | Add a list | Sponsored by KoreLogic