RFR: 8235681: Remove unnecessary workarounds in UnixOperatingSystem.c
Please review a change that removes unnecessary workaround in
UnixOperatingSystem.c.
It looks as this workaround
if (pticks->usedKernel < tmp.usedKernel) {
kdiff = 0;
}
was added to compensate a missed initialization of counters.jvmTicks that
resulted in the new
counters being compared with the garbage when get_cpuload_internal(...) was
called for the first time.
This missed initialization was fixed in [3].
Mach5 tier1-tier3 and open/test/hotspot/jtreg/containers/docker tests
successfully passed.
[1] Webrev : http://cr.openjdk.java.net/~dtitov/8235681/webrev.01/
[2] Issue: https://bugs.openjdk.java.net/browse/JDK-8235681
[3] https://bugs.openjdk.java.net/browse/JDK-8226575
Thank you,
Daniil
Re: RFR(M): 8236913: debug agent's jdwp command logging should include the command set name and command name
Okay, thanks!
Serguei
On 1/17/20 14:58, Chris Plummer wrote:
Hi Serguei,
I'll make that change.
thanks,
Chris
On 1/17/20 2:33 PM, serguei.spit...@oracle.com wrote:
Hi Chris,
It looks good.
Just one nit below.
http://cr.openjdk.java.net/~cjplummer/8236913/webrev.00/src/jdk.jdwp.agent/share/native/libjdwp/debugDispatch.c.udiff.html
+CommandSet *cmd_set;
+*cmdSetName_p = "";
+*cmdName_p = "";
I'd suggest to replace Unknown with Invalid.
Then the *cmdName_p = "" line below can be removed:
+*cmdSetName_p = cmd_set->cmd_set_name;
+if (cmdNum > cmd_set->num_cmds) {
+*cmdName_p = "";
+return NULL;
No need in another webrev if you you decide to fix the above.
Thanks,
Serguei
On 1/16/20 11:41, Chris Plummer wrote:
Hi,
Here's a new webrev:
http://cr.openjdk.java.net/~cjplummer/8236913/webrev.01/
Since the last webrev:
- debugDispatch.c is and the header files (other than
debugDispatch.h) are unchanged other
than renaming from XXX_Cmds to XXX_CmdSets
- debugDispatch.h has a minor change to CommandSet.cmds to
make it a pointer type,
and added the DEBUG_DISPATCH_DEFINE_CMDSET macro
Command sets are now defined using the following form:
Command ArrayReference_Commands[] = {
{length, "Length"},
{getValues, "GetValues"},
{setValues, "SetValues"}
};
DEBUG_DISPATCH_DEFINE_CMDSET(ArrayReference)
There is no need to specify the size of the array anymore.
DEBUG_DISPATCH_DEFINE_CMDSET in its expanded form would be:
CommandSet ArrayReference_Commands_CmdSet = {
sizeof(ArrayReference_Commands) / sizeof(Command),
"ArrayReference",
ArrayReference_Commands
};
Since there are 4 references to ArrayReference, plus the size
calculation, I thought it was worth using a macro here. I
considered also passing the initialization of the Commands
array as an argument, but I dislike macros that take code as
an argument, and I didn't see as much value in it. I could
still be persuaded though if you think it's a good idea.
I had to special case FieldImpl because it is zero length.
When I discovered the Solaris issues, I also found out that
Windows didn't like initializing an empty array. At the time I
fixed it by adding a {NULL, NULL} entry, but eventually
decided to just special case it, especially since I would no
longer be able to cheat and say the array was length 0 even
though it had an entry.
thanks,
Chris
On 1/15/20 2:31 PM, Chris Plummer wrote:
Didn't think of that. It should work
because it's a static array, not a static struct with an
embedded array. I'll give it a try. I should also be able to
use sizeof() rather than hard code the size.
thanks,
Chris
On 1/15/20 2:05 PM, Alex Menkov wrote:
Hi Chris,
I'd prefer to not separate command handlers and names.
maybe something like
static Command ArrayReference_Commands[] = {
{length, "Length"},
{getValues, "GetValues"},
{setValues, "SetValues"}
};
CommandSet ArrayReference_CommandSet = {
3, "ArrayReference", _Commands
};
--alex
On 01/15/2020 13:09, Chris Plummer wrote:
Hi,
Unfortunately I'm going to have to redo this fix. I ran
into compilation problems on Solaris:
error: too many struct/union initializers
(E_TOO_MANY_STRUCT_UNION_INIT)
This turns up on the first initializer of the cmds[]
array:
CommandSet ArrayReference_Cmds = {
3, "ArrayReference",
{
{length, "Length"}, <--
{getValues, "GetValues"},
{setValues, "SetValues"}
Re: RFR(M): 8236913: debug agent's jdwp command logging should include the command set name and command name
Hi Serguei,
I'll make that change.
thanks,
Chris
On 1/17/20 2:33 PM, serguei.spit...@oracle.com wrote:
Hi Chris,
It looks good.
Just one nit below.
http://cr.openjdk.java.net/~cjplummer/8236913/webrev.00/src/jdk.jdwp.agent/share/native/libjdwp/debugDispatch.c.udiff.html
+CommandSet *cmd_set;
+*cmdSetName_p = "";
+*cmdName_p = "";
I'd suggest to replace Unknown with Invalid.
Then the *cmdName_p = "" line below can be removed:
+*cmdSetName_p = cmd_set->cmd_set_name;
+if (cmdNum > cmd_set->num_cmds) {
+*cmdName_p = "";
+return NULL;
No need in another webrev if you you decide to fix the above.
Thanks,
Serguei
On 1/16/20 11:41, Chris Plummer wrote:
Hi,
Here's a new webrev:
http://cr.openjdk.java.net/~cjplummer/8236913/webrev.01/
Since the last webrev:
- debugDispatch.c is and the header files (other than
debugDispatch.h) are unchanged other
than renaming from XXX_Cmds to XXX_CmdSets
- debugDispatch.h has a minor change to CommandSet.cmds to make
it a pointer type,
and added the DEBUG_DISPATCH_DEFINE_CMDSET macro
Command sets are now defined using the following form:
Command ArrayReference_Commands[] = {
{length, "Length"},
{getValues, "GetValues"},
{setValues, "SetValues"}
};
DEBUG_DISPATCH_DEFINE_CMDSET(ArrayReference)
There is no need to specify the size of the array anymore.
DEBUG_DISPATCH_DEFINE_CMDSET in its expanded form would be:
CommandSet ArrayReference_Commands_CmdSet = {
sizeof(ArrayReference_Commands) / sizeof(Command),
"ArrayReference",
ArrayReference_Commands
};
Since there are 4 references to ArrayReference, plus the size
calculation, I thought it was worth using a macro here. I
considered also passing the initialization of the Commands array
as an argument, but I dislike macros that take code as an
argument, and I didn't see as much value in it. I could still be
persuaded though if you think it's a good idea.
I had to special case FieldImpl because it is zero length. When
I discovered the Solaris issues, I also found out that Windows
didn't like initializing an empty array. At the time I fixed it
by adding a {NULL, NULL} entry, but eventually decided to just
special case it, especially since I would no longer be able to
cheat and say the array was length 0 even though it had an
entry.
thanks,
Chris
On 1/15/20 2:31 PM, Chris Plummer wrote:
Didn't think of that. It should work
because it's a static array, not a static struct with an
embedded array. I'll give it a try. I should also be able to
use sizeof() rather than hard code the size.
thanks,
Chris
On 1/15/20 2:05 PM, Alex Menkov wrote:
Hi Chris,
I'd prefer to not separate command handlers and names.
maybe something like
static Command ArrayReference_Commands[] = {
{length, "Length"},
{getValues, "GetValues"},
{setValues, "SetValues"}
};
CommandSet ArrayReference_CommandSet = {
3, "ArrayReference", _Commands
};
--alex
On 01/15/2020 13:09, Chris Plummer wrote:
Hi,
Unfortunately I'm going to have to redo this fix. I ran
into compilation problems on Solaris:
error: too many struct/union initializers
(E_TOO_MANY_STRUCT_UNION_INIT)
This turns up on the first initializer of the cmds[]
array:
CommandSet ArrayReference_Cmds = {
3, "ArrayReference",
{
{length, "Length"}, <--
{getValues, "GetValues"},
{setValues, "SetValues"}
}
};
It turns out that statically initializing a variable
length array that is a field of a struct is not valid C
syntax. You can statically initialize a variable length
array, which is what the code was originally doing, but
not a
Re: PING: RFR: JDK-8235846: Improve WindbgDebuggerLocal implementation
Hi Alex,
I assume that the following:
65 operator T* () const {
66 return m_ptr;
67 }
Is used here:
183 AutoArrayPtr 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.
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();
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
Re: RFR(M): 8236913: debug agent's jdwp command logging should include the command set name and command name
Hi Chris,
It looks good.
Just one nit below.
http://cr.openjdk.java.net/~cjplummer/8236913/webrev.00/src/jdk.jdwp.agent/share/native/libjdwp/debugDispatch.c.udiff.html
+CommandSet *cmd_set;
+*cmdSetName_p = "";
+*cmdName_p = "";
I'd suggest to replace Unknown with Invalid.
Then the *cmdName_p = "" line below can be removed:
+*cmdSetName_p = cmd_set->cmd_set_name;
+if (cmdNum > cmd_set->num_cmds) {
+*cmdName_p = "";
+return NULL;
No need in another webrev if you you decide to fix the above.
Thanks,
Serguei
On 1/16/20 11:41, Chris Plummer wrote:
Hi,
Here's a new webrev:
http://cr.openjdk.java.net/~cjplummer/8236913/webrev.01/
Since the last webrev:
- debugDispatch.c is and the header files (other than
debugDispatch.h) are unchanged other
than renaming from XXX_Cmds to XXX_CmdSets
- debugDispatch.h has a minor change to CommandSet.cmds to make
it a pointer type,
and added the DEBUG_DISPATCH_DEFINE_CMDSET macro
Command sets are now defined using the following form:
Command ArrayReference_Commands[] = {
{length, "Length"},
{getValues, "GetValues"},
{setValues, "SetValues"}
};
DEBUG_DISPATCH_DEFINE_CMDSET(ArrayReference)
There is no need to specify the size of the array anymore.
DEBUG_DISPATCH_DEFINE_CMDSET in its expanded form would be:
CommandSet ArrayReference_Commands_CmdSet = {
sizeof(ArrayReference_Commands) / sizeof(Command),
"ArrayReference",
ArrayReference_Commands
};
Since there are 4 references to ArrayReference, plus the size
calculation, I thought it was worth using a macro here. I
considered also passing the initialization of the Commands array
as an argument, but I dislike macros that take code as an
argument, and I didn't see as much value in it. I could still be
persuaded though if you think it's a good idea.
I had to special case FieldImpl because it is zero length. When I
discovered the Solaris issues, I also found out that Windows
didn't like initializing an empty array. At the time I fixed it by
adding a {NULL, NULL} entry, but eventually decided to just
special case it, especially since I would no longer be able to
cheat and say the array was length 0 even though it had an entry.
thanks,
Chris
On 1/15/20 2:31 PM, Chris Plummer wrote:
Didn't think of that. It should work
because it's a static array, not a static struct with an
embedded array. I'll give it a try. I should also be able to use
sizeof() rather than hard code the size.
thanks,
Chris
On 1/15/20 2:05 PM, Alex Menkov wrote:
Hi Chris,
I'd prefer to not separate command handlers and names.
maybe something like
static Command ArrayReference_Commands[] = {
{length, "Length"},
{getValues, "GetValues"},
{setValues, "SetValues"}
};
CommandSet ArrayReference_CommandSet = {
3, "ArrayReference", _Commands
};
--alex
On 01/15/2020 13:09, Chris Plummer wrote:
Hi,
Unfortunately I'm going to have to redo this fix. I ran into
compilation problems on Solaris:
error: too many struct/union initializers
(E_TOO_MANY_STRUCT_UNION_INIT)
This turns up on the first initializer of the cmds[] array:
CommandSet ArrayReference_Cmds = {
3, "ArrayReference",
{
{length, "Length"}, <--
{getValues, "GetValues"},
{setValues, "SetValues"}
}
};
It turns out that statically initializing a variable length
array that is a field of a struct is not valid C syntax. You
can statically initialize a variable length array, which is
what the code was originally doing,
Re: RFR(M): 8236913: debug agent's jdwp command logging should include the command set name and command name
Chris,
Please, ignore this (will resend my review).
In a part of my review I've looked at the old webrev.
Thanks,
Serguei
On 1/17/20 14:16, serguei.spit...@oracle.com wrote:
Hi Chris,
http://cr.openjdk.java.net/~cjplummer/8236913/webrev.00/src/jdk.jdwp.agent/share/native/libjdwp/MethodImpl.c.udiff.html
+CommandSet Method_Cmds = {
+5, "Method",
+{
+{lineTable, "lineTable"},
+{variableTable, "variableTable"},
+{bytecodes, "bytecodes"},
+{isObsolete, "isObsolete"},
+{variableTableWithGenerics, "variableTableWithGenerics"}
+}
String names should start from a capital letter.
http://cr.openjdk.java.net/~cjplummer/8236913/webrev.00/src/jdk.jdwp.agent/share/native/libjdwp/debugDispatch.c.udiff.html
+CommandSet *cmd_set;
+*cmdSetName_p = "";
+*cmdName_p = "";
I'd suggest to replace Unknown with Invalid.
Then the *cmdName_p = "" line below can be removed:
+*cmdSetName_p = cmd_set->cmd_set_name;
+if (cmdNum > cmd_set->num_cmds) {
+*cmdName_p = "";
+return NULL;
Otherwise, it looks good.
Thanks,
Serguei
On 1/16/20 11:41, Chris Plummer wrote:
Hi,
Here's a new webrev:
http://cr.openjdk.java.net/~cjplummer/8236913/webrev.01/
Since the last webrev:
- debugDispatch.c is and the header files (other than
debugDispatch.h) are unchanged other
than renaming from XXX_Cmds to XXX_CmdSets
- debugDispatch.h has a minor change to CommandSet.cmds to make
it a pointer type,
and added the DEBUG_DISPATCH_DEFINE_CMDSET macro
Command sets are now defined using the following form:
Command ArrayReference_Commands[] = {
{length, "Length"},
{getValues, "GetValues"},
{setValues, "SetValues"}
};
DEBUG_DISPATCH_DEFINE_CMDSET(ArrayReference)
There is no need to specify the size of the array anymore.
DEBUG_DISPATCH_DEFINE_CMDSET in its expanded form would be:
CommandSet ArrayReference_Commands_CmdSet = {
sizeof(ArrayReference_Commands) / sizeof(Command),
"ArrayReference",
ArrayReference_Commands
};
Since there are 4 references to ArrayReference, plus the size
calculation, I thought it was worth using a macro here. I
considered also passing the initialization of the Commands array
as an argument, but I dislike macros that take code as an
argument, and I didn't see as much value in it. I could still be
persuaded though if you think it's a good idea.
I had to special case FieldImpl because it is zero length. When
I discovered the Solaris issues, I also found out that Windows
didn't like initializing an empty array. At the time I fixed it
by adding a {NULL, NULL} entry, but eventually decided to just
special case it, especially since I would no longer be able to
cheat and say the array was length 0 even though it had an
entry.
thanks,
Chris
On 1/15/20 2:31 PM, Chris Plummer wrote:
Didn't think of that. It should work
because it's a static array, not a static struct with an
embedded array. I'll give it a try. I should also be able to
use sizeof() rather than hard code the size.
thanks,
Chris
On 1/15/20 2:05 PM, Alex Menkov wrote:
Hi Chris,
I'd prefer to not separate command handlers and names.
maybe something like
static Command ArrayReference_Commands[] = {
{length, "Length"},
{getValues, "GetValues"},
{setValues, "SetValues"}
};
CommandSet ArrayReference_CommandSet = {
3, "ArrayReference", _Commands
};
--alex
On 01/15/2020 13:09, Chris Plummer wrote:
Hi,
Unfortunately I'm going to have to redo this fix. I ran
into compilation problems on Solaris:
error: too many struct/union initializers
(E_TOO_MANY_STRUCT_UNION_INIT)
This turns up on the first initializer of the cmds[]
array:
CommandSet ArrayReference_Cmds = {
3, "ArrayReference",
{
{length, "Length"},
Re: RFR(M): 8236913: debug agent's jdwp command logging should include the command set name and command name
Hi Chris,
http://cr.openjdk.java.net/~cjplummer/8236913/webrev.00/src/jdk.jdwp.agent/share/native/libjdwp/MethodImpl.c.udiff.html
+CommandSet Method_Cmds = {
+5, "Method",
+{
+{lineTable, "lineTable"},
+{variableTable, "variableTable"},
+{bytecodes, "bytecodes"},
+{isObsolete, "isObsolete"},
+{variableTableWithGenerics, "variableTableWithGenerics"}
+}
String names should start from a capital letter.
http://cr.openjdk.java.net/~cjplummer/8236913/webrev.00/src/jdk.jdwp.agent/share/native/libjdwp/debugDispatch.c.udiff.html
+CommandSet *cmd_set;
+*cmdSetName_p = "";
+*cmdName_p = "";
I'd suggest to replace Unknown with Invalid.
Then the *cmdName_p = "" line below can be removed:
+*cmdSetName_p = cmd_set->cmd_set_name;
+if (cmdNum > cmd_set->num_cmds) {
+*cmdName_p = "";
+return NULL;
Otherwise, it looks good.
Thanks,
Serguei
On 1/16/20 11:41, Chris Plummer wrote:
Hi,
Here's a new webrev:
http://cr.openjdk.java.net/~cjplummer/8236913/webrev.01/
Since the last webrev:
- debugDispatch.c is and the header files (other than
debugDispatch.h) are unchanged other
than renaming from XXX_Cmds to XXX_CmdSets
- debugDispatch.h has a minor change to CommandSet.cmds to make
it a pointer type,
and added the DEBUG_DISPATCH_DEFINE_CMDSET macro
Command sets are now defined using the following form:
Command ArrayReference_Commands[] = {
{length, "Length"},
{getValues, "GetValues"},
{setValues, "SetValues"}
};
DEBUG_DISPATCH_DEFINE_CMDSET(ArrayReference)
There is no need to specify the size of the array anymore.
DEBUG_DISPATCH_DEFINE_CMDSET in its expanded form would be:
CommandSet ArrayReference_Commands_CmdSet = {
sizeof(ArrayReference_Commands) / sizeof(Command),
"ArrayReference",
ArrayReference_Commands
};
Since there are 4 references to ArrayReference, plus the size
calculation, I thought it was worth using a macro here. I
considered also passing the initialization of the Commands array
as an argument, but I dislike macros that take code as an
argument, and I didn't see as much value in it. I could still be
persuaded though if you think it's a good idea.
I had to special case FieldImpl because it is zero length. When I
discovered the Solaris issues, I also found out that Windows
didn't like initializing an empty array. At the time I fixed it by
adding a {NULL, NULL} entry, but eventually decided to just
special case it, especially since I would no longer be able to
cheat and say the array was length 0 even though it had an entry.
thanks,
Chris
On 1/15/20 2:31 PM, Chris Plummer wrote:
Didn't think of that. It should work
because it's a static array, not a static struct with an
embedded array. I'll give it a try. I should also be able to use
sizeof() rather than hard code the size.
thanks,
Chris
On 1/15/20 2:05 PM, Alex Menkov wrote:
Hi Chris,
I'd prefer to not separate command handlers and names.
maybe something like
static Command ArrayReference_Commands[] = {
{length, "Length"},
{getValues, "GetValues"},
{setValues, "SetValues"}
};
CommandSet ArrayReference_CommandSet = {
3, "ArrayReference", _Commands
};
--alex
On 01/15/2020 13:09, Chris Plummer wrote:
Hi,
Unfortunately I'm going to have to redo this fix. I ran into
compilation problems on Solaris:
error: too many struct/union initializers
(E_TOO_MANY_STRUCT_UNION_INIT)
This turns up on the first initializer of the cmds[] array:
CommandSet ArrayReference_Cmds = {
3, "ArrayReference",
{
{length, "Length"}, <--
{getValues, "GetValues"},
PING: RFR: JDK-8235846: Improve WindbgDebuggerLocal implementation
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
Re: [PING] RFR: 8231111: Cgroups v2: Rework Metrics in java.base so as to recognize unified hierarchy
On 1/17/20 5:31 AM, Severin Gehwolf wrote: If I understand you correctly, then we'd have to change how -XshowSettings:system works currently with it. Not all metrics as currently printed via LauncherHelper.java are supported in both worlds (cgroup v1 & v2). It would make handling of cgroup v1 and cgroup v2 specific metrics a little more awkward. Adding instanceof checks and downcasting as you suggest the consequence for a client would be. See: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-823/08/webrev/src/java.base/share/classes/sun/launcher/LauncherHelper.java.sdiff.html Also, in order to not break cross platform promises we'd also have to make the version specific interfaces available in shared code (or we'd risk requiring the user to use reflection) to get the proper sub- interface logic called. -XshowSettings:system output is implementation detail [1] and also linux-only option. jdk.internal.platform.Metrics is an internal API and there is no promise that it supports the union of all cgroups v1 and v2 metrics. As you see, 1/3 of the metrics in v1 are not supported in v2. It's reasonable for -XshowSettings:system showing the metrics valid to the container. thanks Mandy [1] https://bugs.openjdk.java.net/browse/JDK-8204107
Re: [PING] RFR: 8231111: Cgroups v2: Rework Metrics in java.base so as to recognize unified hierarchy
Hi Mandy, On Thu, 2020-01-16 at 17:43 -0800, Mandy Chung wrote: > Hi Bob, Severin, > > On 1/9/20 11:51 AM, Severin Gehwolf wrote: > > Thanks for the review! Should all be fixed now. Updated webrev: > > > > incremental: > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-823/07/incremental/webrev/ > > full: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-823/07/webrev/ > > This patch may be more appropriate to be reviewed by the > serviceability group (cc'ed) as this is monitoring-related. Thanks for the review, Mandy. > jdk.internal.platform.Metrics is an internal API that you are free to > change the API as appropriate. Given that 13 out of 38 metrics > defined in Metrics are no longer supported by cgroups v2, it's > cleaner to refactor Metrics interface to be implementable by cgroups > v1 and v2 and then define a cgroups version-specific metrics to > extend Metrics, which means that it seems reasonable to make it > linux-only sub-interface. Client can cast to cgroup v1 metrics > interface if needed. Sorry for not chiming in earlier and I am not > following the cgroups v2 changes. This should be a straight-forward > change which will make the implementation cleaner. You would no > longer need the new *_UNLIMITED and *_NOT_SUPPORTED constants. If I understand you correctly, then we'd have to change how -XshowSettings:system works currently with it. Not all metrics as currently printed via LauncherHelper.java are supported in both worlds (cgroup v1 & v2). It would make handling of cgroup v1 and cgroup v2 specific metrics a little more awkward. Adding instanceof checks and downcasting as you suggest the consequence for a client would be. See: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-823/08/webrev/src/java.base/share/classes/sun/launcher/LauncherHelper.java.sdiff.html Also, in order to not break cross platform promises we'd also have to make the version specific interfaces available in shared code (or we'd risk requiring the user to use reflection) to get the proper sub- interface logic called. I'll experiment with it, but I'm not convinced this makes the API any nicer, really. > A couple of quick comments when skimming on the new files: > CgroupSubsystemController.java >s/parm/param (including javadoc @param tag) > > CgroupInfo.java >cgroupPath is not used?? Thanks, will update that in the next webrev. Cheers, Severin
Re: jmx-dev RFR(S): 8234484: Add ability to configure third port for remote JMX
Thanks Felix! On 16/01/2020 02:01, Yangfei (Felix) wrote: Modified accordingly in new webrev:http://cr.openjdk.java.net/~fyang/8234484/webrev.02/ Also renamed property to: com.sun.management.jmxremote.local.port That looks fine to me. best regards, -- daniel
