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

[Chris Plummer-2]

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](https://bugs.openjdk.java.net/browse/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](https://bugs.openjdk.java.net/browse/JDK-8244670)

-------------

Commit messages:
 - Improvements for PointerFinder and findpc command.

Changes: https://git.openjdk.java.net/jdk/pull/2111/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2111&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8247514
  Stats: 292 lines in 5 files changed: 237 ins; 8 del; 47 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2111.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2111/head:pull/2111

PR: https://git.openjdk.java.net/jdk/pull/2111

[Chris Plummer-2]

On Sun, 17 Jan 2021 03:57:59 GMT, Chris Plummer <[hidden email]> wrote:

> 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](https://bugs.openjdk.java.net/browse/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](https://bugs.openjdk.java.net/browse/JDK-8244670)

Ping!

-------------

PR: https://git.openjdk.java.net/jdk/pull/2111

[Chris Plummer-2]

On Mon, 25 Jan 2021 20:00:41 GMT, Chris Plummer <[hidden email]> wrote:

>> 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](https://bugs.openjdk.java.net/browse/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](https://bugs.openjdk.java.net/browse/JDK-8244670)
>
> Ping!

Ping again.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2111

[Yasumasa Suenaga-7]

On Sun, 17 Jan 2021 03:57:59 GMT, Chris Plummer <[hidden email]> wrote:

> 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](https://bugs.openjdk.java.net/browse/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](https://bugs.openjdk.java.net/browse/JDK-8244670)

LGTM

-------------

Marked as reviewed by ysuenaga (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2111

[Kevin Walls-2]

On Sun, 17 Jan 2021 03:57:59 GMT, Chris Plummer <[hidden email]> wrote:

> 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](https://bugs.openjdk.java.net/browse/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](https://bugs.openjdk.java.net/browse/JDK-8244670)

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

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

> 245:                        stackThread.getStackBase(), stackThread.lastSPDbg(),
> 246:                        stackThread.getStackBase().addOffsetTo(-stackThread.getStackSize()),
> 247:                        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()?

-------------

Marked as reviewed by kevinw (Committer).

PR: https://git.openjdk.java.net/jdk/pull/2111

[Chris Plummer-2]

On Wed, 10 Feb 2021 17:52:59 GMT, Kevin Walls <[hidden email]> wrote:

>> 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](https://bugs.openjdk.java.net/browse/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](https://bugs.openjdk.java.net/browse/JDK-8244670)
>
> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/PointerLocation.java line 247:
>
>> 245:                        stackThread.getStackBase(), stackThread.lastSPDbg(),
>> 246:                        stackThread.getStackBase().addOffsetTo(-stackThread.getStackSize()),
>> 247:                        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.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2111

[Chris Plummer-2]

> 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](https://bugs.openjdk.java.net/browse/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](https://bugs.openjdk.java.net/browse/JDK-8244670)

Chris Plummer has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains two commits:

 - Merge master
 - Improvements for PointerFinder and findpc command.

-------------

Changes: https://git.openjdk.java.net/jdk/pull/2111/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2111&range=01
  Stats: 291 lines in 5 files changed: 237 ins; 8 del; 46 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2111.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2111/head:pull/2111

PR: https://git.openjdk.java.net/jdk/pull/2111

[Kevin Walls-2]

On Wed, 10 Feb 2021 21:12:19 GMT, Chris Plummer <[hidden email]> wrote:

>> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/PointerLocation.java line 247:
>>
>>> 245:                        stackThread.getStackBase(), stackThread.lastSPDbg(),
>>> 246:                        stackThread.getStackBase().addOffsetTo(-stackThread.getStackSize()),
>>> 247:                        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.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2111

[Chris Plummer-2]

> 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](https://bugs.openjdk.java.net/browse/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](https://bugs.openjdk.java.net/browse/JDK-8244670)

Chris Plummer has updated the pull request incrementally with one additional commit since the last revision:

  Fix issue with parsing 'examine' output when there is unexecptected output due to CDS logging or -Xcheck:jni warnings.

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2111/files
  - new: https://git.openjdk.java.net/jdk/pull/2111/files/69a8ae59..79cb1080

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2111&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2111&range=01-02

  Stats: 5 lines in 1 file changed: 2 ins; 1 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2111.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2111/head:pull/2111

PR: https://git.openjdk.java.net/jdk/pull/2111

[Chris Plummer-2]

On Tue, 2 Feb 2021 23:21:50 GMT, Yasumasa Suenaga <[hidden email]> wrote:

>> Chris Plummer has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Fix issue with parsing 'examine' output when there is unexecptected output due to CDS logging or -Xcheck:jni warnings.
>
> LGTM

@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](https://bugs.openjdk.java.net/browse/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.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2111

[Kevin Walls-2]

On Thu, 11 Feb 2021 23:52:58 GMT, Chris Plummer <[hidden email]> wrote:

>> 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](https://bugs.openjdk.java.net/browse/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](https://bugs.openjdk.java.net/browse/JDK-8244670)
>
> Chris Plummer has updated the pull request incrementally with one additional commit since the last revision:
>
>   Fix issue with parsing 'examine' output when there is unexecptected output due to CDS logging or -Xcheck:jni warnings.

Yes, joy of text processing.  Looks good.

-------------

Marked as reviewed by kevinw (Committer).

PR: https://git.openjdk.java.net/jdk/pull/2111

[Yasumasa Suenaga-7]

On Thu, 11 Feb 2021 23:52:58 GMT, Chris Plummer <[hidden email]> wrote:

>> 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](https://bugs.openjdk.java.net/browse/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](https://bugs.openjdk.java.net/browse/JDK-8244670)
>
> Chris Plummer has updated the pull request incrementally with one additional commit since the last revision:
>
>   Fix issue with parsing 'examine' output when there is unexecptected output due to CDS logging or -Xcheck:jni warnings.

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

-------------

Marked as reviewed by ysuenaga (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2111

[Chris Plummer-2]

On Sun, 17 Jan 2021 03:57:59 GMT, Chris Plummer <[hidden email]> wrote:

> 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](https://bugs.openjdk.java.net/browse/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](https://bugs.openjdk.java.net/browse/JDK-8244670)

This pull request has now been integrated.

Changeset: e29c560a
Author:    Chris Plummer <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/e29c560a
Stats:     292 lines in 5 files changed: 238 ins; 8 del; 46 mod

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

Reviewed-by: ysuenaga, kevinw

-------------

PR: https://git.openjdk.java.net/jdk/pull/2111