PING: RFR: JDK-8235846: Improve WindbgDebuggerLocal implementation
Chris Plummer
chris.plummer at oracle.com
Thu Jan 23 20:35:13 UTC 2020
On 1/23/20 11:21 AM, Alex Menkov wrote:
> 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
It's fine the way you have it.
>
>>>
>>>> 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.
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