8261098: Add clhsdb "findsym" command#2567
Conversation
|
👋 Welcome back cjplummer! A progress list of the required criteria for merging this PR into |
|
/csr JDK-8261101 |
|
@plummercj The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
|
@plummercj usage: |
Webrevs
|
|
/csr needed |
|
@plummercj this pull request will not be integrated until the CSR request JDK-8261101 for issue JDK-8261098 has been approved. |
| CHECK_EXCEPTION_(0); | ||
| // Note, objectName is ignored, and may in fact be NULL. | ||
| // lookup_symbol will always search all objects/libs | ||
| //AutoJavaString objectName_cstr(env, objectName); |
There was a problem hiding this comment.
I think it would be better to update AutoJavaString class to handle null strings:
m_buf(str == NULL ? NULL : env->GetStringUTFChars(str, NULL))
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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);
There was a problem hiding this comment.
I've pushed the AutoJavaString change.
|
@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: 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 46 new commits pushed to the
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 |
|
/integrate |
openjdk
bot
added
integrated
|
@plummercj Since your change was applied there have been 59 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit c158413. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
3 participants
Add "findsym" to clhsdb. See the CR and CSR for details. The CSR 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:
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.Progress
Issue
Reviewers
Download
$ git fetch https://git.openjdk.java.net/jdk pull/2567/head:pull/2567$ git checkout pull/2567