8315380: AsyncGetCallTrace crash in frame::safe_for_sender

[jbachorik]

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

• [x] Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• [x] Change must not contain extraneous whitespace
• [x] 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

• Paul Hohensee (@phohensee - Reviewer)

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[bot]]

:wave: 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[bot]]

@jbachorik This change now passes all automated pre-integration checks.

After integration, the commit message for the final commit will be:

8315380: AsyncGetCallTrace crash in frame::safe_for_sender

Reviewed-by: phh, shade, apangin

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 31 new commits pushed to the master branch:

• 276a157622f76d7210e11423879d81ccd70c3568: 8296631: NSS tests failing on OL9 linux-aarch64 hosts
• 524170d5a03c9df2ca463ed416e786a4b412ed6c: Merge
• 856bab316a63ce3acddd08d3fbe76b9c3223cdb6: 8354087: [11u] Remove designator DEFAULT_PROMOTED_VERSION_PRE=ea for release 11.0.27
• ... and 28 more: https://git.openjdk.org/jdk11u-dev/compare/659a4669208645420e151e78ab5fd3ac3808b310...master

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 master branch, type /integrate in a new comment.

[mlbridge[bot]]

Webrevs

• 03: Full - Incremental (56e19864)
• 02: Full - Incremental (d8e1257f)
• 01: Full - Incremental (4d040261)
• 00: Full (3a89bf64)

[jbachorik]

/label add hotspot,serviceability

[openjdk[bot]]

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

[jbachorik]

/label hotspot-runtime

[openjdk[bot]]

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

[jbachorik]

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.

[openjdk[bot]]

⚠️ @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]

/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[bot]]

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

[jbachorik]

/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[bot]]

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

[bridgekeeper[bot]]

@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]

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

[jerboaa]

@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]

/help

[openjdk[bot]]

@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]

/reviewers @simonis @shipilev

[openjdk[bot]]

@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]

/reviewer @dholmes-ora @shipilev

[openjdk[bot]]

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

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

[jbachorik]

/reviewers 2 reviewer

[openjdk[bot]]

@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]

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.

[jbachorik]

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

[jbachorik]

Thanks everyone for helping me to review this!

@jerboaa I think we are ok now?

[jerboaa]

Please integrate this.

[jbachorik]

/integrate

[openjdk[bot]]

Going to push as commit d46f769e70f6c0e8effcb78310cacc391a14fd6f. Since your change was applied there have been 38 commits pushed to the master branch:

• 8894d6ad1b83553a5c60af51fd2de2319fd1d6ec: 8350498: Remove two Camerfirma root CA certificates
• c4f2b0c0d9189c6193892f509a06fc3ee62c9ea5: 8339300: CollectorPolicy.young_scaled_initial_ergo_vm gtest fails on ppc64 based platforms
• 9d55a69f64756d1df8e3c6ca9c23f210078ccdd6: 8258483: [TESTBUG] gtest CollectorPolicy.young_scaled_initial_ergo_vm fails if heap is too small
• ... and 35 more: https://git.openjdk.org/jdk11u-dev/compare/659a4669208645420e151e78ab5fd3ac3808b310...master

Your commit was automatically rebased without conflicts.