Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8263434: Dangling references after MethodComparator::methods_EMCP #2937

Conversation

shipilev
Copy link
Member

@shipilev shipilev commented Mar 11, 2021

SonarCloud reports the following problem in MethodComparator::methods_EMCP:
"Address of stack memory associated with local variable 's_new' is still referred to by the global variable '_s_new' upon returning to the caller. This will be a dangling reference"

Code inspection reveals the assignment to static variables is only needed to pass them to helper methods. So, while this is not a detectable bug (yet), it is still cleaner not to expose stack variables in globals.

Additional testing:

  • Linux x86_64 fastdebug tier1
  • Linux x86_64 fastdebug, vmTestbase_nsk_jvmti

Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8263434: Dangling references after MethodComparator::methods_EMCP

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2937/head:pull/2937
$ git checkout pull/2937

@bridgekeeper
Copy link

bridgekeeper bot commented Mar 11, 2021

👋 Welcome back shade! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

openjdk bot commented Mar 11, 2021

@shipilev The following label will be automatically applied to this pull request:

  • hotspot

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.

@openjdk openjdk bot added the hotspot hotspot-dev@openjdk.org label Mar 11, 2021
@shipilev shipilev marked this pull request as ready for review March 11, 2021 11:08
@openjdk openjdk bot added the rfr Pull request is ready for review label Mar 11, 2021
@mlbridge
Copy link

mlbridge bot commented Mar 11, 2021

Webrevs

Copy link
Contributor

@coleenp coleenp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is much cleaner! Thank you, and thank you SonarCloud.

@openjdk
Copy link

openjdk bot commented Mar 11, 2021

@shipilev 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:

8263434: Dangling references after MethodComparator::methods_EMCP

Reviewed-by: coleenp, sspitsyn

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 no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@coleenp
Copy link
Contributor

coleenp commented Mar 11, 2021

/label add serviceability-dev

@openjdk openjdk bot added ready Pull request is ready to be integrated serviceability serviceability-dev@openjdk.org labels Mar 11, 2021
@openjdk
Copy link

openjdk bot commented Mar 11, 2021

@coleenp
The serviceability label was successfully added.

@shipilev
Copy link
Member Author

/reviewers 2

@openjdk
Copy link

openjdk bot commented Mar 11, 2021

@shipilev
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

@openjdk openjdk bot removed the ready Pull request is ready to be integrated label Mar 11, 2021
@dcubed-ojdk
Copy link
Member

Please make sure that you get a review from someone on the Serviceability team.

Copy link
Contributor

@sspitsyn sspitsyn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aleksey,
Sorry for being late to the party.
This looks good to me.
One nit: unneeded extra '()' what came from the original code:
if ((old_cp->klass_at_noresolve(cpi_old) != new_cp->klass_at_noresolve(cpi_new)))

Thanks,
Serguei

@shipilev
Copy link
Member Author

One nit: unneeded extra '()' what came from the original code:
if ((old_cp->klass_at_noresolve(cpi_old) != new_cp->klass_at_noresolve(cpi_new)))

Sure, updated. Also updated copyrights.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Mar 17, 2021
@shipilev
Copy link
Member Author

/integrate

@openjdk openjdk bot closed this Mar 17, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Mar 17, 2021
@openjdk
Copy link

openjdk bot commented Mar 17, 2021

@shipilev Since your change was applied there have been 5 commits pushed to the master branch:

  • 23fc2a4: 8263688: Coordinate equals, hashCode and compareTo of JavacFileManager.PathAndContainer
  • d1baed6: 8263672: fatal error: no reachable node should have no use
  • 086a66a: 8261095: Add test for clhsdb "symbol" command
  • ec95a5c: 8263410: ListModel javadoc refers to non-existent interface
  • 7b9d256: 8261262: Kitchensink24HStress.java crashed with EXCEPTION_ACCESS_VIOLATION

Your commit was automatically rebased without conflicts.

Pushed as commit f9f2eef.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@shipilev shipilev deleted the JDK-8263434-dangling-methodcomparator-ecmp branch March 25, 2021 12:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot hotspot-dev@openjdk.org integrated Pull request has been integrated serviceability serviceability-dev@openjdk.org
Development

Successfully merging this pull request may close these issues.

4 participants