Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8263572: Output from jstack mixed mode is misaligned #3004

Closed
wants to merge 2 commits into from

Conversation

jyukutyo
Copy link
Contributor

@jyukutyo jyukutyo commented Mar 15, 2021

When running jstack with mixed option, the output of the lines that doesn't have an address are misaligned as followings.

$ sudo jhsdb jstack --mixed --pid 8281
----------------- 8330 -----------------
"event-handler" #20 daemon prio=5 tid=0x00007f2384838490 nid=0x208a in Object.wait() [0x00007f23585ec000]
   java.lang.Thread.State: WAITING (on object monitor)
   JavaThread state: _thread_blocked
0x00007f238da899f3      __pthread_cond_wait + 0x243
0x00007f238c63a26b      os::PlatformEvent::park() + 0x8b
0x00007f238c5e689d      ObjectMonitor::wait(long, bool, Thread*) + 0xfed
0x00007f238ca1f1d6      ObjectSynchronizer::wait(Handle, long, Thread*) + 0x96
0x00007f238c0b4d71      JVM_MonitorWait + 0x241
0x00007f237cb246b7      java.lang.Object.wait(long) + 0xd7 (Native method)
0x00007f2375777344      * java.lang.Object.wait() bci:2 line:338 (Compiled frame)
* com.sun.tools.jdi.EventQueueImpl.removeUnfiltered(long) bci:64 line:190 (Compiled frame)
* com.sun.tools.jdi.EventQueueImpl.remove(long) bci:18 line:97 (Interpreted frame)
0x00007f2375009543      * com.sun.tools.jdi.EventQueueImpl.remove() bci:2 line:83 (Interpreted frame)

This pull request aligns the indentation. So It will be able to improve readability.

In this pull request the address part is filled with white spaces. The amount of space is calculated from VM's address size and lastly 2, which is the length of "0x", is added.

jtreg test

$ sudo make run-test TEST=serviceability/sa/TestJhsdbJstackMixed.java

$ cat ./jdk/build/linux-x86_64-server-fastdebug/test-results/jtreg_test_hotspot_jtreg_serviceability_sa_TestJhsdbJstackMixed_java/text/summary.txt
serviceability/sa/TestJhsdbJstackMixed.java  Passed. Execution successful

Output Confirmation

I checked the output on 64-bit and 32-bit JDK.

First, I ran JShell. Next, on another shell, I ran jstack with mixed option for the JShell process and checked the output.

Linux 64-bit JDK

 $ ./jdk/build/linux-x86_64-server-fastdebug/jdk/bin/jshell
|  Welcome to JShell -- Version 16-internal
|  For an introduction type: /help intro

jshell>

$ ./jdk/build/linux-x86_64-server-fastdebug/jdk/bin/jhsdb jstack --mixed --pid 19272
...
----------------- 19324 -----------------
"output reader" #21 daemon prio=5 tid=0x00007f2ae4783760 nid=0x4b7c runnable [0x00007f2a86efd000]
   java.lang.Thread.State: RUNNABLE
   JavaThread state: _thread_in_native
0x00007f2aed5d2394      __libc_read + 0x44
0x00007f2ac450f93b      Java_sun_nio_ch_SocketDispatcher_read0 + 0x2b
0x00007f2adcb2045a      sun.nio.ch.SocketDispatcher.read0(java.io.FileDescriptor, long, int) + 0xfa (Native method)
0x00007f2ad573e024      * sun.nio.ch.SocketDispatcher.read(java.io.FileDescriptor, long, int) bci:4 line:47 (Compiled frame)
                        * sun.nio.ch.NioSocketImpl.tryRead(java.io.FileDescriptor, byte[], int, int) bci:45 line:261 (Compiled frame)
                        * sun.nio.ch.NioSocketImpl.implRead(byte[], int, int) bci:99 line:312 (Compiled frame)
                        * sun.nio.ch.NioSocketImpl.read(byte[], int, int) bci:54 line:350 (Compiled frame)
                        * sun.nio.ch.NioSocketImpl$1.read(byte[], int, int) bci:7 line:803 (Compiled frame)
                        * java.net.Socket$SocketInputStream.read(byte[], int, int) bci:7 line:961 (Compiled frame)
                        * java.net.Socket$SocketInputStream.read() bci:8 line:956 (Compiled frame)
                        * java.io.FilterInputStream.read() bci:4 line:82 (Interpreted frame)
0x00007f2ad50094fe      * jdk.jshell.execution.DemultiplexInput.run() bci:4 line:58 (Interpreted frame)
0x00007f2ad5000d4a      <StubRoutines>
...

Windows 32-bit JDK

$ ./jdk/build/windows-x86-server-fastdebug/jdk/bin/jhsdb.exe jstack --mixed --pid 78808
...
----------------- 29 -----------------
"output reader" #17 daemon prio=5 tid=0x2711c3b8 nid=0x13178 runnable [0x27d6f000]
   java.lang.Thread.State: RUNNABLE
   JavaThread state: _thread_in_native
0x77ae2f5c      ntdll!NtWaitForSingleObject + 0xc
0x6dc5cb18      mswsock!sethostname + 0xbdc8
0x753cdecc      WS2_32!WSARecv + 0x11c
0x6c0d35f8      nio!Java_sun_nio_ch_SocketDispatcher_read0 + 0x48
0x02d15ee9      sun.nio.ch.SocketDispatcher.read0(java.io.FileDescriptor, long, int) + 0xa9 (Native method)
0x02d16540      * sun.nio.ch.SocketDispatcher.read(java.io.FileDescriptor, long, int) bci:4 line:46 (Compiled frame)
                * sun.nio.ch.NioSocketImpl.tryRead(java.io.FileDescriptor, byte[], int, int) bci:45 line:261 (Compiled frame)
                * sun.nio.ch.NioSocketImpl.implRead(byte[], int, int) bci:99 line:312 (Compiled frame)
                * sun.nio.ch.NioSocketImpl.read(byte[], int, int) bci:54 line:350 (Compiled frame)
                * sun.nio.ch.NioSocketImpl$1.read(byte[], int, int) bci:7 line:803 (Compiled frame)
                * java.net.Socket$SocketInputStream.read(byte[], int, int) bci:7 line:961 (Compiled frame)
                * java.net.Socket$SocketInputStream.read() bci:8 line:956 (Compiled frame)
                * java.io.FilterInputStream.read() bci:4 line:82 (Interpreted frame)
0x02ac6804      * jdk.jshell.execution.DemultiplexInput.run() bci:4 line:58 (Interpreted frame)
0x02ac0cdc      <StubRoutines>
...

Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8263572: Output from jstack mixed mode is misaligned

Reviewers

Download

To checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3004/head:pull/3004
$ git checkout pull/3004

To update a local copy of the PR:
$ git checkout pull/3004
$ git pull https://git.openjdk.java.net/jdk pull/3004/head

@bridgekeeper
Copy link

bridgekeeper bot commented Mar 15, 2021

👋 Welcome back ksakata! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr Pull request is ready for review label Mar 15, 2021
@openjdk
Copy link

openjdk bot commented Mar 15, 2021

@jyukutyo The following label will be automatically applied to this pull request:

  • serviceability

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the serviceability serviceability-dev@openjdk.org label Mar 15, 2021
@mlbridge
Copy link

mlbridge bot commented Mar 15, 2021

Webrevs

Copy link
Contributor

@plummercj plummercj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What testing have you done?

Please update the copyright to 2021.

I noticed that in you original jstack output there is only 1 space after the address, and now there appears to be multiple spaces. However, when I produce the jstack out there's already multiple spaces. Was there some formatting issue with your original output?

@@ -168,6 +169,9 @@ public void run(PrintStream out, Debugger dbg) {
if (names != null && names.length != 0) {
// print java frame(s)
for (int i = 0; i < names.length; i++) {
if (0 < i) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use i > 0

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure!

@@ -82,6 +82,7 @@ public void run(PrintStream out, Debugger dbg) {
return;
}
final boolean cdbgCanDemangle = cdbg.canDemangle();
String fillerForAddress = " ".repeat(2 + (2 << VM.getVM().getLogAddressSize())) + "\t";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use 2 * VM.getVM().getAddressSize() rather than (2 << VM.getVM().getLogAddressSize()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. I missed that method. I've changed the code.

@jyukutyo
Copy link
Contributor Author

Thank you for your comment, @plummercj.

What testing have you done?

I added the content of the test that I've done in the body text.

Please update the copyright to 2021.

Sure!

I noticed that in you original jstack output there is only 1 space after the address, and now there appears to be multiple spaces. However, when I produce the jstack out there's already multiple spaces. Was there some formatting issue with your original output?

I'm sorry. It seemed that a tab after the address was lost on pasting. I pasted proper one once again. So there is no format issue after the address.

@plummercj
Copy link
Contributor

What testing have you done?

I added the content of the test that I've done in the body text.

I was referring to the running of existing tests. I just want to make sure the format changes don't cause any tests to fail. It looks like the only test we have that uses --mixed is test/hotspot/jtreg/serviceability/sa/TestJhsdbJstackMixed.java. This test is normally run as part of the git automated testing, but it is only run on linux, and I'm seeing the following in the test results:

TEST RESULT: Passed. Skipped: jtreg.SkippedException: SA Attach not expected to work. Ptrace attach not supported.

So it was skipped since the machine does not support Ptrace attach, and this was the only attempt to run the test.

@jyukutyo
Copy link
Contributor Author

Thank you. I understand it.

I ran that jtreg test by myself. TestJhsdbJstackMixed.java wasn't skipped and the execution was successful as follows.

$ sudo make run-test TEST=serviceability/sa/TestJhsdbJstackMixed.java

$ cat ./jdk/build/linux-x86_64-server-fastdebug/test-results/jtreg_test_hotspot_jtreg_serviceability_sa_TestJhsdbJstackMixed_java/text/summary.txt
serviceability/sa/TestJhsdbJstackMixed.java  Passed. Execution successful

I also added this test result to the body.

@openjdk
Copy link

openjdk bot commented Mar 19, 2021

@jyukutyo This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8263572: Output from jstack mixed mode is misaligned

Reviewed-by: cjplummer, sspitsyn

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 160 new commits pushed to the master branch:

  • b23228d: 8263914: CDS fails to find the default shared archive on x86_32
  • a5e7a89: 8263904: compiler/intrinsics/bmi/verifycode/BzhiTestI2L.java fails on x86_32
  • f62b100: 8263895: Test nsk/jvmti/GetThreadGroupChildren/getthrdgrpchld001/getthrdgrpchld001.cpp uses incorrect indices
  • f84b52b: 8263897: compiler/c2/aarch64/TestVolatilesSerial.java failed with "java.lang.RuntimeException: Wrong method"
  • f08bf4b: 8263891: Changes for 8076985 missed the fix.
  • b2df513: 8261785: Calling "main" method in anonymous nested class crashes the JVM
  • 840ab7b: 8263894: Convert defaultPrinter and printers fields to local variables
  • ba504fc: 8187450: JNI local refs exceeds capacity warning in NetworkInterface::getAll
  • 0abbfb2: 8263729: [test] divert spurious output away from stream under test in ProcessBuilder Basic test
  • 6c2220e: 8263861: Shenandoah: Remove unused member in ShenandoahGCStateResetter
  • ... and 150 more: https://git.openjdk.java.net/jdk/compare/ad1f60541940ae59da2af63015c5b5038832a676...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@plummercj, @sspitsyn) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Mar 19, 2021
@jyukutyo
Copy link
Contributor Author

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Mar 22, 2021
@openjdk
Copy link

openjdk bot commented Mar 22, 2021

@jyukutyo
Your change (at version 5a83d60) is now ready to be sponsored by a Committer.

Copy link
Contributor

@sspitsyn sspitsyn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Koichi,
It looks good to me. Thank you for the contribution!
Thanks,
Serguei

@jyukutyo
Copy link
Contributor Author

Hi Serguei,
Thank you! I'll continue to contribute to OpenJDK.

Now this pull request is approved by two reviewers. Can someone sponsor it?

Regards,
Koichi

@plummercj
Copy link
Contributor

/sponsor

@openjdk openjdk bot closed this Mar 23, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed sponsor Pull request is ready to be sponsored ready Pull request is ready to be integrated rfr Pull request is ready for review labels Mar 23, 2021
@openjdk
Copy link

openjdk bot commented Mar 23, 2021

@plummercj @jyukutyo Since your change was applied there have been 188 commits pushed to the master branch:

  • 47ef038: 8263905: Remove finalize methods for SocketInput/OutputStream
  • 1c9817b: 8261479: CDS runtime code should check exceptions
  • 087c8bf: 8264041: Incorrect comments for ParallelCompactData::summarize_dense_prefix
  • c087f3e: 8263995: Incorrect double-checked locking in Types.arraySuperType()
  • d7268fa: 8251942: PrintStream specification is not clear which flush method is automatically invoked
  • 8fa34e4: 8241619: (fs) Files.newByteChannel(path, Set.of(CREATE_NEW, READ)) does not throw a FileAlreadyExistsException when the file exists
  • e9321cd: 8263964: Redundant check in ObjectStartArray::object_starts_in_range
  • bd7a184: 8263442: Potential bug in jdk.internal.net.http.common.Utils.CONTEXT_RESTRICTED
  • 2335362: 8264032: Improve thread safety of Runtime.version()
  • 8c1ab38: 8263766: Confusing specification of JEditorPaneAccessibleHypertextSupport constructor
  • ... and 178 more: https://git.openjdk.java.net/jdk/compare/ad1f60541940ae59da2af63015c5b5038832a676...master

Your commit was automatically rebased without conflicts.

Pushed as commit 91d86e6.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated serviceability serviceability-dev@openjdk.org
3 participants