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.

Sign up for GitHub

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

Jump to bottom

8315380: AsyncGetCallTrace crash in frame::safe_for_sender #3003

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

8315380: AsyncGetCallTrace crash in frame::safe_for_sender #3003

wants to merge 2 commits into from

Conversation

jbachorik
Copy link
Contributor

@jbachorik jbachorik commented Feb 26, 2025 •
edited by openjdk bot
Loading

This change is fixing the problem in frame_aarch64.cpp, function safe_for_sender, where we have this code

bool unextended_sp_safe = unextended_sp < thread->stack_base();

While this captures one possibility of not being safe, it omits the check for unextended_sp falling within the stack space.

The proposed change then is

bool unextended_sp_safe = (unextended_sp < thread->stack_base() && \
                             sp >= thread->stack_base() - thread->stack_size());

This is actually just making sure the behaviour is the same as in JDK 15+ (since JDK-8238988) where the unextended_sp is checked for being within the stack limits.

The change is not accompanied by a JTReg test because I was not able to craft one triggering the issue reliably.

Existing tests from tier1-tier4 were run on a linux-aarch64 system with no new failures observed.

Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • JDK-8315380 needs maintainer approval

Issue

  • JDK-8315380: AsyncGetCallTrace crash in frame::safe_for_sender (Bug - P4 - Requested)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk11u-dev.git pull/3003/head:pull/3003
$ git checkout pull/3003

Update a local copy of the PR:
$ git checkout pull/3003
$ git pull https://git.openjdk.org/jdk11u-dev.git pull/3003/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3003

View PR using the GUI difftool:
$ git pr show -t 3003

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk11u-dev/pull/3003.diff

Using Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Feb 26, 2025

👋 Welcome back jbachorik! 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 Feb 26, 2025

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

@openjdk openjdk bot added the rfr Pull request is ready for review label Feb 26, 2025
@mlbridge
Copy link

mlbridge bot commented Feb 26, 2025 •
edited
Loading

Webrevs

@jbachorik
Copy link
Contributor Author

/label add hotspot,serviceability

@openjdk
Copy link

openjdk bot commented Mar 7, 2025

@jbachorik
The label hotspot is not a valid label.
The label serviceability is not a valid label.
These labels are valid:

@jbachorik
Copy link
Contributor Author

/label hotspot-runtime

@openjdk
Copy link

openjdk bot commented Mar 7, 2025

@jbachorik
The label hotspot-runtime is not a valid label.
These labels are valid:

phohensee
phohensee reviewed Mar 7, 2025
View reviewed changes
Copy link
Member

@phohensee phohensee left a comment

Choose a reason for hiding this comment

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

Should not the new check be of unextended_sp rather than sp? That would match the check in JDK 17.

@jbachorik
Copy link
Contributor Author

Should not the new check be of unextended_sp rather than sp? That would match the check in JDK 17.

Indeed. I feel a bit stupid now. Fixed.

phohensee
phohensee approved these changes Mar 10, 2025
View reviewed changes
Copy link
Member

@phohensee phohensee left a comment

Choose a reason for hiding this comment

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

Looks good now.

@openjdk
Copy link

openjdk bot commented Mar 10, 2025

⚠️ @jbachorik This change is now ready for you to apply for maintainer approval. This can be done directly in each associated issue or by using the /approval command.

@jbachorik
Copy link
Contributor Author

/approval I would like to ask for approval to integrate this JDK 11 specific bug fix. The change is very limited and improves the profiling experience for users of this Java version.

@openjdk
Copy link

openjdk bot commented Mar 10, 2025

@jbachorik usage: /approval [<id>] (request|cancel) [<text>]

@jbachorik
Copy link
Contributor Author

/approval request I would like to ask for approval to integrate this JDK 11 specific bug fix. The change is very limited and improves the profiling experience for users of this Java version.

@openjdk
Copy link

openjdk bot commented Mar 10, 2025

@jbachorik
8315380: The approval request has been created successfully.

@openjdk openjdk bot added the approval label Mar 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phohensee phohensee phohensee approved these changes

Assignees
No one assigned
Labels
approval rfr Pull request is ready for review
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jbachorik @phohensee