8261354: SIGSEGV at MethodIteratorHost #4143
Conversation
|
👋 Welcome back jbachorik! A progress list of the required criteria for merging this PR into |
|
@jbachorik 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. |
jbachorik
changed the title
Webrevs
|
There was a problem hiding this comment.
Wow, this was subtle - great work in locating the root cause Jaroslav!
Were you able to reproduce this issue?
I wonder how this affects the Filter construct that is intended to prevent multiple entries from being serialized (and to leave when done). Now it looks like there is no state carried between iterations, unfortunately. This might be ok in practice, it would probably be ok to have multiple entries of the same method serialized (although redundant). Might need to reconsider if the Filter should still be used here.
Looks good, thank you
Markus
|
@jbachorik 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 11 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 |
|
/reviewer remove mgronlun |
|
@mgronlun Only the author (@jbachorik) is allowed to issue the |
Well, yes. But I did it with BTrace script - the reproducer requires an instrumenting agent which is adding |
@mgronlun Do you want to 'unreview' this PR? |
|
Hi again, I noticed the Filter stuff after I clicked Approve, and it was an attempt to roll back my approve state - I don't know if it is possible to even do that. So consider it approved, but we need to think about how to handle the Filter thing. Because now it will no longer work as intended, so maybe consider removing it. |
|
I can remove the filter before integrating to make the code less confusing - since the filter has no function right now it would be better if it is not there at all. |
|
/integrate |
openjdk
bot
added
integrated
|
@jbachorik Since your change was applied there have been 13 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit 2462316. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
The
MethodIteratorHostwill iterate over previous versions of redefined classes but the number of methods in that class is retrieved only for the current version.Since the previous versions may theoretically, and before JDK 15 also practically, contain fewer methods than the current version (hotspot did support adding
private staticmethods during retransformation) this can lead to SIGSEGV when the iterator tries accessing out-of-range method element.Although for after JDK 15 it is not possible to have differing number of methods between multiple versions of a retransformed class it feels right not to rely on the implementation detail and re-retrieve the number of methods per each class version.
All jdk_jfr tests are passing on Linux x64 and MacOS x64
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4143/head:pull/4143$ git checkout pull/4143Update a local copy of the PR:
$ git checkout pull/4143$ git pull https://git.openjdk.java.net/jdk pull/4143/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 4143View PR using the GUI difftool:
$ git pr show -t 4143Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4143.diff