8247514: Improve clhsdb 'findpc' ability to determine what an address points to by improving PointerFinder and PointerLocation classes #2111

plummercj · 2021-01-17T03:57:59Z

See the bug for most details. A few notes here about some implementation details:

In the PointerLocation class, I added more consistency w.r.t. whether or not a newline is printed. It used to for some address types, but not others. Now it always does. And if you see a comment something like the following:

getTLAB().printOn(tty); // includes "\n"

That's just clarifying whether or not the printOn() method called will include the newline. Some do and some don't, and knowing what the various printOn() methods do makes getting the proper inclusion of the newline easier to understand.

I added verbose and printAddress boolean arguments to PointerLocation.printOn(). Currently they are always true. The false arguments will be used when I complete JDK-8250801, which will use PointerFinder/Location to show what each register points to.

The CR mentions that the main motivation for this work is for eventual replacement of the old clhsdb whatis command, which was implemented in javascript. It used to resolve DSO symbols, whereas findpc did not. The whatis code did this with the following:

  var dso = loadObjectContainingPC(addr);
  if (dso == null) {
    return ptrLoc.toString();
  }
  var sym = dso.closestSymbolToPC(addr);
  if (sym != null) {
    return sym.name + '+' + sym.offset;
  }

And now you'll see something similar in the PointerFinder code:

        loc.loadObject = cdbg.loadObjectContainingPC(a);
        if (loc.loadObject != null) {
            loc.nativeSymbol = loc.loadObject.closestSymbolToPC(a);
            return loc;
        }

Note that now that findpc does everything that whatis used to (and more), we don't really need to add a java version of whatis, but I'll probably do so anyway just help out people who are used to using the whatis command. That will be done using JDK-8244670

Progress

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

Issue

JDK-8247514: Improve clhsdb 'findpc' ability to determine what an address points to by improving PointerFinder and PointerLocation classes

Reviewers

Yasumasa Suenaga (@YaSuenag - Reviewer)
Kevin Walls (@kevinjwalls - Committer)

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2111/head:pull/2111
$ git checkout pull/2111

bridgekeeper · 2021-01-17T03:59:19Z

👋 Welcome back cjplummer! 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 · 2021-01-17T04:00:18Z

@plummercj The following labels will be automatically applied to this pull request:

hotspot-gc
serviceability

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

mlbridge · 2021-01-19T22:48:00Z

Webrevs

plummercj · 2021-01-25T20:00:41Z

Ping!

plummercj · 2021-02-02T19:48:57Z

Ping again.

YaSuenag

LGTM

openjdk · 2021-02-02T23:22:35Z

@plummercj 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:

8247514: Improve clhsdb 'findpc' ability to determine what an address points to by improving PointerFinder and PointerLocation classes

Reviewed-by: ysuenaga, kevinw

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 25 new commits pushed to the master branch:

3210095: 8261079: Fix support for @hidden in classes and interfaces
9c0ec8d: 8260941: Remove the conc_scan parameter for CardTable
da9895a: 8261499: Simplify HTML for javadoc links
0779add: 8255059: Regressions >5% in all Javadoc benchmarks in 16-b19
6a84ec6: 8260044: Parallel GC: Concurrent allocation after heap expansion may cause unnecessary full gc
92ff891: 8261593: Do not use NULL pointer as write buffer parameter in jfrEmergencyDump.cpp write_repository_files
60a2072: 8260431: com/sun/jdi/JdbOptions.java failed with "RuntimeException: 'prop[boo] = >foo<' missing from stdout/stderr"
bf47a47: 8261282: Lazy initialization of built-in ICC_Profile/ColorSpace classes is too lazy
f4cfd75: 8261510: Use RFC numbers and protocol titles in sun.security.ssl.SSLExtension comments
75c8489: 8261604: ProblemList jdk/dynalink/TypeConverterFactoryMemoryLeakTest.java
... and 15 more: https://git.openjdk.java.net/jdk/compare/cc5691c69ed2e71f99001a660c71c9b57c803e60...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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

kevinjwalls · 2021-02-10T17:52:59Z

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/PointerLocation.java

+            tty.format("In java stack [%s,%s,%s] for thread %s:\n   ",
+                       stackThread.getStackBase(), stackThread.lastSPDbg(),
+                       stackThread.getStackBase().addOffsetTo(-stackThread.getStackSize()),
+                       stackThread);


When we print a JavaThread, in the verbose block,
the final argument to tty.format in line 247, I wonder what that prints?

We then call printThreadInfoOn() which will first print the quoted thread name,
so maybe we don't need that item.
Or maybe we want the JavaThread.toString()?

stackThread.toString() ends up in VMObject.toString():

public String toString() { return getClass().getName() + "@" + addr; }

And here's an example output:

hsdb> + findpc 0x0000152f45df6000 Address 0x0000152f45df6000: In java stack [0x0000152f45df8000,0x0000152f45df6580,0x0000152f45cf7000] for thread sun.jvm.hotspot.runtime.JavaThread@0x0000152f3c026f70: "main" #1 prio=5 tid=0x0000152f3c026f70 nid=0x308e waiting on condition [0x0000152f45df6000] java.lang.Thread.State: TIMED_WAITING (sleeping) JavaThread state: _thread_blocked

So I think the stackThread argument is doing what was intended, and there is no duplication in the output.

Great, thanks.

kevinjwalls

Looks good, thanks.
(Comment in PointerLocation.java, treat as you see fit.)

openjdk · 2021-02-10T17:54:27Z

@plummercj this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout JDK-8247514
git fetch https://git.openjdk.java.net/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

…utput due to CDS logging or -Xcheck:jni warnings.

plummercj · 2021-02-12T00:02:44Z

@YaSuenag and @kevinjwalls I had to make a minor fix to the test. Can you please review it. The issued turned up when I ran some higher test tiers, one of which enabled CDS with some tracing and the other enabled -Xcheck:jni, which produced output due to JDK-8261607. Both caused extra output that resulted in improperly parsing the examine output and not actually finding that address that it produced. This was because there were lines of output before even issuing the examine command that matched the pattern being looked for. I made the pattern more specific by including the tid of the thread. I also cleaned up the comments around that code a bit. thanks.

kevinjwalls

Yes, joy of text processing. Looks good.

YaSuenag

LGTM

We may be able to use regex to collect any addresses from jstack output, but I'm not sure it makes the test code simpler...

plummercj · 2021-02-12T22:00:59Z

/integrate

openjdk · 2021-02-12T22:02:06Z

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

dc46aa8: 8261534: Test sun/security/pkcs11/KeyAgreement/IllegalPackageAccess.java fails on platforms where no nsslib artifacts are defined
f0d9829: 8261533: Java_sun_font_CFont_getCascadeList leaks memory according to Xcode
06170b7: 8261662: Rename compute_loader_lock_object
3dc6f52: 8261160: Add a deserialization JFR event
a305743: 8261660: AArch64: Race condition in stub code generation for LSE Atomics
28163a9: 8261652: Remove some dead comments from os_bsd_x86
6675775: 8253702: BigSur version number reported as 10.16, should be 11.nn
33fcd32: 8261659: JDK-8261027 causes a Tier1 validate-source failure
3aa1b4c: 8261623: reference to javac internals in Extern class
350303d: 8260221: java.util.Formatter throws wrong exception for mismatched flags in %% conversion
... and 34 more: https://git.openjdk.java.net/jdk/compare/cc5691c69ed2e71f99001a660c71c9b57c803e60...master

Your commit was automatically rebased without conflicts.

Pushed as commit e29c560.

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

Improvements for PointerFinder and findpc command.

2463255

openjdk bot added serviceability hotspot-gc labels Jan 17, 2021

plummercj marked this pull request as ready for review January 19, 2021 22:44

openjdk bot added the rfr label Jan 19, 2021

YaSuenag approved these changes Feb 2, 2021

View reviewed changes

openjdk bot added the ready label Feb 2, 2021

kevinjwalls reviewed Feb 10, 2021

View reviewed changes

kevinjwalls approved these changes Feb 10, 2021

View reviewed changes

openjdk bot added merge-conflict and removed ready labels Feb 10, 2021

Merge master

69a8ae5

openjdk bot added ready and removed merge-conflict labels Feb 11, 2021

Fix issue with parsing 'examine' output when there is unexecptected o…

79cb108

…utput due to CDS logging or -Xcheck:jni warnings.

kevinjwalls approved these changes Feb 12, 2021

View reviewed changes

YaSuenag approved these changes Feb 12, 2021

View reviewed changes

openjdk bot closed this Feb 12, 2021

openjdk bot added integrated and removed ready rfr labels Feb 12, 2021

plummercj deleted the JDK-8247514 branch March 23, 2021 18:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8247514: Improve clhsdb 'findpc' ability to determine what an address points to by improving PointerFinder and PointerLocation classes #2111

8247514: Improve clhsdb 'findpc' ability to determine what an address points to by improving PointerFinder and PointerLocation classes #2111

plummercj commented Jan 17, 2021 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jan 17, 2021

openjdk bot commented Jan 17, 2021

mlbridge bot commented Jan 19, 2021 •

edited

Loading

plummercj commented Jan 25, 2021

plummercj commented Feb 2, 2021

YaSuenag left a comment

openjdk bot commented Feb 2, 2021 •

edited

Loading

kevinjwalls Feb 10, 2021

plummercj Feb 10, 2021

kevinjwalls Feb 11, 2021

kevinjwalls left a comment

openjdk bot commented Feb 10, 2021

plummercj commented Feb 12, 2021

kevinjwalls left a comment

YaSuenag left a comment

plummercj commented Feb 12, 2021

openjdk bot commented Feb 12, 2021

8247514: Improve clhsdb 'findpc' ability to determine what an address points to by improving PointerFinder and PointerLocation classes #2111

8247514: Improve clhsdb 'findpc' ability to determine what an address points to by improving PointerFinder and PointerLocation classes #2111

Conversation

plummercj commented Jan 17, 2021 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Download

bridgekeeper bot commented Jan 17, 2021

openjdk bot commented Jan 17, 2021

mlbridge bot commented Jan 19, 2021 • edited Loading

Webrevs

plummercj commented Jan 25, 2021

plummercj commented Feb 2, 2021

YaSuenag left a comment

Choose a reason for hiding this comment

openjdk bot commented Feb 2, 2021 • edited Loading

kevinjwalls Feb 10, 2021

Choose a reason for hiding this comment

plummercj Feb 10, 2021

Choose a reason for hiding this comment

kevinjwalls Feb 11, 2021

Choose a reason for hiding this comment

kevinjwalls left a comment

Choose a reason for hiding this comment

openjdk bot commented Feb 10, 2021

plummercj commented Feb 12, 2021

kevinjwalls left a comment

Choose a reason for hiding this comment

YaSuenag left a comment

Choose a reason for hiding this comment

plummercj commented Feb 12, 2021

openjdk bot commented Feb 12, 2021

plummercj commented Jan 17, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Jan 19, 2021 •

edited

Loading

openjdk bot commented Feb 2, 2021 •

edited

Loading