[jdk17u-dev] RFR: 8321509: False positive in get_trampoline fast path causes crash
Boris Ulasevich
bulasevich at openjdk.org
Mon Apr 14 16:54:11 UTC 2025
This change is a backport of [JDK-8321509](https://bugs.openjdk.org/browse/JDK-8321509).
**backport issue №1**
This change adds two functions to trampoline_stub_Relocation class. On macos-aarch64 platform it triggers clang to report:
/Users/runner/work/jdk17u-dev/jdk17u-dev/src/hotspot/share/code/relocInfo.hpp:1212:8: error: 'pack_data_to' overrides a member function but is not marked 'override' [-Werror,-Winconsistent-missing-override]
void pack_data_to(CodeSection * dest);
^
... (rest of output omitted)
Indeed, trampoline_stub_Relocation::pack_data_to overrides the virtual Relocation::pack_data_to. To fix the issue, I added the override keyword to the declaration of both pack_data_to and unpack_data in the trampoline_stub_Relocation class ([fixing clang error: pack_data_to overrides a member function but is not marked](https://github.com/openjdk/jdk17u-dev/pull/3441/commits/fd823c77b23b8f59011a8e9dc70b4cd51350e663)). To keep changes to a minimum, I have not changed other places where the functions are overridden.
**backport issue №2**
A manual merge was also required due to minor discrepancies between the mainline and jdk17u repositories. The reasons for the manual intervention are as follows:
- Copyright Year
- The mainline patch updated the copyright from 2023 to 2024
- In jdk17u the year is still 2021
- NULL -> nullptr conversion
- mainline deals with nullptr after JDK-8301493: Replace NULL with nullptr in cpu/aarch64
- CodeBlob Lookup Changes
- on the mainline "JDK-8290025: Remove the Sweeper" changed CodeCache::find_blob_unsafe call to CodeCache::find_blob
- in jdk17u the old variant is used
See the minor mismatches below. Left: original diff on JDK mainline. Right: diff actually applied
diff --git a/src/hotspot/cpu/aarch64/nativeInst_aarch64.cpp b/src/ho diff --git a/src/hotspot/cpu/aarch64/nativeInst_aarch64.cpp b/src/ho
--- a/src/hotspot/cpu/aarch64/nativeInst_aarch64.cpp --- a/src/hotspot/cpu/aarch64/nativeInst_aarch64.cpp
+++ b/src/hotspot/cpu/aarch64/nativeInst_aarch64.cpp +++ b/src/hotspot/cpu/aarch64/nativeInst_aarch64.cpp
@@ -1,5 +1,5 @@ @@ -1,5 +1,5 @@
/* /*
- * Copyright (c) 1997, 2023, Oracle and/or its affiliates. All righ | - * Copyright (c) 1997, 2021, Oracle and/or its affiliates. All righ
+ * Copyright (c) 1997, 2024, Oracle and/or its affiliates. All righ + * Copyright (c) 1997, 2024, Oracle and/or its affiliates. All righ
* Copyright (c) 2014, 2020, Red Hat Inc. All rights reserved. * Copyright (c) 2014, 2020, Red Hat Inc. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
* *
@@ -158,13 +158,18 @@ void NativeGotJump::verify() const { @@ -158,13 +158,18 @@ void NativeGotJump::verify() const {
} }
address NativeCall::destination() const { address NativeCall::destination() const {
- address addr = (address)this; - address addr = (address)this;
- address destination = instruction_address() + displacement(); - address destination = instruction_address() + displacement();
+ address addr = instruction_address(); + address addr = instruction_address();
+ address destination = addr + displacement(); + address destination = addr + displacement();
+ +
+ // Performance optimization: no need to call find_blob() if it is + // Performance optimization: no need to call find_blob() if it is
+ if (destination == addr) { + if (destination == addr) {
+ return destination; + return destination;
+ } + }
// Do we use a trampoline stub for this call? // Do we use a trampoline stub for this call?
CodeBlob* cb = CodeCache::find_blob(addr); | CodeBlob* cb = CodeCache::find_blob_unsafe(addr); // Else we get assertion if nmethod is zombie.
- assert(cb && cb->is_nmethod(), "sanity"); - assert(cb && cb->is_nmethod(), "sanity");
- nmethod *nm = (nmethod *)cb; - nmethod *nm = (nmethod *)cb;
+ assert(cb != nullptr && cb->is_nmethod(), "nmethod expected"); + assert(cb != nullptr && cb->is_nmethod(), "nmethod expected");
+ nmethod *nm = cb->as_nmethod(); + nmethod *nm = cb->as_nmethod();
if (nm->stub_contains(destination) && is_NativeCallTrampolineStub if (nm->stub_contains(destination) && is_NativeCallTrampolineStub
// Yes we do, so get the destination from the trampoline stub. // Yes we do, so get the destination from the trampoline stub.
const address trampoline_stub_addr = destination; const address trampoline_stub_addr = destination;
@@ -211,22 +212,18 @@ void NativeCall::set_destination_mt_safe(addre @@ -211,22 +212,18 @@ void NativeCall::set_destination_mt_safe(addre
} }
address NativeCall::get_trampoline() { address NativeCall::get_trampoline() {
- address call_addr = addr_at(0); - address call_addr = addr_at(0);
+ address call_addr = instruction_address(); + address call_addr = instruction_address();
CodeBlob *code = CodeCache::find_blob(call_addr); CodeBlob *code = CodeCache::find_blob(call_addr);
- assert(code != nullptr, "Could not find the containing code blob" | - assert(code != NULL, "Could not find the containing code blob");
+ assert(code != nullptr && code->is_nmethod(), "nmethod expected") + assert(code != nullptr && code->is_nmethod(), "nmethod expected")
+ nmethod* nm = code->as_nmethod(); + nmethod* nm = code->as_nmethod();
- address bl_destination - address bl_destination
- = MacroAssembler::pd_call_destination(call_addr); - = MacroAssembler::pd_call_destination(call_addr);
- if (code->contains(bl_destination) && - if (code->contains(bl_destination) &&
+ address bl_destination = call_addr + displacement(); + address bl_destination = call_addr + displacement();
+ if (nm->stub_contains(bl_destination) && + if (nm->stub_contains(bl_destination) &&
is_NativeCallTrampolineStub_at(bl_destination)) is_NativeCallTrampolineStub_at(bl_destination))
return bl_destination; return bl_destination;
- if (code->is_nmethod()) { - if (code->is_nmethod()) {
- return trampoline_stub_Relocation::get_trampoline_for(call_addr - return trampoline_stub_Relocation::get_trampoline_for(call_addr
- } - }
- -
- return nullptr; | - return NULL;
+ return trampoline_stub_Relocation::get_trampoline_for(call_addr, + return trampoline_stub_Relocation::get_trampoline_for(call_addr,
} }
// Inserts a native call instruction at a given pc // Inserts a native call instruction at a given pc
diff --git a/src/hotspot/cpu/aarch64/nativeInst_aarch64.hpp b/src/ho diff --git a/src/hotspot/cpu/aarch64/nativeInst_aarch64.hpp b/src/ho
--- a/src/hotspot/cpu/aarch64/nativeInst_aarch64.hpp --- a/src/hotspot/cpu/aarch64/nativeInst_aarch64.hpp
+++ b/src/hotspot/cpu/aarch64/nativeInst_aarch64.hpp +++ b/src/hotspot/cpu/aarch64/nativeInst_aarch64.hpp
@@ -1,5 +1,5 @@ @@ -1,5 +1,5 @@
/* /*
- * Copyright (c) 1997, 2023, Oracle and/or its affiliates. All righ | - * Copyright (c) 1997, 2021, Oracle and/or its affiliates. All righ
+ * Copyright (c) 1997, 2024, Oracle and/or its affiliates. All righ + * Copyright (c) 1997, 2024, Oracle and/or its affiliates. All righ
* Copyright (c) 2014, 2108, Red Hat Inc. All rights reserved. * Copyright (c) 2014, 2108, Red Hat Inc. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
* *
diff --git a/src/hotspot/cpu/aarch64/relocInfo_aarch64.cpp b/src/hot diff --git a/src/hotspot/cpu/aarch64/relocInfo_aarch64.cpp b/src/hot
--- a/src/hotspot/cpu/aarch64/relocInfo_aarch64.cpp --- a/src/hotspot/cpu/aarch64/relocInfo_aarch64.cpp
+++ b/src/hotspot/cpu/aarch64/relocInfo_aarch64.cpp +++ b/src/hotspot/cpu/aarch64/relocInfo_aarch64.cpp
@@ -60,13 +60,12 @@ void Relocation::pd_set_data_value(address x, in @@ -60,13 +60,12 @@ void Relocation::pd_set_data_value(address x, in
address Relocation::pd_call_destination(address orig_addr) { address Relocation::pd_call_destination(address orig_addr) {
assert(is_call(), "should be a call here"); assert(is_call(), "should be a call here");
- if (NativeCall::is_call_at(addr())) { - if (NativeCall::is_call_at(addr())) {
- address trampoline = nativeCall_at(addr())->get_trampoline(); - address trampoline = nativeCall_at(addr())->get_trampoline();
- if (trampoline) { - if (trampoline) {
- return nativeCallTrampolineStub_at(trampoline)->destination() - return nativeCallTrampolineStub_at(trampoline)->destination()
+ if (orig_addr == nullptr) { + if (orig_addr == nullptr) {
+ if (NativeCall::is_call_at(addr())) { + if (NativeCall::is_call_at(addr())) {
+ NativeCall* call = nativeCall_at(addr()); + NativeCall* call = nativeCall_at(addr());
+ return call->destination(); + return call->destination();
} }
- } - }
- if (orig_addr != nullptr) { | - if (orig_addr != NULL) {
+ } else { + } else {
address new_addr = MacroAssembler::pd_call_destination(orig_add address new_addr = MacroAssembler::pd_call_destination(orig_add
// If call is branch to self, don't try to relocate it, just le // If call is branch to self, don't try to relocate it, just le
// as branch to self. This happens during code generation if th // as branch to self. This happens during code generation if th
**Testing**
Ran tier1-3 on Linux aarch64 (release & slowdebug).
-------------
Commit messages:
- fixing clang error: pack_data_to overrides a member function but is not marked
- Backport 73e3e0edeb20c6f701b213423476f92fb05dd262
Changes: https://git.openjdk.org/jdk17u-dev/pull/3441/files
Webrev: https://webrevs.openjdk.org/?repo=jdk17u-dev&pr=3441&range=00
Issue: https://bugs.openjdk.org/browse/JDK-8321509
Stats: 74 lines in 6 files changed: 34 ins; 14 del; 26 mod
Patch: https://git.openjdk.org/jdk17u-dev/pull/3441.diff
Fetch: git fetch https://git.openjdk.org/jdk17u-dev.git pull/3441/head:pull/3441
PR: https://git.openjdk.org/jdk17u-dev/pull/3441
More information about the jdk-updates-dev
mailing list