Skip to content

8315380: AsyncGetCallTrace crash in frame::safe_for_sender #3003

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
+2 −1
Open

8315380: AsyncGetCallTrace crash in frame::safe_for_sender #3003

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3a89bf6
8315380: AsyncGetCallTrace crash in frame::safe_for_sender
jbachorik Feb 19, 2025
4d04026
Make proper fix: 'sp' -> 'unextended_sp' in the condition
jbachorik Mar 10, 2025
d8e1257
Use 'thread->stack_end()'
jbachorik Apr 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/hotspot/cpu/aarch64/frame_aarch64.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
/*
* Copyright (c) 1997, 2021, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2014, 2021, Red Hat Inc. All rights reserved.
* Copyrithg (c) 2025, Datadog, Inc. All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Copyrithg (c) 2025, Datadog, Inc. All rights reserved.
* Copyright (c) 2025, Datadog, Inc. All rights reserved.

I tend to agree that this doesn't really need a copyright update.

* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -84,7 +85,8 @@ bool frame::safe_for_sender(JavaThread *thread) {
// So unextended sp must be within the stack but we need not to check
// that unextended sp >= sp

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

Choose a reason for hiding this comment

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

  1. What's the meaning of trailing \ here?
  2. I think you can do unextended_sp >= thread->stack_end() to better capture the intent and match 8238988 better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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 :)

Copy link
Contributor

Choose a reason for hiding this comment

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

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


if (!unextended_sp_safe) {
return false;
Expand Down