Skip to content

Commit

Permalink
8321509: False positive in get_trampoline fast path causes crash
Browse files Browse the repository at this point in the history 
Reviewed-by: kvn, adinn, thartmann
  • Loading branch information
@dean-long
dean-long committed Jul 11, 2024
1 parent 9eb611e commit 73e3e0e
Show file tree
Hide file tree
Showing 6 changed files with 58 additions and 38 deletions.
4 changes: 3 additions & 1 deletion src/hotspot/cpu/aarch64/globalDefinitions_aarch64.hpp
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 1999, 2021, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 1999, 2024, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2014, 2015, Red Hat Inc. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
Expand Down Expand Up @@ -76,4 +76,6 @@ const bool CCallingConventionRequiresIntsAsLongs = false;

#define USE_POINTERS_TO_REGISTER_IMPL_ARRAY

#define USE_TRAMPOLINE_STUB_FIX_OWNER

#endif // CPU_AARCH64_GLOBALDEFINITIONS_AARCH64_HPP
39 changes: 18 additions & 21 deletions src/hotspot/cpu/aarch64/nativeInst_aarch64.cpp
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 1997, 2023, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 1997, 2024, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2014, 2020, Red Hat Inc. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
Expand Down Expand Up @@ -51,13 +51,18 @@ void NativeInstruction::wrote(int offset) {
}

address NativeCall::destination() const {
address addr = (address)this;
address destination = instruction_address() + displacement();
address addr = instruction_address();
address destination = addr + displacement();

// Performance optimization: no need to call find_blob() if it is a self-call
if (destination == addr) {
return destination;
}

// Do we use a trampoline stub for this call?
CodeBlob* cb = CodeCache::find_blob(addr);
assert(cb && cb->is_nmethod(), "sanity");
nmethod *nm = (nmethod *)cb;
assert(cb != nullptr && cb->is_nmethod(), "nmethod expected");
nmethod *nm = cb->as_nmethod();
if (nm->stub_contains(destination) && is_NativeCallTrampolineStub_at(destination)) {
// Yes we do, so get the destination from the trampoline stub.
const address trampoline_stub_addr = destination;
Expand All @@ -72,12 +77,8 @@ address NativeCall::destination() const {
// call instruction at all times.
//
// Used in the runtime linkage of calls; see class CompiledIC.
//
// Add parameter assert_lock to switch off assertion
// during code generation, where no patching lock is needed.
void NativeCall::set_destination_mt_safe(address dest, bool assert_lock) {
assert(!assert_lock ||
(Patching_lock->is_locked() || SafepointSynchronize::is_at_safepoint()) ||
void NativeCall::set_destination_mt_safe(address dest) {
assert((Patching_lock->is_locked() || SafepointSynchronize::is_at_safepoint()) ||
CompiledICLocker::is_safe(addr_at(0)),
"concurrent code patching");

Expand All @@ -104,22 +105,18 @@ void NativeCall::set_destination_mt_safe(address dest, bool assert_lock) {
}

address NativeCall::get_trampoline() {
address call_addr = addr_at(0);
address call_addr = instruction_address();

CodeBlob *code = CodeCache::find_blob(call_addr);
assert(code != nullptr, "Could not find the containing code blob");
assert(code != nullptr && code->is_nmethod(), "nmethod expected");
nmethod* nm = code->as_nmethod();

address bl_destination
= MacroAssembler::pd_call_destination(call_addr);
if (code->contains(bl_destination) &&
address bl_destination = call_addr + displacement();
if (nm->stub_contains(bl_destination) &&
is_NativeCallTrampolineStub_at(bl_destination))
return bl_destination;

if (code->is_nmethod()) {
return trampoline_stub_Relocation::get_trampoline_for(call_addr, (nmethod*)code);
}

return nullptr;
return trampoline_stub_Relocation::get_trampoline_for(call_addr, nm);
}

// Inserts a native call instruction at a given pc
Expand Down
7 changes: 3 additions & 4 deletions src/hotspot/cpu/aarch64/nativeInst_aarch64.hpp
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 1997, 2023, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 1997, 2024, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2014, 2108, Red Hat Inc. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
Expand Down Expand Up @@ -174,6 +174,7 @@ class NativeCall: public NativeInstruction {
int displacement() const { return (int_at(displacement_offset) << 6) >> 4; }
address displacement_address() const { return addr_at(displacement_offset); }
address return_address() const { return addr_at(return_address_offset); }
address raw_destination() const { return instruction_address() + displacement(); }
address destination() const;

void set_destination(address dest) {
Expand Down Expand Up @@ -213,9 +214,7 @@ class NativeCall: public NativeInstruction {
//
// Used in the runtime linkage of calls; see class CompiledIC.
// (Cf. 4506997 and 4479829, where threads witnessed garbage displacements.)

// The parameter assert_lock disables the assertion during code generation.
void set_destination_mt_safe(address dest, bool assert_lock = true);
void set_destination_mt_safe(address dest);

address get_trampoline();
#if INCLUDE_JVMCI
Expand Down
33 changes: 21 additions & 12 deletions src/hotspot/cpu/aarch64/relocInfo_aarch64.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,13 +60,12 @@ void Relocation::pd_set_data_value(address x, bool verify_only) {

address Relocation::pd_call_destination(address orig_addr) {
assert(is_call(), "should be a call here");
if (NativeCall::is_call_at(addr())) {
address trampoline = nativeCall_at(addr())->get_trampoline();
if (trampoline) {
return nativeCallTrampolineStub_at(trampoline)->destination();
if (orig_addr == nullptr) {
if (NativeCall::is_call_at(addr())) {
NativeCall* call = nativeCall_at(addr());
return call->destination();
}
}
if (orig_addr != nullptr) {
} else {
address new_addr = MacroAssembler::pd_call_destination(orig_addr);
// If call is branch to self, don't try to relocate it, just leave it
// as branch to self. This happens during code generation if the code
Expand All @@ -82,16 +81,26 @@ address Relocation::pd_call_destination(address orig_addr) {
void Relocation::pd_set_call_destination(address x) {
assert(is_call(), "should be a call here");
if (NativeCall::is_call_at(addr())) {
address trampoline = nativeCall_at(addr())->get_trampoline();
if (trampoline) {
nativeCall_at(addr())->set_destination_mt_safe(x, /* assert_lock */false);
return;
}
NativeCall* call = nativeCall_at(addr());
call->set_destination(x);
} else {
MacroAssembler::pd_patch_instruction(addr(), x);
}
MacroAssembler::pd_patch_instruction(addr(), x);
assert(pd_call_destination(addr()) == x, "fail in reloc");
}

void trampoline_stub_Relocation::pd_fix_owner_after_move() {
NativeCall* call = nativeCall_at(owner());
assert(call->raw_destination() == owner(), "destination should be empty");
address trampoline = addr();
address dest = nativeCallTrampolineStub_at(trampoline)->destination();
if (!Assembler::reachable_from_branch_at(owner(), dest)) {
dest = trampoline;
}
call->set_destination(dest);
}


address* Relocation::pd_address_in_code() {
return (address*)(addr() + 8);
}
Expand Down
8 changes: 8 additions & 0 deletions src/hotspot/share/code/relocInfo.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -373,6 +373,14 @@ void CallRelocation::fix_relocation_after_move(const CodeBuffer* src, CodeBuffer
}


#ifdef USE_TRAMPOLINE_STUB_FIX_OWNER
void trampoline_stub_Relocation::fix_relocation_after_move(const CodeBuffer* src, CodeBuffer* dest) {
// Finalize owner destination only for nmethods
if (dest->blob() != nullptr) return;
pd_fix_owner_after_move();
}
#endif

//// pack/unpack methods

void oop_Relocation::pack_data_to(CodeSection* dest) {
Expand Down
5 changes: 5 additions & 0 deletions src/hotspot/share/code/relocInfo.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1258,6 +1258,11 @@ class runtime_call_w_cp_Relocation : public CallRelocation {
// in the code, it can patch it to jump to the trampoline where is
// sufficient space for a far branch. Needed on PPC.
class trampoline_stub_Relocation : public Relocation {
#ifdef USE_TRAMPOLINE_STUB_FIX_OWNER
void pd_fix_owner_after_move();
void fix_relocation_after_move(const CodeBuffer* src, CodeBuffer* dest) override;
#endif

public:
static RelocationHolder spec(address static_call) {
return RelocationHolder::construct<trampoline_stub_Relocation>(static_call);
Expand Down

5 comments on commit 73e3e0e

@openjdk-notifier
Copy link

Choose a reason for hiding this comment

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

Review

Issues

@lutkerd
Copy link
Contributor

Choose a reason for hiding this comment

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

/backport jdk21u-dev

@openjdk
Copy link

@openjdk openjdk bot commented on 73e3e0e Jul 22, 2024

Choose a reason for hiding this comment

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

@lutkerd the backport was successfully created on the branch backport-lutkerd-73e3e0ed-master in my personal fork of openjdk/jdk21u-dev. To create a pull request with this backport targeting openjdk/jdk21u-dev:master, just click the following link:

➡️ Create pull request

The title of the pull request is automatically filled in correctly and below you find a suggestion for the pull request body:

Hi all,

This pull request contains a backport of commit 73e3e0ed from the openjdk/jdk repository.

The commit being backported was authored by Dean Long on 11 Jul 2024 and was reviewed by Vladimir Kozlov, Andrew Dinn and Tobias Hartmann.

Thanks!

If you need to update the source branch of the pull then run the following commands in a local clone of your personal fork of openjdk/jdk21u-dev:

$ git fetch https://github.com/openjdk-bots/jdk21u-dev.git backport-lutkerd-73e3e0ed-master:backport-lutkerd-73e3e0ed-master
$ git checkout backport-lutkerd-73e3e0ed-master
# make changes
$ git add paths/to/changed/files
$ git commit --message 'Describe additional changes made'
$ git push https://github.com/openjdk-bots/jdk21u-dev.git backport-lutkerd-73e3e0ed-master

@lutkerd
Copy link
Contributor

Choose a reason for hiding this comment

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

/backport jdk23u

@openjdk
Copy link

@openjdk openjdk bot commented on 73e3e0e Jul 25, 2024

Choose a reason for hiding this comment

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

@lutkerd the backport was successfully created on the branch backport-lutkerd-73e3e0ed-master in my personal fork of openjdk/jdk23u. To create a pull request with this backport targeting openjdk/jdk23u:master, just click the following link:

➡️ Create pull request

The title of the pull request is automatically filled in correctly and below you find a suggestion for the pull request body:

Hi all,

This pull request contains a backport of commit 73e3e0ed from the openjdk/jdk repository.

The commit being backported was authored by Dean Long on 11 Jul 2024 and was reviewed by Vladimir Kozlov, Andrew Dinn and Tobias Hartmann.

Thanks!

If you need to update the source branch of the pull then run the following commands in a local clone of your personal fork of openjdk/jdk23u:

$ git fetch https://github.com/openjdk-bots/jdk23u.git backport-lutkerd-73e3e0ed-master:backport-lutkerd-73e3e0ed-master
$ git checkout backport-lutkerd-73e3e0ed-master
# make changes
$ git add paths/to/changed/files
$ git commit --message 'Describe additional changes made'
$ git push https://github.com/openjdk-bots/jdk23u.git backport-lutkerd-73e3e0ed-master

⚠️ @lutkerd You are not yet a collaborator in my fork openjdk-bots/jdk23u. An invite will be sent out and you need to accept it before you can proceed.

Please sign in to comment.