Re: RFR 8235300: VM_HeapDumper hits assert with bad dump_len
Hi Paul, I made some minor updates to the CSR request and added myself as a reviewer. Thanks, David On 15/01/2020 10:00 am, Hohensee, Paul wrote: Please review this CSR for a backport of JDK-8144732 to jdk8u. JBS issue: https://bugs.openjdk.java.net/browse/JDK-8144732 JBS release note issue: https://bugs.openjdk.java.net/browse/JDK-8174881 8u backport JBS issue: https://bugs.openjdk.java.net/browse/JDK-8235299 8u backport CSR: https://bugs.openjdk.java.net/browse/JDK-8235300 The original patch went through the Oracle CCC process prior to the creation of the public CSR process, so there’s no public CSR issue, hence the release note JBS issue reference. Thanks, Paul
Re: RFR (XS) 8236968: jmap -clstats fails to work after JDK-8232759
Thanks Chris, I'll fix the copyrights on commit. Coleen On 1/14/20 6:11 PM, Chris Plummer wrote: Hi Coleen, Looks good. Please update the copyright. thanks, Chris On 1/14/20 12:37 PM, coleen.phillim...@oracle.com wrote: Summary: Make jmap -clstats call jcmd VM.classloader_stats instead which better matches the documentation Tested with tier1 and * jtreg:open/test/jdk/sun/tools/jmap/BasicJMapTest.java locally. open webrev at http://cr.openjdk.java.net/~coleenp/2020/8236968.01/webrev bug link https://bugs.openjdk.java.net/browse/JDK-8236968 Thanks, Coleen
Re: RFR (XS) 8236968: jmap -clstats fails to work after JDK-8232759
Hi Coleen, Looks good. Please update the copyright. thanks, Chris On 1/14/20 12:37 PM, coleen.phillim...@oracle.com wrote: Summary: Make jmap -clstats call jcmd VM.classloader_stats instead which better matches the documentation Tested with tier1 and * jtreg:open/test/jdk/sun/tools/jmap/BasicJMapTest.java locally. open webrev at http://cr.openjdk.java.net/~coleenp/2020/8236968.01/webrev bug link https://bugs.openjdk.java.net/browse/JDK-8236968 Thanks, Coleen
Re: RFR (XS) 8236968: jmap -clstats fails to work after JDK-8232759
Hi Coleen, I concur with the discussion in the bug report. This change looks good. Thanks, David On 15/01/2020 6:37 am, coleen.phillim...@oracle.com wrote: Summary: Make jmap -clstats call jcmd VM.classloader_stats instead which better matches the documentation Tested with tier1 and * jtreg:open/test/jdk/sun/tools/jmap/BasicJMapTest.java locally. open webrev at http://cr.openjdk.java.net/~coleenp/2020/8236968.01/webrev bug link https://bugs.openjdk.java.net/browse/JDK-8236968 Thanks, Coleen
Re: RFR (XS) 8236917: TestInstanceKlassSize.java fails with "The size computed by SA for java.lang.Object does not match"
Thanks David. The follow up issue is: https://bugs.openjdk.java.net/browse/JDK-8237111 But there are more than the LingeredApps that use getVmOptions(). Somebody should make sure all these tests are getting the right values. thanks, Coleen On 1/14/20 4:56 PM, David Holmes wrote: Fix looks good - thanks Coleen. This one certainly had us scratching our heads for a while! I agree there should be a follow up issue about use of getVmOptions() in tests. David On 15/01/2020 5:38 am, coleen.phillim...@oracle.com wrote: Summary: Use getTestJavaOpts() instead of getVmOptions() because of mach5 configuration settings. Tested with hs-tier3 with no failures anymore. open webrev at http://cr.openjdk.java.net/~coleenp/2020/8236917.01/webrev bug link https://bugs.openjdk.java.net/browse/JDK-8236917 Thanks, Coleen
Re: RFR (XS) 8236917: TestInstanceKlassSize.java fails with "The size computed by SA for java.lang.Object does not match"
Fix looks good - thanks Coleen. This one certainly had us scratching our heads for a while! I agree there should be a follow up issue about use of getVmOptions() in tests. David On 15/01/2020 5:38 am, coleen.phillim...@oracle.com wrote: Summary: Use getTestJavaOpts() instead of getVmOptions() because of mach5 configuration settings. Tested with hs-tier3 with no failures anymore. open webrev at http://cr.openjdk.java.net/~coleenp/2020/8236917.01/webrev bug link https://bugs.openjdk.java.net/browse/JDK-8236917 Thanks, Coleen
Re: RFR (XS) 8236917: TestInstanceKlassSize.java fails with "The size computed by SA for java.lang.Object does not match"
Thanks Chris! Coleen On 1/14/20 3:57 PM, Chris Plummer wrote: Looks good. Thanks for fixing! Chris On 1/14/20 11:38 AM, coleen.phillim...@oracle.com wrote: Summary: Use getTestJavaOpts() instead of getVmOptions() because of mach5 configuration settings. Tested with hs-tier3 with no failures anymore. open webrev at http://cr.openjdk.java.net/~coleenp/2020/8236917.01/webrev bug link https://bugs.openjdk.java.net/browse/JDK-8236917 Thanks, Coleen
Re: RFR (XS) 8236917: TestInstanceKlassSize.java fails with "The size computed by SA for java.lang.Object does not match"
On 1/14/20 3:45 PM, Leonid Mesnik wrote: Hi The fix looks good itself (you still need 'R'eview). However I see about 20 other SA/ tests where lingeredApp is still executed with vm.opts only. Even it doesn't cause any failures it means that tests silently ignore java.opts set by user. I think it makes sense to fix them also or file new issue for this activity. I'll file another bug for the other instances. I'm not sure that maybe we want to remove getVmOptions() with that bug, so as not to trick anyone else. Thanks, Coleen Leonid On 1/14/20 11:38 AM, coleen.phillim...@oracle.com wrote: Summary: Use getTestJavaOpts() instead of getVmOptions() because of mach5 configuration settings. Tested with hs-tier3 with no failures anymore. open webrev at http://cr.openjdk.java.net/~coleenp/2020/8236917.01/webrev bug link https://bugs.openjdk.java.net/browse/JDK-8236917 Thanks, Coleen
Re: RFR: JDK-8235846: Improve WindbgDebuggerLocal implementation
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: 8213222: remove RMIConnectorServer.CREDENTIAL_TYPES
Hi Alan and Daniel, Thank you for reviewing this change and the CSR. I updated the release note as Daniel suggested. Best regards, Daniil On 1/13/20, 7:36 AM, "Daniel Fuchs" wrote: Hi Daniil, Looks good to me as well. I wonder however if the release note should point to the replacement? Something like: The terminally deprecated constant `javax.management.remote.rmi.RMIConnectorServer.CREDENTIAL_TYPE` has been removed. A filter pattern can be specified instead using `RMIConnectorServer.CREDENTIALS_FILTER_PATTERN`. best regards, -- daniel On 11/01/2020 04:52, Daniil Titov wrote: > Please review a change [1] that removes constant javax.management.remote.rmi.RMIConnectorServer.CREDENTIAL_TYPE. > This constant represents the name of the attribute that specifies a list of class names acceptable to the RMIServer.newClient() > remote method call. This constant was superseded by RMIConnectorServer.CREDENTIALS_FILTER_PATTERN and marked as > deprecated for removal in JDK 10 [3]. > > A new CRS [4] and a release note sub-task [5] were also created. Please review the CSR [4] as well. > > Text search over the repository didn't find any other usage of this constant. Tier1 - tier3 Mach5 tests succeeded. > > [1] Webrev: http://cr.openjdk.java.net/~dtitov/8213222/webrev.01/ > [2] Issue: https://bugs.openjdk.java.net/browse/JDK-8213222 > [3] https://bugs.openjdk.java.net/browse/JDK-8191313 > [4] CSR: https://bugs.openjdk.java.net/browse/JDK-8236954 > [5] Release Note sub-task: https://bugs.openjdk.java.net/browse/JDK-8236955 > > Thank you, > Daniil > >
Re: RFR (XS) 8236917: TestInstanceKlassSize.java fails with "The size computed by SA for java.lang.Object does not match"
Looks good. Thanks for fixing! Chris On 1/14/20 11:38 AM, coleen.phillim...@oracle.com wrote: Summary: Use getTestJavaOpts() instead of getVmOptions() because of mach5 configuration settings. Tested with hs-tier3 with no failures anymore. open webrev at http://cr.openjdk.java.net/~coleenp/2020/8236917.01/webrev bug link https://bugs.openjdk.java.net/browse/JDK-8236917 Thanks, Coleen
Re: RFR (XS) 8236917: TestInstanceKlassSize.java fails with "The size computed by SA for java.lang.Object does not match"
Hi The fix looks good itself (you still need 'R'eview). However I see about 20 other SA/ tests where lingeredApp is still executed with vm.opts only. Even it doesn't cause any failures it means that tests silently ignore java.opts set by user. I think it makes sense to fix them also or file new issue for this activity. Leonid On 1/14/20 11:38 AM, coleen.phillim...@oracle.com wrote: Summary: Use getTestJavaOpts() instead of getVmOptions() because of mach5 configuration settings. Tested with hs-tier3 with no failures anymore. open webrev at http://cr.openjdk.java.net/~coleenp/2020/8236917.01/webrev bug link https://bugs.openjdk.java.net/browse/JDK-8236917 Thanks, Coleen
Re: RFR: JDK-8235846: Improve WindbgDebuggerLocal implementation
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
RFR (XS) 8236917: TestInstanceKlassSize.java fails with "The size computed by SA for java.lang.Object does not match"
Summary: Use getTestJavaOpts() instead of getVmOptions() because of mach5 configuration settings. Tested with hs-tier3 with no failures anymore. open webrev at http://cr.openjdk.java.net/~coleenp/2020/8236917.01/webrev bug link https://bugs.openjdk.java.net/browse/JDK-8236917 Thanks, Coleen
Re: RFR(M): 8236913: debug agent's jdwp command logging should include the command set name and command name
Hi Chris,
Okay, thanks!
Serguei
On 1/14/20 10:39 AM, Chris Plummer wrote:
Hi Serguei,
I didn't want to return a Command because then the cmdSetNum and
cmdNum would need to be checked by the caller before calling
debugDispatch_getHandler()or have special handling for NULL being
returned.
Thanks for the review.
Chris
On 1/14/20 1:40 AM, serguei.spit...@oracle.com wrote:
Hi Chris,
It looks good to me modulo the comments from Alex.
I'm ok with the _p arguments.
Probably, you've already considered to return Command (instead
ofCommandHandler) from the debugDispatch_getHandler().
It allows to get rid of the cmdName_pargument but gives a
non-symmetry in getting names.
I think, it would not give any real advantage, so I'm ok with current
approach.
Thanks,
Serguei
On 1/13/20 20:11, Chris Plummer wrote:
Hi Alex,
Are you ok with the _p arguments?
Also, can I get a second reviewer please.
thanks,
Chris
On 1/10/20 3:00 PM, Chris Plummer wrote:
Hi Alex,
I'll fix the mistakes in MethodImpl.c and ReferenceTypeImpl.c. As
for the "_p" suffix, it means the argument is a pointer type that a
value will be returned in. I've seen this used elsewhere in
hotspot. For example VM_RedefineClasses::merge_constant_pools() and
ObjectSynchronizer::deflate_monitor_list().
bool VM_RedefineClasses::merge_constant_pools(const
constantPoolHandle& old_cp,
const constantPoolHandle& scratch_cp, constantPoolHandle
*merge_cp_p,
int *merge_cp_length_p, TRAPS) {
int ObjectSynchronizer::deflate_monitor_list(ObjectMonitor** list_p,
ObjectMonitor**
free_head_p,
ObjectMonitor**
free_tail_p) {
thanks,
Chris
On 1/10/20 2:12 PM, Alex Menkov wrote:
Hi Chris,
Thanks for making the code more "typed" (this "void*" arrays are
error prone).
Looks good in general, some minor comments:
MethodImpl.c
- command names starts with lower case letters
ReferenceTypeImpl.c
- please fix indentation for command definitions
debugDispatch.h/.c
+debugDispatch_getHandler(int cmdSetNum, int cmdNum, const char
**cmdSetName_p, const char **cmdName_p)
What are the "_p" suffixes for? to show that this are pointers?
To me this doesn't make much sense.
--alex
On 01/10/2020 11:27, Chris Plummer wrote:
Hello,
Please review the following
https://bugs.openjdk.java.net/browse/JDK-8236913
http://cr.openjdk.java.net/~cjplummer/8236913/webrev.00/
The debug agent has logging support that will trace all jdwp
commands coming in. Currently all it traces is the command set
number and the command number within that command set. So you see
something like:
[#|10.01.2020 06:27:24.366
GMT|FINEST|J2SE1.5|jdwp|LOC=MISC:"debugLoop.c":240;;PID=12719;THR=t@915490560|:Command
set 1, command 9|#]
I've added support for including the name of the command set and
command, so now you see:
[#|10.01.2020 06:27:24.366
GMT|FINEST|J2SE1.5|jdwp|LOC=MISC:"debugLoop.c":240;;PID=12719;THR=t@915490560|:Command
set VirtualMachine(1), command Resume(9)|#]
So in this case command set 1 represents VirtualMachine and
command 9 is the Resume command.
I was initially going to leverage jdwp.spec which is already
processed by build.tools.jdwpgen.Main to produce JDWP.java and
JDWPCommands.h. However, I could see it was more of a challenge
than I initially hoped. Also, the main advantage would have been
not having to hard code arrays of command names, but we already
have harded coded arrays of function pointers to handle the
various jdwp commands, so I just replaced these with a more
specialized arrays that also include the names of the commands.
As an example, we used to have:
void *ArrayReference_Cmds[] = { (void *)0x3
,(void *)length
,(void *)getValues
,(void *)setValues};
Now we have:
CommandSet ArrayReference_Cmds = {
3, "ArrayReference",
{
{length, "Length"},
{getValues, "GetValues"},
{setValues, "SetValues"}
}
};
So no worse w.r.t. hard coding things that could be generated off
the spec, and it cleans up some ugly casts also. The CommandSet
typedef can be found in debugDispatch.h.
All the header files except for debugDispatch.h have the same
pattern for changes, so they are pretty easy to review
All .c files except debugDispatch.c and debugLoop.c also have the
same pattern. Note some command handler function names are not
the same as the command name. If you want to double check command
set names and command names, you can find the spec here:
https://docs.oracle.com/javase/9/docs/specs/jdwp/jdwp-protocol.html
In ReferenceTypeImpl.c I fixed a typo in the method() prototype.
It had an extra argument which I think was a very old
copy-n-paste bug from the method1() prototype. This was caught
because the command handler functions are now directly assigned
to a CommandHandler type rather than cast. The cast was hiding
this bug.
I tested by doing a test run where
Re: RFR(M): 8236913: debug agent's jdwp command logging should include the command set name and command name
Hi Serguei,
I didn't want to return a Command because then the
cmdSetNum and cmdNum would need to be checked by the caller before
calling debugDispatch_getHandler() or have special handling for NULL being returned.
Thanks for the review.
Chris
On 1/14/20 1:40 AM, serguei.spit...@oracle.com wrote:
Hi Chris,
It looks good to me modulo the comments from Alex.
I'm ok with the _p arguments.
Probably, you've already considered to return Command (instead
of CommandHandler)
from the
debugDispatch_getHandler().
It allows to get rid of the cmdName_p argument but gives a
non-symmetry in getting names.
I think, it would not give any real advantage, so I'm ok with
current approach.
Thanks,
Serguei
On 1/13/20 20:11, Chris Plummer wrote:
Hi
Alex,
Are you ok with the _p arguments?
Also, can I get a second reviewer please.
thanks,
Chris
On 1/10/20 3:00 PM, Chris Plummer wrote:
Hi Alex,
I'll fix the mistakes in MethodImpl.c and ReferenceTypeImpl.c.
As for the "_p" suffix, it means the argument is a pointer
type that a value will be returned in. I've seen this used
elsewhere in hotspot. For example
VM_RedefineClasses::merge_constant_pools() and
ObjectSynchronizer::deflate_monitor_list().
bool VM_RedefineClasses::merge_constant_pools(const
constantPoolHandle& old_cp,
const constantPoolHandle& scratch_cp,
constantPoolHandle *merge_cp_p,
int *merge_cp_length_p, TRAPS) {
int ObjectSynchronizer::deflate_monitor_list(ObjectMonitor**
list_p,
ObjectMonitor**
free_head_p,
ObjectMonitor**
free_tail_p) {
thanks,
Chris
On 1/10/20 2:12 PM, Alex Menkov wrote:
Hi Chris,
Thanks for making the code more "typed" (this "void*" arrays
are error prone).
Looks good in general, some minor comments:
MethodImpl.c
- command names starts with lower case letters
ReferenceTypeImpl.c
- please fix indentation for command definitions
debugDispatch.h/.c
+debugDispatch_getHandler(int cmdSetNum, int cmdNum, const
char **cmdSetName_p, const char **cmdName_p)
What are the "_p" suffixes for? to show that this are
pointers?
To me this doesn't make much sense.
--alex
On 01/10/2020 11:27, Chris Plummer wrote:
Hello,
Please review the following
https://bugs.openjdk.java.net/browse/JDK-8236913
http://cr.openjdk.java.net/~cjplummer/8236913/webrev.00/
The debug agent has logging support that will trace all
jdwp commands coming in. Currently all it traces is the
command set number and the command number within that
command set. So you see something like:
[#|10.01.2020 06:27:24.366
GMT|FINEST|J2SE1.5|jdwp|LOC=MISC:"debugLoop.c":240;;PID=12719;THR=t@915490560|:Command
set 1, command 9|#]
I've added support for including the name of the command
set and command, so now you see:
[#|10.01.2020 06:27:24.366
GMT|FINEST|J2SE1.5|jdwp|LOC=MISC:"debugLoop.c":240;;PID=12719;THR=t@915490560|:Command
set VirtualMachine(1), command Resume(9)|#]
So in this case command set 1 represents VirtualMachine
and command 9 is the Resume command.
I was initially going to leverage jdwp.spec which is
already processed by build.tools.jdwpgen.Main to produce
JDWP.java and JDWPCommands.h. However, I could see it was
more of a challenge than I initially hoped. Also, the main
advantage would have been not having to hard code arrays
of command names, but we already have harded coded arrays
of function pointers to handle the various jdwp commands,
so I just replaced these with a more specialized arrays
that also include the names of the
Re: RFR(M): 8236913: debug agent's jdwp command logging should include the command set name and command name
Hi Chris,
On 01/13/2020 20:11, Chris Plummer wrote:
Hi Alex,
Are you ok with the _p arguments?
Yes, LGTM.
--alex
Also, can I get a second reviewer please.
thanks,
Chris
On 1/10/20 3:00 PM, Chris Plummer wrote:
Hi Alex,
I'll fix the mistakes in MethodImpl.c and ReferenceTypeImpl.c. As for
the "_p" suffix, it means the argument is a pointer type that a value
will be returned in. I've seen this used elsewhere in hotspot. For
example VM_RedefineClasses::merge_constant_pools() and
ObjectSynchronizer::deflate_monitor_list().
bool VM_RedefineClasses::merge_constant_pools(const
constantPoolHandle& old_cp,
const constantPoolHandle& scratch_cp, constantPoolHandle
*merge_cp_p,
int *merge_cp_length_p, TRAPS) {
int ObjectSynchronizer::deflate_monitor_list(ObjectMonitor** list_p,
ObjectMonitor** free_head_p,
ObjectMonitor**
free_tail_p) {
thanks,
Chris
On 1/10/20 2:12 PM, Alex Menkov wrote:
Hi Chris,
Thanks for making the code more "typed" (this "void*" arrays are
error prone).
Looks good in general, some minor comments:
MethodImpl.c
- command names starts with lower case letters
ReferenceTypeImpl.c
- please fix indentation for command definitions
debugDispatch.h/.c
+debugDispatch_getHandler(int cmdSetNum, int cmdNum, const char
**cmdSetName_p, const char **cmdName_p)
What are the "_p" suffixes for? to show that this are pointers?
To me this doesn't make much sense.
--alex
On 01/10/2020 11:27, Chris Plummer wrote:
Hello,
Please review the following
https://bugs.openjdk.java.net/browse/JDK-8236913
http://cr.openjdk.java.net/~cjplummer/8236913/webrev.00/
The debug agent has logging support that will trace all jdwp
commands coming in. Currently all it traces is the command set
number and the command number within that command set. So you see
something like:
[#|10.01.2020 06:27:24.366
GMT|FINEST|J2SE1.5|jdwp|LOC=MISC:"debugLoop.c":240;;PID=12719;THR=t@915490560|:Command
set 1, command 9|#]
I've added support for including the name of the command set and
command, so now you see:
[#|10.01.2020 06:27:24.366
GMT|FINEST|J2SE1.5|jdwp|LOC=MISC:"debugLoop.c":240;;PID=12719;THR=t@915490560|:Command
set VirtualMachine(1), command Resume(9)|#]
So in this case command set 1 represents VirtualMachine and command
9 is the Resume command.
I was initially going to leverage jdwp.spec which is already
processed by build.tools.jdwpgen.Main to produce JDWP.java and
JDWPCommands.h. However, I could see it was more of a challenge than
I initially hoped. Also, the main advantage would have been not
having to hard code arrays of command names, but we already have
harded coded arrays of function pointers to handle the various jdwp
commands, so I just replaced these with a more specialized arrays
that also include the names of the commands. As an example, we used
to have:
void *ArrayReference_Cmds[] = { (void *)0x3
,(void *)length
,(void *)getValues
,(void *)setValues};
Now we have:
CommandSet ArrayReference_Cmds = {
3, "ArrayReference",
{
{length, "Length"},
{getValues, "GetValues"},
{setValues, "SetValues"}
}
};
So no worse w.r.t. hard coding things that could be generated off
the spec, and it cleans up some ugly casts also. The CommandSet
typedef can be found in debugDispatch.h.
All the header files except for debugDispatch.h have the same
pattern for changes, so they are pretty easy to review
All .c files except debugDispatch.c and debugLoop.c also have the
same pattern. Note some command handler function names are not the
same as the command name. If you want to double check command set
names and command names, you can find the spec here:
https://docs.oracle.com/javase/9/docs/specs/jdwp/jdwp-protocol.html
In ReferenceTypeImpl.c I fixed a typo in the method() prototype. It
had an extra argument which I think was a very old copy-n-paste bug
from the method1() prototype. This was caught because the command
handler functions are now directly assigned to a CommandHandler type
rather than cast. The cast was hiding this bug.
I tested by doing a test run where MISC logging was enabled by
default. All jdwp, jdb, and jdi tests were run in this way and passed.
thanks,
Chris
Re: serviceability agent : problems when using gcc LTO (link time optimization)
On 14/01/2020 19:57, Baesken, Matthias wrote: > Hello Magnus and Aleksei, thanks for the input . > > The times you provided really look like they make a big difference at least > for people often building minimal-vm . > Guess I have to measure myself a bit (maybe the difference is not that big > on our linux s390x / ppc64(le) ) . > >> If the change to enable lto by default is proposed, what would be the >> recommended strategy for development? >> > Probably we should a) do not enable it by default but just make sure it > can be enabled easily and works for the minimal-vm That would be welcome. I have high hopes to LTO the VM some time by default, and the tendency observed is that the compiler time overhead for GCC becomes smaller. At the same time there is no reason why vendors that invested in testing and can absorb the build time hit could provide binaries with LTO built VMs by passing an additional option flag. > or b) take it easy to disable it for local development. > > Best regards, Matthias > > > >> Magnus, Matthias, >> >> for me, lto is a little heavyweight for development. x86_64 build time >> with gcc 7: >> >> Server 1m32.484s >> Server+Minimal 1m42.166s >> Server+Minimal (--with-jvm-features="link-time-opt") 5m29.422s >> >> If the change to enable lto by default is proposed, what would be the >> recommended strategy for development? >> >> For ARM32 Minimal, please keep in mind that it's not uncommon to disable >> LTO plugin in commodity ARM32 gcc compiler distributions, so for some it >> does not matter what settings we have in OpenJDK. I believe there could >> be other reasons for that on top of build time (bugs?). >>
RE: serviceability agent : problems when using gcc LTO (link time optimization)
Hello Magnus and Aleksei, thanks for the input . The times you provided really look like they make a big difference at least for people often building minimal-vm . Guess I have to measure myself a bit (maybe the difference is not that big on our linux s390x / ppc64(le) ) . > > If the change to enable lto by default is proposed, what would be the > recommended strategy for development? > Probably we should a) do not enable it by default but just make sure it can be enabled easily and works for the minimal-vm or b) take it easy to disable it for local development. Best regards, Matthias > > Magnus, Matthias, > > for me, lto is a little heavyweight for development. x86_64 build time > with gcc 7: > > Server 1m32.484s > Server+Minimal 1m42.166s > Server+Minimal (--with-jvm-features="link-time-opt") 5m29.422s > > If the change to enable lto by default is proposed, what would be the > recommended strategy for development? > > For ARM32 Minimal, please keep in mind that it's not uncommon to disable > LTO plugin in commodity ARM32 gcc compiler distributions, so for some it > does not matter what settings we have in OpenJDK. I believe there could > be other reasons for that on top of build time (bugs?). >
Re: serviceability agent : problems when using gcc LTO (link time optimization)
Magnus, Matthias, for me, lto is a little heavyweight for development. x86_64 build time with gcc 7: Server 1m32.484s Server+Minimal 1m42.166s Server+Minimal (--with-jvm-features="link-time-opt") 5m29.422s If the change to enable lto by default is proposed, what would be the recommended strategy for development? For ARM32 Minimal, please keep in mind that it's not uncommon to disable LTO plugin in commodity ARM32 gcc compiler distributions, so for some it does not matter what settings we have in OpenJDK. I believe there could be other reasons for that on top of build time (bugs?). -Aleksei On 14/01/2020 17:04, Magnus Ihse Bursie wrote: > On 2020-01-14 13:49, Baesken, Matthias wrote: >> >> Hi Magnus, thanks for the info , I already noticed yesterday the >> setting for arm-32 in the minimal build . >> >> Do you think we could set it too for the other Linux platforms in >> the minimal build ( serviceability agent is not supported there as >> well so the observed issue wouldn’t be a problem). >> > > You mean if you could enable it on your builds without any issues? I'd > guess so, but I don't know. Just try it: > --with-jvm-features="link-time-opt". > > If you mean that it should be turned on by default on minimal builds > for all platforms? No, I don't think that's a good idea. The link time > is really a killer. I remember arm-32 going from like a couple of > minutes to half an hour for linking libjvm.so. > > Things might be different with gold, though. I know they have done > work with at least some kind of "lightweight" LTO, that might be worth > at least looking into. > > /Magnus > > >> Best regards, Matthias >> >> On 2020-01-10 11:01, Baesken, Matthias wrote: >> >> Hello, I recently looked into the gcc lto optimization mode >> (see for some >> detailshttps://gcc.gnu.org/onlinedocs/gccint/LTO-Overview.html >> andhttp://hubicka.blogspot.com/2019/05/gcc-9-link-time-and-inter-procedural.html >> ). >> >> This mode can lead to more compact binaries (~10% smaller) , it >> also might bring small performance improvements but that wasn't my >> (main) goal . >> >> The changes for this are rather small , one needs to use a recent >> gcc , add -flto to the compile flags , for example >> >> --- a/make/autoconf/flags-cflags.m4 Wed Jan 01 03:08:45 2020 >> +0100 >> >> +++ b/make/autoconf/flags-cflags.m4 Wed Jan 08 17:39:10 2020 +0100 >> >> @@ -530,8 +530,13 @@ >> >> fi >> >> if test "x$TOOLCHAIN_TYPE" = xgcc; then >> >> - TOOLCHAIN_CFLAGS_JVM="$TOOLCHAIN_CFLAGS_JVM -fcheck-new >> -fstack-protector" >> >> - TOOLCHAIN_CFLAGS_JDK="-pipe -fstack-protector" >> >> + TOOLCHAIN_CFLAGS_JVM="$TOOLCHAIN_CFLAGS_JVM -fcheck-new >> -fstack-protector -flto" >> >> + TOOLCHAIN_CFLAGS_JDK="-pipe -fstack-protector -flto" >> >> and you have to make sure to use gcc-ar and gcc-nm >> instead of ar / nm . >> >> Build and test(s) work, however with one exception. >> >> The serviceability tests like serviceability/sa seems to >> rely heavily on the "normal" structure of libjvm.so (from >> what I understand e.g. in LinuxVtblAccess it is attempted to >> access internal symbols like _ZTV ). >> >> Errors in the sa tests look like : >> >> java.lang.InternalError: Metadata does not appear to be polymorphic >> >> at >> jdk.hotspot.agent/sun.jvm.hotspot.types.basic.BasicTypeDataBase.findDynamicTypeForAddress(BasicTypeDataBase.java:279) >> >> at >> jdk.hotspot.agent/sun.jvm.hotspot.runtime.VirtualBaseConstructor.instantiateWrapperFor(VirtualBaseConstructor.java:102) >> >> at >> jdk.hotspot.agent/sun.jvm.hotspot.oops.Metadata.instantiateWrapperFor(Metadata.java:74) >> >> at >> jdk.hotspot.agent/sun.jvm.hotspot.memory.SystemDictionary.getClassLoaderKlass(SystemDictionary.java:96) >> >> at >> jdk.hotspot.agent/sun.jvm.hotspot.tools.ClassLoaderStats.printClassLoaderStatistics(ClassLoaderStats.java:93) >> >> at >> jdk.hotspot.agent/sun.jvm.hotspot.tools.ClassLoaderStats.run(ClassLoaderStats.java:78) >> >> at >> jdk.hotspot.agent/sun.jvm.hotspot.tools.JMap.run(JMap.java:115) >> >> at >> jdk.hotspot.agent/sun.jvm.hotspot.tools.Tool.startInternal(Tool.java:262) >> >> at >> jdk.hotspot.agent/sun.jvm.hotspot.tools.Tool.start(Tool.java:225) >> >> at >> jdk.hotspot.agent/sun.jvm.hotspot.tools.Tool.execute(Tool.java:118) >> >> at >> jdk.hotspot.agent/sun.jvm.hotspot.tools.JMap.main(JMap.java:176) >> >> at >> jdk.hotspot.agent/sun.jvm.hotspot.SALauncher.runJMAP(SALauncher.java:321) >> >> at >> jdk.hotspot.agent/sun.jvm.hotspot.SALauncher.main(SALauncher.java:406) >> >> Has anyone experimented with LTO optimization ? >> >> >> Hi Matthias, >> >> We used to have LTO enabled on the old, closed-source Oracle arm-32 >> builds.
Re: serviceability agent : problems when using gcc LTO (link time optimization)
On 2020-01-14 13:49, Baesken, Matthias wrote: Hi Magnus, thanks for the info , I already noticed yesterday the setting for arm-32 in the minimal build . Do you think we could set it too for the other Linux platforms in the minimal build ( serviceability agent is not supported there as well so the observed issue wouldn’t be a problem). You mean if you could enable it on your builds without any issues? I'd guess so, but I don't know. Just try it: --with-jvm-features="link-time-opt". If you mean that it should be turned on by default on minimal builds for all platforms? No, I don't think that's a good idea. The link time is really a killer. I remember arm-32 going from like a couple of minutes to half an hour for linking libjvm.so. Things might be different with gold, though. I know they have done work with at least some kind of "lightweight" LTO, that might be worth at least looking into. /Magnus Best regards, Matthias On 2020-01-10 11:01, Baesken, Matthias wrote: Hello, I recently looked into the gcc lto optimization mode (see for some detailshttps://gcc.gnu.org/onlinedocs/gccint/LTO-Overview.html andhttp://hubicka.blogspot.com/2019/05/gcc-9-link-time-and-inter-procedural.html ). This mode can lead to more compact binaries (~10% smaller) , it also might bring small performance improvements but that wasn't my (main) goal . The changes for this are rather small , one needs to use a recent gcc , add -flto to the compile flags , for example --- a/make/autoconf/flags-cflags.m4 Wed Jan 01 03:08:45 2020 +0100 +++ b/make/autoconf/flags-cflags.m4 Wed Jan 08 17:39:10 2020 +0100 @@ -530,8 +530,13 @@ fi if test "x$TOOLCHAIN_TYPE" = xgcc; then - TOOLCHAIN_CFLAGS_JVM="$TOOLCHAIN_CFLAGS_JVM -fcheck-new -fstack-protector" - TOOLCHAIN_CFLAGS_JDK="-pipe -fstack-protector" + TOOLCHAIN_CFLAGS_JVM="$TOOLCHAIN_CFLAGS_JVM -fcheck-new -fstack-protector -flto" + TOOLCHAIN_CFLAGS_JDK="-pipe -fstack-protector -flto" and you have to make sure to use gcc-ar and gcc-nm instead of ar / nm . Build and test(s) work, however with one exception. The serviceability tests like serviceability/sa seems to rely heavily on the "normal" structure of libjvm.so (from what I understand e.g. in LinuxVtblAccess it is attempted to access internal symbols like _ZTV ). Errors in the sa tests look like : java.lang.InternalError: Metadata does not appear to be polymorphic at jdk.hotspot.agent/sun.jvm.hotspot.types.basic.BasicTypeDataBase.findDynamicTypeForAddress(BasicTypeDataBase.java:279) at jdk.hotspot.agent/sun.jvm.hotspot.runtime.VirtualBaseConstructor.instantiateWrapperFor(VirtualBaseConstructor.java:102) at jdk.hotspot.agent/sun.jvm.hotspot.oops.Metadata.instantiateWrapperFor(Metadata.java:74) at jdk.hotspot.agent/sun.jvm.hotspot.memory.SystemDictionary.getClassLoaderKlass(SystemDictionary.java:96) at jdk.hotspot.agent/sun.jvm.hotspot.tools.ClassLoaderStats.printClassLoaderStatistics(ClassLoaderStats.java:93) at jdk.hotspot.agent/sun.jvm.hotspot.tools.ClassLoaderStats.run(ClassLoaderStats.java:78) at jdk.hotspot.agent/sun.jvm.hotspot.tools.JMap.run(JMap.java:115) at jdk.hotspot.agent/sun.jvm.hotspot.tools.Tool.startInternal(Tool.java:262) at jdk.hotspot.agent/sun.jvm.hotspot.tools.Tool.start(Tool.java:225) at jdk.hotspot.agent/sun.jvm.hotspot.tools.Tool.execute(Tool.java:118) at jdk.hotspot.agent/sun.jvm.hotspot.tools.JMap.main(JMap.java:176) at jdk.hotspot.agent/sun.jvm.hotspot.SALauncher.runJMAP(SALauncher.java:321) at jdk.hotspot.agent/sun.jvm.hotspot.SALauncher.main(SALauncher.java:406) Has anyone experimented with LTO optimization ? Hi Matthias, We used to have LTO enabled on the old, closed-source Oracle arm-32 builds. There is still a "link-time-opt" JVM feature present; afaik it still works and adds the -flto flag. The main drawback of this is the *extremely* long link times of libjvm.so. I don't think servicability was ever supported for that platform, so I'm not surprised this does not work. /Magnus And to the serviceability agent experts - any idea how to make the jdk.hotspot.agent more independent from optimization settings ? Best regards, Matthias
RE: serviceability agent : problems when using gcc LTO (link time optimization)
Hi Magnus, thanks for the info , I already noticed yesterday the setting for arm-32 in the minimal build . Do you think we could set it too for the other Linux platforms in the minimal build ( serviceability agent is not supported there as well so the observed issue wouldn’t be a problem). Best regards, Matthias On 2020-01-10 11:01, Baesken, Matthias wrote: Hello, I recently looked into the gcc lto optimization mode (see for some details https://gcc.gnu.org/onlinedocs/gccint/LTO-Overview.html and http://hubicka.blogspot.com/2019/05/gcc-9-link-time-and-inter-procedural.html ). This mode can lead to more compact binaries (~10% smaller) , it also might bring small performance improvements but that wasn't my (main) goal . The changes for this are rather small , one needs to use a recent gcc , add -flto to the compile flags , for example --- a/make/autoconf/flags-cflags.m4 Wed Jan 01 03:08:45 2020 +0100 +++ b/make/autoconf/flags-cflags.m4 Wed Jan 08 17:39:10 2020 +0100 @@ -530,8 +530,13 @@ fi if test "x$TOOLCHAIN_TYPE" = xgcc; then -TOOLCHAIN_CFLAGS_JVM="$TOOLCHAIN_CFLAGS_JVM -fcheck-new -fstack-protector" -TOOLCHAIN_CFLAGS_JDK="-pipe -fstack-protector" +TOOLCHAIN_CFLAGS_JVM="$TOOLCHAIN_CFLAGS_JVM -fcheck-new -fstack-protector -flto" +TOOLCHAIN_CFLAGS_JDK="-pipe -fstack-protector -flto" and you have to make sure to use gcc-ar and gcc-nm instead of ar / nm . Build and test(s) work, however with one exception. The serviceability tests like serviceability/sa seems to rely heavily on the "normal" structure of libjvm.so (from what I understand e.g. in LinuxVtblAccess it is attempted to access internal symbols like _ZTV ). Errors in the sa tests look like : java.lang.InternalError: Metadata does not appear to be polymorphic at jdk.hotspot.agent/sun.jvm.hotspot.types.basic.BasicTypeDataBase.findDynamicTypeForAddress(BasicTypeDataBase.java:279) at jdk.hotspot.agent/sun.jvm.hotspot.runtime.VirtualBaseConstructor.instantiateWrapperFor(VirtualBaseConstructor.java:102) at jdk.hotspot.agent/sun.jvm.hotspot.oops.Metadata.instantiateWrapperFor(Metadata.java:74) at jdk.hotspot.agent/sun.jvm.hotspot.memory.SystemDictionary.getClassLoaderKlass(SystemDictionary.java:96) at jdk.hotspot.agent/sun.jvm.hotspot.tools.ClassLoaderStats.printClassLoaderStatistics(ClassLoaderStats.java:93) at jdk.hotspot.agent/sun.jvm.hotspot.tools.ClassLoaderStats.run(ClassLoaderStats.java:78) at jdk.hotspot.agent/sun.jvm.hotspot.tools.JMap.run(JMap.java:115) at jdk.hotspot.agent/sun.jvm.hotspot.tools.Tool.startInternal(Tool.java:262) at jdk.hotspot.agent/sun.jvm.hotspot.tools.Tool.start(Tool.java:225) at jdk.hotspot.agent/sun.jvm.hotspot.tools.Tool.execute(Tool.java:118) at jdk.hotspot.agent/sun.jvm.hotspot.tools.JMap.main(JMap.java:176) at jdk.hotspot.agent/sun.jvm.hotspot.SALauncher.runJMAP(SALauncher.java:321) at jdk.hotspot.agent/sun.jvm.hotspot.SALauncher.main(SALauncher.java:406) Has anyone experimented with LTO optimization ? Hi Matthias, We used to have LTO enabled on the old, closed-source Oracle arm-32 builds. There is still a "link-time-opt" JVM feature present; afaik it still works and adds the -flto flag. The main drawback of this is the *extremely* long link times of libjvm.so. I don't think servicability was ever supported for that platform, so I'm not surprised this does not work. /Magnus And to the serviceability agent experts - any idea how to make the jdk.hotspot.agent more independent from optimization settings ? Best regards, Matthias
Re: serviceability agent : problems when using gcc LTO (link time optimization)
On 2020-01-10 11:01, Baesken, Matthias wrote:
Hello, I recently looked into the gcc lto optimization mode (see for some
details https://gcc.gnu.org/onlinedocs/gccint/LTO-Overview.html and
http://hubicka.blogspot.com/2019/05/gcc-9-link-time-and-inter-procedural.html
).
This mode can lead to more compact binaries (~10% smaller) , it also might
bring small performance improvements but that wasn't my (main) goal .
The changes for this are rather small , one needs to use a recent gcc , add
-flto to the compile flags , for example
--- a/make/autoconf/flags-cflags.m4 Wed Jan 01 03:08:45 2020 +0100
+++ b/make/autoconf/flags-cflags.m4 Wed Jan 08 17:39:10 2020 +0100
@@ -530,8 +530,13 @@
fi
if test "x$TOOLCHAIN_TYPE" = xgcc; then
-TOOLCHAIN_CFLAGS_JVM="$TOOLCHAIN_CFLAGS_JVM -fcheck-new -fstack-protector"
-TOOLCHAIN_CFLAGS_JDK="-pipe -fstack-protector"
+TOOLCHAIN_CFLAGS_JVM="$TOOLCHAIN_CFLAGS_JVM -fcheck-new -fstack-protector
-flto"
+TOOLCHAIN_CFLAGS_JDK="-pipe -fstack-protector -flto"
and you have to make sure to use gcc-ar and gcc-nm instead of ar
/ nm .
Build and test(s) work, however with one exception.
The serviceability tests like serviceability/sa seems to rely heavily on the
"normal" structure of libjvm.so (from what I understand e.g. in
LinuxVtblAccess it is attempted to access internal symbols like _ZTV ).
Errors in the sa tests look like :
java.lang.InternalError: Metadata does not appear to be polymorphic
at
jdk.hotspot.agent/sun.jvm.hotspot.types.basic.BasicTypeDataBase.findDynamicTypeForAddress(BasicTypeDataBase.java:279)
at
jdk.hotspot.agent/sun.jvm.hotspot.runtime.VirtualBaseConstructor.instantiateWrapperFor(VirtualBaseConstructor.java:102)
at
jdk.hotspot.agent/sun.jvm.hotspot.oops.Metadata.instantiateWrapperFor(Metadata.java:74)
at
jdk.hotspot.agent/sun.jvm.hotspot.memory.SystemDictionary.getClassLoaderKlass(SystemDictionary.java:96)
at
jdk.hotspot.agent/sun.jvm.hotspot.tools.ClassLoaderStats.printClassLoaderStatistics(ClassLoaderStats.java:93)
at
jdk.hotspot.agent/sun.jvm.hotspot.tools.ClassLoaderStats.run(ClassLoaderStats.java:78)
at jdk.hotspot.agent/sun.jvm.hotspot.tools.JMap.run(JMap.java:115)
at
jdk.hotspot.agent/sun.jvm.hotspot.tools.Tool.startInternal(Tool.java:262)
at jdk.hotspot.agent/sun.jvm.hotspot.tools.Tool.start(Tool.java:225)
at jdk.hotspot.agent/sun.jvm.hotspot.tools.Tool.execute(Tool.java:118)
at jdk.hotspot.agent/sun.jvm.hotspot.tools.JMap.main(JMap.java:176)
at
jdk.hotspot.agent/sun.jvm.hotspot.SALauncher.runJMAP(SALauncher.java:321)
at
jdk.hotspot.agent/sun.jvm.hotspot.SALauncher.main(SALauncher.java:406)
Has anyone experimented with LTO optimization ?
Hi Matthias,
We used to have LTO enabled on the old, closed-source Oracle arm-32
builds. There is still a "link-time-opt" JVM feature present; afaik it
still works and adds the -flto flag. The main drawback of this is the
*extremely* long link times of libjvm.so.
I don't think servicability was ever supported for that platform, so I'm
not surprised this does not work.
/Magnus
And to the serviceability agent experts - any idea how to make the
jdk.hotspot.agent more independent from optimization settings ?
Best regards, Matthias
Re: RFR(M): 8236913: debug agent's jdwp command logging should include the command set name and command name
Hi Chris,
It looks good to me modulo the comments from Alex.
I'm ok with the _p arguments.
Probably, you've already considered to return Command (instead of CommandHandler) from the
debugDispatch_getHandler().
It allows to get rid of the cmdName_p argument but gives a
non-symmetry in getting names.
I think, it would not give any real advantage, so I'm ok with
current approach.
Thanks,
Serguei
On 1/13/20 20:11, Chris Plummer wrote:
Hi
Alex,
Are you ok with the _p arguments?
Also, can I get a second reviewer please.
thanks,
Chris
On 1/10/20 3:00 PM, Chris Plummer wrote:
Hi Alex,
I'll fix the mistakes in MethodImpl.c and ReferenceTypeImpl.c.
As for the "_p" suffix, it means the argument is a pointer type
that a value will be returned in. I've seen this used elsewhere
in hotspot. For example
VM_RedefineClasses::merge_constant_pools() and
ObjectSynchronizer::deflate_monitor_list().
bool VM_RedefineClasses::merge_constant_pools(const
constantPoolHandle& old_cp,
const constantPoolHandle& scratch_cp,
constantPoolHandle *merge_cp_p,
int *merge_cp_length_p, TRAPS) {
int ObjectSynchronizer::deflate_monitor_list(ObjectMonitor**
list_p,
ObjectMonitor**
free_head_p,
ObjectMonitor**
free_tail_p) {
thanks,
Chris
On 1/10/20 2:12 PM, Alex Menkov wrote:
Hi Chris,
Thanks for making the code more "typed" (this "void*" arrays
are error prone).
Looks good in general, some minor comments:
MethodImpl.c
- command names starts with lower case letters
ReferenceTypeImpl.c
- please fix indentation for command definitions
debugDispatch.h/.c
+debugDispatch_getHandler(int cmdSetNum, int cmdNum, const
char **cmdSetName_p, const char **cmdName_p)
What are the "_p" suffixes for? to show that this are
pointers?
To me this doesn't make much sense.
--alex
On 01/10/2020 11:27, Chris Plummer wrote:
Hello,
Please review the following
https://bugs.openjdk.java.net/browse/JDK-8236913
http://cr.openjdk.java.net/~cjplummer/8236913/webrev.00/
The debug agent has logging support that will trace all jdwp
commands coming in. Currently all it traces is the command
set number and the command number within that command set.
So you see something like:
[#|10.01.2020 06:27:24.366
GMT|FINEST|J2SE1.5|jdwp|LOC=MISC:"debugLoop.c":240;;PID=12719;THR=t@915490560|:Command
set 1, command 9|#]
I've added support for including the name of the command set
and command, so now you see:
[#|10.01.2020 06:27:24.366
GMT|FINEST|J2SE1.5|jdwp|LOC=MISC:"debugLoop.c":240;;PID=12719;THR=t@915490560|:Command
set VirtualMachine(1), command Resume(9)|#]
So in this case command set 1 represents VirtualMachine and
command 9 is the Resume command.
I was initially going to leverage jdwp.spec which is already
processed by build.tools.jdwpgen.Main to produce JDWP.java
and JDWPCommands.h. However, I could see it was more of a
challenge than I initially hoped. Also, the main advantage
would have been not having to hard code arrays of command
names, but we already have harded coded arrays of function
pointers to handle the various jdwp commands, so I just
replaced these with a more specialized arrays that also
include the names of the commands. As an example, we used to
have:
void *ArrayReference_Cmds[] = { (void *)0x3
,(void *)length
,(void *)getValues
,(void *)setValues};
