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

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

Closed
wants to merge 3 commits into from

Conversation

plummercj
Copy link
Contributor

@plummercj plummercj commented Jan 17, 2021

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

Download

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

@bridgekeeper
Copy link

bridgekeeper bot commented Jan 17, 2021

👋 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
Copy link

openjdk bot commented Jan 17, 2021

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

@openjdk openjdk bot added serviceability hotspot-gc labels Jan 17, 2021
@plummercj plummercj marked this pull request as ready for review Jan 19, 2021
@openjdk openjdk bot added the rfr label Jan 19, 2021
@mlbridge
Copy link

mlbridge bot commented Jan 19, 2021

Webrevs

@plummercj
Copy link
Contributor Author

plummercj commented Jan 25, 2021

Ping!

@plummercj
Copy link
Contributor Author

plummercj commented Feb 2, 2021

Ping again.

Copy link
Member

@YaSuenag YaSuenag left a comment

LGTM

@openjdk
Copy link

openjdk bot commented Feb 2, 2021

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

@openjdk openjdk bot added the ready label Feb 2, 2021
tty.format("In java stack [%s,%s,%s] for thread %s:\n ",
stackThread.getStackBase(), stackThread.lastSPDbg(),
stackThread.getStackBase().addOffsetTo(-stackThread.getStackSize()),
stackThread);

Choose a reason for hiding this comment

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

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()?

Copy link
Contributor Author

@plummercj plummercj Feb 10, 2021

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

Great, thanks.

Copy link

@kevinjwalls kevinjwalls left a comment

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

@openjdk
Copy link

openjdk bot commented Feb 10, 2021

@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

@openjdk openjdk bot added merge-conflict and removed ready labels Feb 10, 2021
@openjdk openjdk bot added ready and removed merge-conflict labels Feb 11, 2021
…utput due to CDS logging or -Xcheck:jni warnings.
@plummercj
Copy link
Contributor Author

plummercj commented Feb 12, 2021

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

Copy link

@kevinjwalls kevinjwalls left a comment

Yes, joy of text processing. Looks good.

Copy link
Member

@YaSuenag YaSuenag left a comment

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
Copy link
Contributor Author

plummercj commented Feb 12, 2021

/integrate

@openjdk openjdk bot closed this Feb 12, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Feb 12, 2021
@openjdk
Copy link

openjdk bot commented Feb 12, 2021

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

@plummercj plummercj deleted the JDK-8247514 branch Mar 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-gc integrated serviceability
3 participants