OpenJDK › OpenJDK Serviceability Development

RFR: 8261098: Add clhsdb "findsym" command

Classic

List

Threaded

11 messages Options

Chris Plummer-2

RFR: 8261098: Add clhsdb "findsym" command

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

Chris Plummer-2

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

> 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

Alex Menkov-3

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

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

Chris Plummer-2

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

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

Alex Menkov-3

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

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

Chris Plummer-2

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

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

Chris Plummer-2