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.spit...@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.spit...@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
