RFR: 8359820: Improve handshake/safepoint timeout diagnostic messages [v5]
David Holmes
dholmes at openjdk.org
Sun Jul 20 21:55:49 UTC 2025
On Fri, 18 Jul 2025 15:10:35 GMT, Anton Artemov <duke at openjdk.org> wrote:
>> Hi, please consider the following changes:
>>
>> The problem in the issue description is not a problem by itself, the behavior is not unexpected, but it is somewhat difficult to find out what caused SIGILL to be fired.
>>
>> We propagate this information from `handshake::handle_timeout()` to `VMError::report()` with a help of a global variable. The same mechanism is used to address a similar issue in the safepoint timeout handler.
>>
>> Tested in tiers 1-3.
>
> Anton Artemov has updated the pull request incrementally with two additional commits since the last revision:
>
> - 8359820: Fixed spaces
> - 8359820: Addressed reviewer's comments
The structure of this looks good, but I have a few remaining nits. Thanks.
src/hotspot/share/runtime/handshake.cpp line 214:
> 212: }
> 213: if (target != nullptr) {
> 214: fatal("Thread " PTR_FORMAT " has not cleared handshake op: " PTR_FORMAT ", then failed to terminate JVM", p2i(target), p2i(op));
Suggestion:
fatal("Thread " PTR_FORMAT " has not cleared handshake op %s, and failed to terminate the JVM", p2i(target), op->name());
The earlier logging statement that uses `p2i(op)` relies on an even earlier logging statement (line 189/190) that reports the name and the `p2i` value. But the fatal error message can't rely on using logging information to map the `p2i` value back to a name, so we need the name directly.
src/hotspot/share/utilities/vmError.cpp line 108:
> 106: const intptr_t VMError::segfault_address = pd_segfault_address;
> 107: volatile intptr_t VMError::handshakeTimedOutThread = p2i(nullptr);
> 108: volatile intptr_t VMError::safepointTimedOutThread = p2i(nullptr);
Suggestion:
volatile intptr_t VMError::_handshake_timeout_thread = p2i(nullptr);
volatile intptr_t VMError::_safepoint_timeout_thread = p2i(nullptr);
src/hotspot/share/utilities/vmError.cpp line 1340:
> 1338: }
> 1339:
> 1340: void VMError::set_handshake_timed_out_thread(intptr_t x) {
Suggestion:
void VMError::set_handshake_timed_out_thread(intptr_t thread_addr) {
src/hotspot/share/utilities/vmError.cpp line 1344:
> 1342: }
> 1343:
> 1344: void VMError::set_safepoint_timed_out_thread(intptr_t x) {
Suggestion:
void VMError::set_safepoint_timed_out_thread(intptr_t thread_addr) {
-------------
Changes requested by dholmes (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/26309#pullrequestreview-3036188379
PR Review Comment: https://git.openjdk.org/jdk/pull/26309#discussion_r2217970953
PR Review Comment: https://git.openjdk.org/jdk/pull/26309#discussion_r2217971507
PR Review Comment: https://git.openjdk.org/jdk/pull/26309#discussion_r2217972623
PR Review Comment: https://git.openjdk.org/jdk/pull/26309#discussion_r2217972740
More information about the hotspot-runtime-dev
mailing list