Skip to content

8358725: RunThese30M: assert(nm->insts_contains_inclusive(original_pc)) failed: original PC must be in the main code section of the compiled method (or must be immediately following it) #27985

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

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

8358725: RunThese30M: assert(nm->insts_contains_inclusive(original_pc)) failed: original PC must be in the main code section of the compiled method (or must be immediately following it) #27985

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5e18428
stop stack walk at signal handler
dean-long Oct 23, 2025
8716124
missing include
dean-long Oct 23, 2025
d3e5ca4
sort includes
dean-long Oct 24, 2025
d5a0b53
remove added blank lines
dean-long Oct 24, 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
5 changes: 5 additions & 0 deletions src/hotspot/os/posix/signals_posix.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
#include "utilities/checkedCast.hpp"
#include "utilities/deferredStatic.hpp"
#include "utilities/events.hpp"
#include "utilities/nativeCallStack.hpp"
#include "utilities/ostream.hpp"
#include "utilities/parseInteger.hpp"
#include "utilities/vmError.hpp"
Expand Down Expand Up @@ -569,6 +570,10 @@ int JVM_HANDLE_XXX_SIGNAL(int sig, siginfo_t* info,
// (note: RAII ok here, even with JFR thread crash protection, see below).
ErrnoPreserver ep;

// We are called from a signal handler, so stop the stack backtrace here.
// See os::is_first_C_frame() in os::get_native_stack().
os::FirstNativeFrameMark fnfm;
Comment on lines +573 to +575
Copy link
Member

@dholmes-ora dholmes-ora Oct 29, 2025

Choose a reason for hiding this comment

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

Won't this break stack-walking in hs_err file generation when we get a SEGV for example?

Copy link
Member Author

@dean-long dean-long Oct 29, 2025

Choose a reason for hiding this comment

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

Yes, I suppose so. Good catch. We shouldn't consider the "first frame" marker if we are starting before it.

Copy link
Member

@dholmes-ora dholmes-ora Oct 30, 2025

Choose a reason for hiding this comment

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

The hs_err stack trace only seems to report from before the signal handler, though I'm unclear if that is because the signal context sets the initial frame, or because we skip over things till we get to that point.

Copy link
Contributor

@pchilano pchilano Oct 30, 2025

Choose a reason for hiding this comment

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

If we call report_and_die from the signal handler we use the signal context as the starting point to walk the stack. So it shouldn’t affect the hs_err stack trace unless we crashed in the signal handler itself somewhere before calling report_and_die (and that we don’t hit the second time). I guess we still want to support that case. The other case I was thinking was if we call report_and_die due to hitting an assert and there is no context set, but then we crash before printing the stack. So the second attempt to print the stack is done within the signal handler and VMError::_context is still nullptr (only set first time), so we start walking from the current frame. I tested this case with this patch applied and we walk the full stack including the signal handler fine. I was confused first but then realized we execute the secondary signal handler, which doesn’t have this mark.

Copy link
Member Author

@dean-long dean-long Nov 1, 2025

Choose a reason for hiding this comment

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

I can confirm David's concern, than if we get a SEGV, then the stack backtrace only contains a single frame. One solution would be to do something more like anchor frames, which are chained. Imagine wanting to start a stack walk in the middle of the stack between anchor frame A and anchor frame B, and stop at the anchor frame boundary. That is comparable to what error reporting is attempting to do by providing a saved context as a starting point.


// Unblock all synchronous error signals (see JDK-8252533)
PosixSignals::unblock_error_signals();

Expand Down
11 changes: 10 additions & 1 deletion src/hotspot/share/runtime/os.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1404,11 +1404,18 @@ static bool is_pointer_bad(intptr_t* ptr) {
// stack is walkable beyond current frame.
// Returns true if this is not the case, i.e. the frame is possibly
// the first C frame on the stack.
bool os::is_first_C_frame(frame* fr) {
bool os::is_first_C_frame(const frame* fr) {

#ifdef _WINDOWS
return true; // native stack isn't walkable on windows this way.
#endif

// Check for a user-specified first frame, which can be used to prevent
// walking through signal handlers, etc.
if (FirstNativeFrameMark::is_first_native_frame(*fr)) {
return true;
}

// Load up sp, fp, sender sp and sender fp, check for reasonable values.
// Check usp first, because if that's bad the other accessors may fault
// on some architectures. Ditto ufp second, etc.
Expand Down Expand Up @@ -2640,3 +2647,5 @@ char* os::build_agent_function_name(const char *sym_name, const char *lib_name,
}
return agent_entry_name;
}

THREAD_LOCAL address os::FirstNativeFrameMark::_first_frame_stack_pointer = nullptr;
21 changes: 20 additions & 1 deletion src/hotspot/share/runtime/os.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -884,7 +884,7 @@ class os: AllStatic {
// does not require a lookup in the unwind table, which is part of the binary
// file but may be unsafe to read after a fatal error. So on x86, we can
// only walk stack if %ebp is used as frame pointer.
static bool is_first_C_frame(frame *fr);
static bool is_first_C_frame(const frame *fr);
static frame get_sender_for_C_frame(frame *fr);

// return current frame. pc() and sp() are set to null on failure.
Expand Down Expand Up @@ -1100,6 +1100,25 @@ class os: AllStatic {
static bool set_boot_path(char fileSep, char pathSep);

static bool pd_dll_unload(void* libhandle, char* ebuf, int ebuflen);

public:
class FirstNativeFrameMark {
private:
static THREAD_LOCAL address _first_frame_stack_pointer;
address _saved_stack_pointer;

public:
FirstNativeFrameMark(address sp = os::current_stack_pointer()) {
_saved_stack_pointer = _first_frame_stack_pointer;
_first_frame_stack_pointer = sp;
}

~FirstNativeFrameMark() {
_first_frame_stack_pointer = _saved_stack_pointer;
}

static inline bool is_first_native_frame(const frame& fr);
};
};

// Note that "PAUSE" is almost always used with synchronization
Expand Down
7 changes: 7 additions & 0 deletions src/hotspot/share/runtime/os.inline.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@

#include "runtime/os.hpp"

#include "runtime/frame.hpp"

#include OS_HEADER_INLINE(os)
#include OS_CPU_HEADER_INLINE(os)

Expand Down Expand Up @@ -59,4 +61,9 @@ inline void* os::resolve_function_descriptor(void* p) {
}
#endif

inline bool os::FirstNativeFrameMark::is_first_native_frame(const frame& fr) {
address ff_sp = _first_frame_stack_pointer;
return ff_sp != nullptr && (address)fr.sp() >= ff_sp;
}

#endif // SHARE_RUNTIME_OS_INLINE_HPP
14 changes: 14 additions & 0 deletions test/hotspot/gtest/runtime/test_os.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1195,3 +1195,17 @@ TEST_VM(os, dll_load_null_error_buf) {
void* lib = os::dll_load("NoSuchLib", nullptr, 0);
ASSERT_NULL(lib);
}

#if !defined(_WINDOWS)
TEST_VM(os, FirstNativeFrameMark) {
{
NativeCallStack ncs(0);
EXPECT_TRUE(ncs.frames() >= 1) << "expected no less than 1 frame, but saw " << ncs.frames();
}
{
os::FirstNativeFrameMark fnfm;
NativeCallStack ncs(0);
EXPECT_TRUE(ncs.frames() <= 1) << "expected no more than 1 frame, but saw " << ncs.frames();
}
}
#endif