8315380: AsyncGetCallTrace crash in frame::safe_for_sender #3003

jbachorik · 2025-02-26T11:11:12Z

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 not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 2 Reviewers)
JDK-8315380 needs maintainer approval

Issue

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

Reviewers

Paul Hohensee (@phohensee - Reviewer)
Aleksey Shipilev (@shipilev - Reviewer)
Andrei Pangin (@apangin - no project role) Review applies to d8e1257f

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 · 2025-02-26T11:11:52Z

👋 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 · 2025-02-26T11:12:07Z

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

mlbridge · 2025-02-26T11:18:44Z

Webrevs

jbachorik · 2025-03-07T11:53:50Z

/label add hotspot,serviceability

openjdk · 2025-03-07T11:54:04Z

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

jbachorik · 2025-03-07T14:06:19Z

/label hotspot-runtime

openjdk · 2025-03-07T14:06:28Z

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

phohensee

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

jbachorik · 2025-03-10T11:50:44Z

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

Looks good now.

openjdk · 2025-03-10T16:59:08Z

⚠️ @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 · 2025-03-10T17:06:05Z

/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 · 2025-03-10T17:06:51Z

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

jbachorik · 2025-03-10T17:21:35Z

/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 · 2025-03-10T17:22:48Z

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

bridgekeeper · 2025-04-07T17:50:17Z

@jbachorik This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

jbachorik · 2025-04-16T14:36:55Z

@jerboaa @gnu-andrew I have created the approval request and tagged the JBS ticket. Would it be possible to review the request?

jerboaa · 2025-04-16T15:39:19Z

@jbachorik This is in a delicate area of the JVM and is a JDK 11u specific fix. Please get a second review for this. Thanks! It would also be good to explain why this isn't an issue in later JDKs on the issue.

jbachorik · 2025-04-16T16:33:50Z

/help

openjdk · 2025-04-16T16:34:04Z

@jbachorik Available commands:

approval - request for maintainer's approval
approve - null
author - sets an overriding author to be used in the commit when the PR is integrated
backport - create a backport
cc - add or remove an additional classification label
clean - Mark the backport pull request as a clean backport
contributor - adds or removes additional contributors for a PR
covered - used when employer has signed the OCA
csr - require a compatibility and specification request (CSR) for this pull request
help - shows this text
integrate - performs integration of the changes in the PR
issue - edit the list of issues that this PR solves
jep - require a JDK Enhancement Proposal (JEP) for this pull request
label - add or remove an additional classification label
open - Set the pull request state to "open"
reviewer - manage additional reviewers for a PR
reviewers - set the number of additional required reviewers for this PR
signed - used after signing the OCA
solves - edit the list of issues that this PR solves
sponsor - performs integration of a PR that is authored by a non-committer
summary - updates the summary in the commit message
test - used to run tests

jbachorik · 2025-04-16T16:35:56Z

/reviewers @simonis @shipilev

openjdk · 2025-04-16T16:36:21Z

@jbachorik Usage: /reviewers <n> [<role>] where <n> is the number of required reviewers. If role is set, the reviewers need to have that project role. If omitted, role defaults to authors.

jbachorik · 2025-04-16T16:36:52Z

/reviewer @dholmes-ora @shipilev

openjdk · 2025-04-16T16:37:40Z

@jbachorik Syntax: /reviewer (credit|remove) [@user | openjdk-user]+. For example:

/reviewer credit @openjdk-bot
/reviewer credit duke
/reviewer credit @user1 @user2

jbachorik · 2025-04-16T16:42:06Z

/reviewers 2 reviewer

openjdk · 2025-04-16T16:42:30Z

@jbachorik
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 2 Reviewers).

jbachorik · 2025-04-16T16:44:24Z

Ok, I bumped up the number of required reviewers.
But I don't seem to be able to find any mechanism to actually request the second review anywhere.

So, if someone is reading this in the email thread and has a few moments to review this rather trivial change, please, have a look.

shipilev

I think @apangin would be good to look at it.

shipilev · 2025-04-16T18:32:52Z

src/hotspot/cpu/aarch64/frame_aarch64.cpp

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


What's the meaning of trailing \ here?

I think you can do unextended_sp >= thread->stack_end() to better capture the intent and match 8238988 better.

What's the meaning of trailing \ here?
No particular meaning, removed.

I think you can do unextended_sp >= thread->stack_end() to better capture the intent and match 8238988 better.
Yes. Done.

Also, I noticed that the check for unextended_sp is not done very consistently across archs. This is also changed in 8238988 but I opted for the simplicity and added the change only for the arch for which the original issue was reported. I hope it's ok but if a more extensive change is preferred I can apply similar logic to other archs as well.

I see this for other arches:

AArch64: Checks (stack_base, *)

ARM: Checks [stack_base, sp]

PPC: Checks (stack_base, *)

S390: Checks (stack_base, *)

SPARC: Checks [stack_base, sp]

x86: Checks (stack_base, sp]

So I think only AArch64, PPC and S390 are affected by this bug? Checking against sp on other arches looks more conservative than checking for stack_end(), so I think we are "fine" there. Given that profiling on PPC and S390 is likely not happening all that often -- I think async-profiler started supporting PPC profiling only not that long ago -- I don't think there is a need to fix those.

@TheRealMDoerr might disagree, though :)

No, I'm not interested in asprof with jdk11. One remark: I wouldn't allow Copyright additions if I was lead maintainer.

jbachorik · 2025-04-17T10:36:49Z

@jerboaa AFAICS, it is already mentioned in this comment, along with the JBS ticket that made it safe in JDK 15+

shipilev

Fine by me, thanks.

jbachorik · 2025-04-17T11:03:13Z

Thanks everyone for helping me to review this!

@jerboaa I think we are ok now?

src/hotspot/cpu/aarch64/frame_aarch64.cpp

shipilev

Still fine.

8315380: AsyncGetCallTrace crash in frame::safe_for_sender

3a89bf6

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

phohensee reviewed Mar 7, 2025

View reviewed changes

Make proper fix: 'sp' -> 'unextended_sp' in the condition

4d04026

phohensee approved these changes Mar 10, 2025

View reviewed changes

openjdk bot added the approval label Mar 10, 2025

shipilev reviewed Apr 16, 2025

View reviewed changes

Use 'thread->stack_end()'

d8e1257

apangin approved these changes Apr 17, 2025

View reviewed changes

shipilev approved these changes Apr 17, 2025

View reviewed changes

phohensee approved these changes Apr 17, 2025

View reviewed changes

jerboaa reviewed Apr 17, 2025

View reviewed changes

src/hotspot/cpu/aarch64/frame_aarch64.cpp Outdated Show resolved Hide resolved

Remove copyright line

56e1986

phohensee approved these changes Apr 18, 2025

View reviewed changes

shipilev approved these changes Apr 22, 2025

View reviewed changes

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

8315380: AsyncGetCallTrace crash in frame::safe_for_sender #3003

Are you sure you want to change the base?

8315380: AsyncGetCallTrace crash in frame::safe_for_sender #3003

Conversation

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

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Feb 26, 2025

openjdk bot commented Feb 26, 2025

mlbridge bot commented Feb 26, 2025 • edited Loading

Webrevs

jbachorik commented Mar 7, 2025

openjdk bot commented Mar 7, 2025

jbachorik commented Mar 7, 2025

openjdk bot commented Mar 7, 2025

phohensee left a comment

Choose a reason for hiding this comment

jbachorik commented Mar 10, 2025

phohensee left a comment

Choose a reason for hiding this comment

openjdk bot commented Mar 10, 2025

jbachorik commented Mar 10, 2025

openjdk bot commented Mar 10, 2025

jbachorik commented Mar 10, 2025

openjdk bot commented Mar 10, 2025

bridgekeeper bot commented Apr 7, 2025

jbachorik commented Apr 16, 2025

jerboaa commented Apr 16, 2025

jbachorik commented Apr 16, 2025

openjdk bot commented Apr 16, 2025

jbachorik commented Apr 16, 2025

openjdk bot commented Apr 16, 2025

jbachorik commented Apr 16, 2025

openjdk bot commented Apr 16, 2025

jbachorik commented Apr 16, 2025

openjdk bot commented Apr 16, 2025

jbachorik commented Apr 16, 2025

shipilev left a comment

Choose a reason for hiding this comment

shipilev Apr 16, 2025

Choose a reason for hiding this comment

jbachorik Apr 17, 2025

Choose a reason for hiding this comment

shipilev Apr 17, 2025

Choose a reason for hiding this comment

TheRealMDoerr Apr 17, 2025

Choose a reason for hiding this comment

jbachorik commented Apr 17, 2025

shipilev left a comment

Choose a reason for hiding this comment

jbachorik commented Apr 17, 2025

shipilev left a comment

Choose a reason for hiding this comment

jbachorik commented Feb 26, 2025 •

edited by openjdk bot

Loading

mlbridge bot commented Feb 26, 2025 •

edited

Loading