RFR: 8261098: Add clhsdb "findsym" command

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: 8261098: Add clhsdb "findsym" command

Chris Plummer-2
Add "findsym" to clhsdb. See the CR and CSR for details. The CSR still needs a reviewer.

There is one other somewhat unrelated fix in the test:

 -            String value = parts[1];
 +           String value = parts[1].split(linesep)[0];

This is suppose to capture just the value at the specified address, but it also captures the newline and some text after. The result if that when `findpc <value>` is executed, it also executes another command or two of garbage commands after that, which produce errors. They were not impacting the test, but were noticeable in the log. I first noticed it when similar code in the new part of the test had the same issue.

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

Commit messages:
 - Add clhsdb findsym command.

Changes: https://git.openjdk.java.net/jdk/pull/2567/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2567&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8261098
  Stats: 69 lines in 3 files changed: 61 ins; 0 del; 8 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2567.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2567/head:pull/2567

PR: https://git.openjdk.java.net/jdk/pull/2567
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261098: Add clhsdb "findsym" command [v2]

Chris Plummer-2
> Add "findsym" to clhsdb. See the CR and CSR for details. The [CSR](https://bugs.openjdk.java.net/browse/JDK-8261101) still needs a reviewer.
>
> There is a fix in LinuxDebuggerLocal_lookupByName0() to allow passing in NULL for the dso name. This is allowed, and in fact even not null it gets ignored. It is needed by the new findsym support in order for the following to work, which passes in null for the dso name:
>
> `    Address addr = VM.getVM().getDebugger().lookup(null, symbol);`
>
> There is one other somewhat unrelated fix in the test:
>
>  -            String value = parts[1];
>  +           String value = parts[1].split(linesep)[0];
>
> This is suppose to capture just the value at the specified address, but it also captures the newline and some text after. The result if that when `findpc <value>` is executed, it also executes another command or two of garbage commands after that, which produce errors. They were not impacting the test, but were noticeable in the log. I first noticed it when similar code in the new part of the test had the same issue.

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

  Fix issue with findsym on windows build that doesn't have symbolic information.

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2567/files
  - new: https://git.openjdk.java.net/jdk/pull/2567/files/8c6d9727..ba3464c4

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

  Stats: 29 lines in 1 file changed: 15 ins; 1 del; 13 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2567.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2567/head:pull/2567

PR: https://git.openjdk.java.net/jdk/pull/2567
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261098: Add clhsdb "findsym" command [v2]

Alex Menkov-3
On Mon, 15 Feb 2021 20:19:51 GMT, Chris Plummer <[hidden email]> wrote:

>> Add "findsym" to clhsdb. See the CR and CSR for details. The [CSR](https://bugs.openjdk.java.net/browse/JDK-8261101) still needs a reviewer.
>>
>> There is a fix in LinuxDebuggerLocal_lookupByName0() to allow passing in NULL for the dso name. This is allowed, and in fact even not null it gets ignored. It is needed by the new findsym support in order for the following to work, which passes in null for the dso name:
>>
>> `    Address addr = VM.getVM().getDebugger().lookup(null, symbol);`
>>
>> There is one other somewhat unrelated fix in the test:
>>
>>  -            String value = parts[1];
>>  +           String value = parts[1].split(linesep)[0];
>>
>> This is suppose to capture just the value at the specified address, but it also captures the newline and some text after. The result if that when `findpc <value>` is executed, it also executes another command or two of garbage commands after that, which produce errors. They were not impacting the test, but were noticeable in the log. I first noticed it when similar code in the new part of the test had the same issue.
>
> Chris Plummer has updated the pull request incrementally with one additional commit since the last revision:
>
>   Fix issue with findsym on windows build that doesn't have symbolic information.

Changes requested by amenkov (Reviewer).

src/jdk.hotspot.agent/linux/native/libsaproc/LinuxDebuggerLocal.cpp line 350:

> 348:   // Note, objectName is ignored, and may in fact be NULL.
> 349:   // lookup_symbol will always search all objects/libs
> 350:   //AutoJavaString objectName_cstr(env, objectName);

I think it would be better to update AutoJavaString class to handle null strings:
`m_buf(str == NULL ? NULL : env->GetStringUTFChars(str, NULL)) `

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

PR: https://git.openjdk.java.net/jdk/pull/2567
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261098: Add clhsdb "findsym" command [v2]

Chris Plummer-2
On Mon, 15 Feb 2021 23:15:46 GMT, Alex Menkov <[hidden email]> wrote:

>> Chris Plummer has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Fix issue with findsym on windows build that doesn't have symbolic information.
>
> src/jdk.hotspot.agent/linux/native/libsaproc/LinuxDebuggerLocal.cpp line 350:
>
>> 348:   // Note, objectName is ignored, and may in fact be NULL.
>> 349:   // lookup_symbol will always search all objects/libs
>> 350:   //AutoJavaString objectName_cstr(env, objectName);
>
> I think it would be better to update AutoJavaString class to handle null strings:
> `m_buf(str == NULL ? NULL : env->GetStringUTFChars(str, NULL)) `

The point is the argument is ignored, so it doesn't really matter if AutoJavaString can handle NULL. I  assume with your suggested fix that also want me to pass objectName_cstr to lookup_symbol() rather than an explicit NULL. I wanted the NULL to be explicit to help further convey that objectName is ignored.

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

PR: https://git.openjdk.java.net/jdk/pull/2567
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261098: Add clhsdb "findsym" command [v2]

Alex Menkov-3
On Tue, 16 Feb 2021 01:26:46 GMT, Chris Plummer <[hidden email]> wrote:

>> src/jdk.hotspot.agent/linux/native/libsaproc/LinuxDebuggerLocal.cpp line 350:
>>
>>> 348:   // Note, objectName is ignored, and may in fact be NULL.
>>> 349:   // lookup_symbol will always search all objects/libs
>>> 350:   //AutoJavaString objectName_cstr(env, objectName);
>>
>> I think it would be better to update AutoJavaString class to handle null strings:
>> `m_buf(str == NULL ? NULL : env->GetStringUTFChars(str, NULL)) `
>
> The point is the argument is ignored, so it doesn't really matter if AutoJavaString can handle NULL. I  assume with your suggested fix that also want me to pass objectName_cstr to lookup_symbol() rather than an explicit NULL. I wanted the NULL to be explicit to help further convey that objectName is ignored.

lookup_symbol implementation contains FIXME comment about object_name.
I suppose you want to make this argument ignorance permanent. Then I think this FIXME should be removed (or updated) and it would be nice to add comment about this to lookup_symbol declaration (or maybe it would be better to delete this argument)

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

PR: https://git.openjdk.java.net/jdk/pull/2567
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261098: Add clhsdb "findsym" command [v2]

Chris Plummer-2
On Tue, 16 Feb 2021 22:07:04 GMT, Alex Menkov <[hidden email]> wrote:

>> The point is the argument is ignored, so it doesn't really matter if AutoJavaString can handle NULL. I  assume with your suggested fix that also want me to pass objectName_cstr to lookup_symbol() rather than an explicit NULL. I wanted the NULL to be explicit to help further convey that objectName is ignored.
>
> lookup_symbol implementation contains FIXME comment about object_name.
> I suppose you want to make this argument ignorance permanent. Then I think this FIXME should be removed (or updated) and it would be nice to add comment about this to lookup_symbol declaration (or maybe it would be better to delete this argument)

If the FIXME in `lookup_symbol` was ever addressed, then we would need for NULL to mean to search all libraries. I suppose in that case it would make sense for AutoJavaString to do as you suggested. It looks like Windows also has an AutoJavaString. I'll change it also, but it is never passed NULL. The Bsd version is, but it handles the GetStringUTFChars() inline without AutoJavaString, and it does support NULL already:

  objectName_cstr = NULL;
  if (objectName != NULL) {
    objectName_cstr = (*env)->GetStringUTFChars(env, objectName, &isCopy);
    CHECK_EXCEPTION_(0);
  }
...
  addr = (jlong) lookup_symbol(ph, objectName_cstr, symbolName_cstr);

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

PR: https://git.openjdk.java.net/jdk/pull/2567
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261098: Add clhsdb "findsym" command [v3]

Chris Plummer-2
In reply to this post by Chris Plummer-2
> Add "findsym" to clhsdb. See the CR and CSR for details. The [CSR](https://bugs.openjdk.java.net/browse/JDK-8261101) still needs a reviewer.
>
> There is a fix in LinuxDebuggerLocal_lookupByName0() to allow passing in NULL for the dso name. This is allowed, and in fact even not null it gets ignored. It is needed by the new findsym support in order for the following to work, which passes in null for the dso name:
>
> `    Address addr = VM.getVM().getDebugger().lookup(null, symbol);`
>
> There is one other somewhat unrelated fix in the test:
>
>  -            String value = parts[1];
>  +           String value = parts[1].split(linesep)[0];
>
> This is suppose to capture just the value at the specified address, but it also captures the newline and some text after. The result if that when `findpc <value>` is executed, it also executes another command or two of garbage commands after that, which produce errors. They were not impacting the test, but were noticeable in the log. I first noticed it when similar code in the new part of the test had the same issue.

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

  Improve handling of NULL objectName

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2567/files
  - new: https://git.openjdk.java.net/jdk/pull/2567/files/ba3464c4..1a772c3d

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

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

PR: https://git.openjdk.java.net/jdk/pull/2567
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261098: Add clhsdb "findsym" command [v2]

Chris Plummer-2
In reply to this post by Chris Plummer-2
On Tue, 16 Feb 2021 22:24:19 GMT, Chris Plummer <[hidden email]> wrote:

>> lookup_symbol implementation contains FIXME comment about object_name.
>> I suppose you want to make this argument ignorance permanent. Then I think this FIXME should be removed (or updated) and it would be nice to add comment about this to lookup_symbol declaration (or maybe it would be better to delete this argument)
>
> If the FIXME in `lookup_symbol` was ever addressed, then we would need for NULL to mean to search all libraries. I suppose in that case it would make sense for AutoJavaString to do as you suggested. It looks like Windows also has an AutoJavaString. I'll change it also, but it is never passed NULL. The Bsd version is, but it handles the GetStringUTFChars() inline without AutoJavaString, and it does support NULL already:
>
>   objectName_cstr = NULL;
>   if (objectName != NULL) {
>     objectName_cstr = (*env)->GetStringUTFChars(env, objectName, &isCopy);
>     CHECK_EXCEPTION_(0);
>   }
> ...
>   addr = (jlong) lookup_symbol(ph, objectName_cstr, symbolName_cstr);

I've pushed the AutoJavaString change.

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

PR: https://git.openjdk.java.net/jdk/pull/2567
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261098: Add clhsdb "findsym" command [v3]

Alex Menkov-3
In reply to this post by Chris Plummer-2
On Wed, 17 Feb 2021 02:12:56 GMT, Chris Plummer <[hidden email]> wrote:

>> Add "findsym" to clhsdb. See the CR and CSR for details. The [CSR](https://bugs.openjdk.java.net/browse/JDK-8261101) still needs a reviewer.
>>
>> There is a fix in LinuxDebuggerLocal_lookupByName0() to allow passing in NULL for the dso name. This is allowed, and in fact even not null it gets ignored. It is needed by the new findsym support in order for the following to work, which passes in null for the dso name:
>>
>> `    Address addr = VM.getVM().getDebugger().lookup(null, symbol);`
>>
>> There is one other somewhat unrelated fix in the test:
>>
>>  -            String value = parts[1];
>>  +           String value = parts[1].split(linesep)[0];
>>
>> This is suppose to capture just the value at the specified address, but it also captures the newline and some text after. The result if that when `findpc <value>` is executed, it also executes another command or two of garbage commands after that, which produce errors. They were not impacting the test, but were noticeable in the log. I first noticed it when similar code in the new part of the test had the same issue.
>
> Chris Plummer has updated the pull request incrementally with one additional commit since the last revision:
>
>   Improve handling of NULL objectName

Marked as reviewed by amenkov (Reviewer).

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

PR: https://git.openjdk.java.net/jdk/pull/2567
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261098: Add clhsdb "findsym" command [v3]

Serguei Spitsyn-2
In reply to this post by Chris Plummer-2
On Wed, 17 Feb 2021 02:12:56 GMT, Chris Plummer <[hidden email]> wrote:

>> Add "findsym" to clhsdb. See the CR and CSR for details. The [CSR](https://bugs.openjdk.java.net/browse/JDK-8261101) still needs a reviewer.
>>
>> There is a fix in LinuxDebuggerLocal_lookupByName0() to allow passing in NULL for the dso name. This is allowed, and in fact even not null it gets ignored. It is needed by the new findsym support in order for the following to work, which passes in null for the dso name:
>>
>> `    Address addr = VM.getVM().getDebugger().lookup(null, symbol);`
>>
>> There is one other somewhat unrelated fix in the test:
>>
>>  -            String value = parts[1];
>>  +           String value = parts[1].split(linesep)[0];
>>
>> This is suppose to capture just the value at the specified address, but it also captures the newline and some text after. The result if that when `findpc <value>` is executed, it also executes another command or two of garbage commands after that, which produce errors. They were not impacting the test, but were noticeable in the log. I first noticed it when similar code in the new part of the test had the same issue.
>
> Chris Plummer has updated the pull request incrementally with one additional commit since the last revision:
>
>   Improve handling of NULL objectName

It looks good to me.

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

Marked as reviewed by sspitsyn (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2567
Reply | Threaded
Open this post in threaded view
|

Integrated: 8261098: Add clhsdb "findsym" command

Chris Plummer-2
In reply to this post by Chris Plummer-2
On Mon, 15 Feb 2021 05:51:07 GMT, Chris Plummer <[hidden email]> wrote:

> Add "findsym" to clhsdb. See the CR and CSR for details. The [CSR](https://bugs.openjdk.java.net/browse/JDK-8261101) still needs a reviewer.
>
> There is a fix in LinuxDebuggerLocal_lookupByName0() to allow passing in NULL for the dso name. This is allowed, and in fact even not null it gets ignored. It is needed by the new findsym support in order for the following to work, which passes in null for the dso name:
>
> `    Address addr = VM.getVM().getDebugger().lookup(null, symbol);`
>
> There is one other somewhat unrelated fix in the test:
>
>  -            String value = parts[1];
>  +           String value = parts[1].split(linesep)[0];
>
> This is suppose to capture just the value at the specified address, but it also captures the newline and some text after. The result if that when `findpc <value>` is executed, it also executes another command or two of garbage commands after that, which produce errors. They were not impacting the test, but were noticeable in the log. I first noticed it when similar code in the new part of the test had the same issue.

This pull request has now been integrated.

Changeset: c158413e
Author:    Chris Plummer <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/c158413e
Stats:     83 lines in 4 files changed: 75 ins; 0 del; 8 mod

8261098: Add clhsdb "findsym" command

Reviewed-by: amenkov, sspitsyn

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

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