PING: RFR: JDK-8235846: Improve WindbgDebuggerLocal implementation

[Alex Menkov]

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?

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