PING: RFR: JDK-8235846: Improve WindbgDebuggerLocal implementation
Alex Menkov
alexey.menkov at oracle.com
Thu Jan 23 19:21:13 UTC 2020
Hi Chris,
On 01/22/2020 19:21, Chris Plummer wrote:
> 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.
Maybe it would be more clear to introduce bool cast operator (so to
check errmsg is invalid "if (!errmsg)" can be used), but it requires
more work to resolve "safe bool" issue:
https://en.wikibooks.org/wiki/More_C%2B%2B_Idioms/Safe_bool
>>
>>> 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.
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