From bfa921ae6ce068c53dfa708d6d3d2cddbad5fc33 Mon Sep 17 00:00:00 2001 From: Kim Barrett Date: Fri, 16 Dec 2022 20:47:40 +0000 Subject: [PATCH] 8160404: RelocationHolder constructors have bugs Reviewed-by: kvn, jrose, jvernee --- src/hotspot/cpu/x86/assembler_x86.cpp | 2 +- src/hotspot/share/code/relocInfo.cpp | 41 +++- src/hotspot/share/code/relocInfo.hpp | 271 ++++++++++++++++---------- 3 files changed, 205 insertions(+), 109 deletions(-) diff --git a/src/hotspot/cpu/x86/assembler_x86.cpp b/src/hotspot/cpu/x86/assembler_x86.cpp index 020f8739317f9..6446accbfac26 100644 --- a/src/hotspot/cpu/x86/assembler_x86.cpp +++ b/src/hotspot/cpu/x86/assembler_x86.cpp @@ -2495,7 +2495,7 @@ void Assembler::jmp_literal(address dest, RelocationHolder const& rspec) { assert(dest != NULL, "must have a target"); intptr_t disp = dest - (pc() + sizeof(int32_t)); assert(is_simm32(disp), "must be 32bit offset (jmp)"); - emit_data(disp, rspec.reloc(), call32_operand); + emit_data(disp, rspec, call32_operand); } void Assembler::jmpb_0(Label& L, const char* file, int line) { diff --git a/src/hotspot/share/code/relocInfo.cpp b/src/hotspot/share/code/relocInfo.cpp index 65b860c9d8501..3c2f02771146b 100644 --- a/src/hotspot/share/code/relocInfo.cpp +++ b/src/hotspot/share/code/relocInfo.cpp @@ -36,6 +36,9 @@ #include "utilities/align.hpp" #include "utilities/copy.hpp" +#include +#include + const RelocationHolder RelocationHolder::none; // its type is relocInfo::none @@ -235,12 +238,44 @@ Relocation* RelocIterator::reloc() { APPLY_TO_RELOCATIONS(EACH_TYPE); #undef EACH_TYPE assert(t == relocInfo::none, "must be padding"); - return new(_rh) Relocation(t); + _rh = RelocationHolder::none; + return _rh.reloc(); } +// Verify all the destructors are trivial, so we don't need to worry about +// destroying old contents of a RelocationHolder being assigned or destroyed. +#define VERIFY_TRIVIALLY_DESTRUCTIBLE_AUX(Reloc) \ + static_assert(std::is_trivially_destructible::value, "must be"); -//////// Methods for flyweight Relocation types +#define VERIFY_TRIVIALLY_DESTRUCTIBLE(name) \ + VERIFY_TRIVIALLY_DESTRUCTIBLE_AUX(PASTE_TOKENS(name, _Relocation)); + +APPLY_TO_RELOCATIONS(VERIFY_TRIVIALLY_DESTRUCTIBLE) +VERIFY_TRIVIALLY_DESTRUCTIBLE_AUX(Relocation) + +#undef VERIFY_TRIVIALLY_DESTRUCTIBLE_AUX +#undef VERIFY_TRIVIALLY_DESTRUCTIBLE + +// Define all the copy_into functions. These rely on all Relocation types +// being trivially destructible (verified above). So it doesn't matter +// whether the target holder has been previously initialized or not. There's +// no need to consider that distinction and destruct the relocation in an +// already initialized holder. +#define DEFINE_COPY_INTO_AUX(Reloc) \ + void Reloc::copy_into(RelocationHolder& holder) const { \ + copy_into_helper(*this, holder); \ + } +#define DEFINE_COPY_INTO(name) \ + DEFINE_COPY_INTO_AUX(PASTE_TOKENS(name, _Relocation)) + +APPLY_TO_RELOCATIONS(DEFINE_COPY_INTO) +DEFINE_COPY_INTO_AUX(Relocation) + +#undef DEFINE_COPY_INTO_AUX +#undef DEFINE_COPY_INTO + +//////// Methods for RelocationHolder RelocationHolder RelocationHolder::plus(int offset) const { if (offset != 0) { @@ -264,6 +299,8 @@ RelocationHolder RelocationHolder::plus(int offset) const { return (*this); } +//////// Methods for flyweight Relocation types + // some relocations can compute their own values address Relocation::value() { ShouldNotReachHere(); diff --git a/src/hotspot/share/code/relocInfo.hpp b/src/hotspot/share/code/relocInfo.hpp index c0232c79efc72..fdaf550298361 100644 --- a/src/hotspot/share/code/relocInfo.hpp +++ b/src/hotspot/share/code/relocInfo.hpp @@ -28,8 +28,10 @@ #include "memory/allocation.hpp" #include "oops/oopsHierarchy.hpp" #include "runtime/osInfo.hpp" -#include "utilities/macros.hpp" #include "utilities/globalDefinitions.hpp" +#include "utilities/macros.hpp" + +#include class nmethod; class CodeBlob; @@ -471,24 +473,70 @@ inline relocInfo prefix_relocInfo(int datalen = 0) { // the holder is "one size fits all". class RelocationHolder { friend class Relocation; - friend class CodeSection; private: - // this preallocated memory must accommodate all subclasses of Relocation - // (this number is assertion-checked in Relocation::operator new) - enum { _relocbuf_size = 5 }; - void* _relocbuf[ _relocbuf_size ]; + // A Relocation is "held" by placement constructing a Relocation into + // _relocbuf. Hence, _relocbuf must accomodate all subclasses of + // Relocation. We also need the Relocation base class to be at the same + // address as the start of the object, e.g. at the address of _relocbuf. + // Both of these requirements are checked (see emplace_relocation). + // The placement of the base class subobject isn't guaranteed by C++, since + // these aren't standard layout classes, but all supported implementations + // provide that behavior. If that changes, we can instead add a Relocation* + // _reloc member to capture the result of the placement new, and use that to + // access the base subobject. + static const size_t _relocbuf_size = 5 * sizeof(void*); + alignas(void*) char _relocbuf[_relocbuf_size]; + + template + void emplace_relocation(const Args&... args) { + static_assert(std::is_base_of::value, "not Relocation"); + static_assert(sizeof(Reloc) <= sizeof(_relocbuf), "_relocbuf too small"); + Relocation* reloc = ::new (_relocbuf) Reloc(args...); + // Verify the base class subobject of the object constructed into + // _relocbuf is at the same address as the derived object. + assert(static_cast(reloc) == _relocbuf, "invariant"); + } + + // Support for Relocation::copy_into. + // reloc should be a most derived object. + template + void copy_into_impl(const Reloc& reloc) { + emplace_relocation(reloc); + } + + // Tag for selecting the constructor below and carrying the type of the + // relocation object the new holder will (initially) contain. + template struct Construct {}; + + // Constructor used by construct(). Constructs a new holder containing a + // relocation of type Reloc that is constructed using the provided args. + template + RelocationHolder(Construct, const Args&... args) { + emplace_relocation(args...); + } public: - Relocation* reloc() const { return (Relocation*) &_relocbuf[0]; } + Relocation* reloc() const { return (Relocation*)_relocbuf; } inline relocInfo::relocType type() const; // Add a constant offset to a relocation. Helper for class Address. RelocationHolder plus(int offset) const; - inline RelocationHolder(); // initializes type to none + // Return a holder containing a relocation of type Reloc, constructed using args. + template + static RelocationHolder construct(const Args&... args) { + return RelocationHolder(Construct(), args...); + } + + RelocationHolder(); // Initializes type to none. - inline RelocationHolder(Relocation* r); // make a copy + // Depends on the destructor for all relocation types being trivial + // (verified in .cpp file). + ~RelocationHolder() = default; + + RelocationHolder(const RelocationHolder& from); + RelocationHolder& operator=(const RelocationHolder& from); static const RelocationHolder none; }; @@ -636,7 +684,6 @@ class RelocIterator : public StackObj { // So, the RelocIterator unpacks relocInfos into Relocations. class Relocation { - friend class RelocationHolder; friend class RelocIterator; private: @@ -658,19 +705,18 @@ class Relocation { assert(_binding != NULL, "must now be bound"); } - Relocation(relocInfo::relocType rtype) : _binding(NULL), _rtype(rtype) { } + explicit Relocation(relocInfo::relocType rtype) : _binding(NULL), _rtype(rtype) { } - static RelocationHolder newHolder() { - return RelocationHolder(); + // Helper for copy_into functions for derived classes. + // Forwards operation to RelocationHolder::copy_into_impl so that + // RelocationHolder only needs to befriend this class, rather than all + // derived classes that implement copy_into. + template + static void copy_into_helper(const Reloc& reloc, RelocationHolder& holder) { + holder.copy_into_impl(reloc); } public: - void* operator new(size_t size, const RelocationHolder& holder) throw() { - assert(size <= sizeof(holder._relocbuf), "Make _relocbuf bigger!"); - assert((void* const *)holder.reloc() == &holder._relocbuf[0], "ptrs must agree"); - return holder.reloc(); - } - // make a generic relocation for a given type (if possible) static RelocationHolder spec_simple(relocInfo::relocType rtype); @@ -793,8 +839,20 @@ class Relocation { int format() const { return binding()->format(); } public: + // Make a filler relocation. + Relocation() : Relocation(relocInfo::none) {} + + // Intentionally public non-virtual destructor, even though polymorphic. We + // never heap allocate a Relocation, so never delete through a base pointer. + // RelocationHolder depends on the destructor for all relocation types being + // trivial, so this must not be virtual (and hence non-trivial). + ~Relocation() = default; + relocInfo::relocType type() const { return _rtype; } + // Copy this relocation into holder. + virtual void copy_into(RelocationHolder& holder) const; + // is it a call instruction? virtual bool is_call() { return false; } @@ -818,17 +876,20 @@ class Relocation { // certain inlines must be deferred until class Relocation is defined: -inline RelocationHolder::RelocationHolder() { - // initialize the vtbl, just to keep things type-safe - new(*this) Relocation(relocInfo::none); -} +inline RelocationHolder::RelocationHolder() : + RelocationHolder(Construct()) +{} +inline RelocationHolder::RelocationHolder(const RelocationHolder& from) { + from.reloc()->copy_into(*this); +} -inline RelocationHolder::RelocationHolder(Relocation* r) { - // wordwise copy from r (ok if it copies garbage after r) - for (int i = 0; i < _relocbuf_size; i++) { - _relocbuf[i] = ((void**)r)[i]; - } +inline RelocationHolder& RelocationHolder::operator=(const RelocationHolder& from) { + // All Relocation types are trivially destructible (verified in .cpp file), + // so we don't need to destruct our old value before copying over it. + // If not for that we would need to decide what to do about self-assignment. + from.reloc()->copy_into(*this); + return *this; } relocInfo::relocType RelocationHolder::type() const { @@ -876,29 +937,29 @@ class DataRelocation : public Relocation { }; class post_call_nop_Relocation : public Relocation { - friend class RelocIterator; + friend class RelocationHolder; public: post_call_nop_Relocation() : Relocation(relocInfo::post_call_nop_type) { } static RelocationHolder spec() { - RelocationHolder rh = newHolder(); - new(rh) post_call_nop_Relocation(); - return rh; + return RelocationHolder::construct(); } + + void copy_into(RelocationHolder& holder) const; }; class entry_guard_Relocation : public Relocation { - friend class RelocIterator; + friend class RelocationHolder; public: entry_guard_Relocation() : Relocation(relocInfo::entry_guard_type) { } static RelocationHolder spec() { - RelocationHolder rh = newHolder(); - new(rh) entry_guard_Relocation(); - return rh; + return RelocationHolder::construct(); } + + void copy_into(RelocationHolder& holder) const; }; // A CallRelocation always points at a call instruction. @@ -923,9 +984,7 @@ class oop_Relocation : public DataRelocation { // an oop in the CodeBlob's oop pool static RelocationHolder spec(int oop_index, int offset = 0) { assert(oop_index > 0, "must be a pool-resident oop"); - RelocationHolder rh = newHolder(); - new(rh) oop_Relocation(oop_index, offset); - return rh; + return RelocationHolder::construct(oop_index, offset); } // an oop in the instruction stream static RelocationHolder spec_for_immediate() { @@ -935,11 +994,11 @@ class oop_Relocation : public DataRelocation { "Must return true so we will search for oops as roots etc. in the code."); const int oop_index = 0; const int offset = 0; // if you want an offset, use the oop pool - RelocationHolder rh = newHolder(); - new(rh) oop_Relocation(oop_index, offset); - return rh; + return RelocationHolder::construct(oop_index, offset); } + void copy_into(RelocationHolder& holder) const; + private: jint _oop_index; // if > 0, index into CodeBlob::oop_at jint _offset; // byte offset to apply to the oop itself @@ -947,7 +1006,7 @@ class oop_Relocation : public DataRelocation { oop_Relocation(int oop_index, int offset) : DataRelocation(relocInfo::oop_type), _oop_index(oop_index), _offset(offset) { } - friend class RelocIterator; + friend class RelocationHolder; oop_Relocation() : DataRelocation(relocInfo::oop_type) {} public: @@ -980,19 +1039,17 @@ class metadata_Relocation : public DataRelocation { // an metadata in the CodeBlob's metadata pool static RelocationHolder spec(int metadata_index, int offset = 0) { assert(metadata_index > 0, "must be a pool-resident metadata"); - RelocationHolder rh = newHolder(); - new(rh) metadata_Relocation(metadata_index, offset); - return rh; + return RelocationHolder::construct(metadata_index, offset); } // an metadata in the instruction stream static RelocationHolder spec_for_immediate() { const int metadata_index = 0; const int offset = 0; // if you want an offset, use the metadata pool - RelocationHolder rh = newHolder(); - new(rh) metadata_Relocation(metadata_index, offset); - return rh; + return RelocationHolder::construct(metadata_index, offset); } + void copy_into(RelocationHolder& holder) const; + private: jint _metadata_index; // if > 0, index into nmethod::metadata_at jint _offset; // byte offset to apply to the metadata itself @@ -1000,7 +1057,7 @@ class metadata_Relocation : public DataRelocation { metadata_Relocation(int metadata_index, int offset) : DataRelocation(relocInfo::metadata_type), _metadata_index(metadata_index), _offset(offset) { } - friend class RelocIterator; + friend class RelocationHolder; metadata_Relocation() : DataRelocation(relocInfo::metadata_type) { } // Fixes a Metadata pointer in the code. Most platforms embeds the @@ -1035,11 +1092,11 @@ class virtual_call_Relocation : public CallRelocation { // The oop_limit helps find the last associated set-oop. // (See comments at the top of this file.) static RelocationHolder spec(address cached_value, jint method_index = 0) { - RelocationHolder rh = newHolder(); - new(rh) virtual_call_Relocation(cached_value, method_index); - return rh; + return RelocationHolder::construct(cached_value, method_index); } + void copy_into(RelocationHolder& holder) const; + private: address _cached_value; // location of set-value instruction jint _method_index; // resolved method for a Java call @@ -1051,7 +1108,7 @@ class virtual_call_Relocation : public CallRelocation { assert(cached_value != NULL, "first oop address must be specified"); } - friend class RelocIterator; + friend class RelocationHolder; virtual_call_Relocation() : CallRelocation(relocInfo::virtual_call_type) { } public: @@ -1074,11 +1131,11 @@ class virtual_call_Relocation : public CallRelocation { class opt_virtual_call_Relocation : public CallRelocation { public: static RelocationHolder spec(int method_index = 0) { - RelocationHolder rh = newHolder(); - new(rh) opt_virtual_call_Relocation(method_index); - return rh; + return RelocationHolder::construct(method_index); } + void copy_into(RelocationHolder& holder) const; + private: jint _method_index; // resolved method for a Java call @@ -1086,7 +1143,7 @@ class opt_virtual_call_Relocation : public CallRelocation { : CallRelocation(relocInfo::opt_virtual_call_type), _method_index(method_index) { } - friend class RelocIterator; + friend class RelocationHolder; opt_virtual_call_Relocation() : CallRelocation(relocInfo::opt_virtual_call_type) {} public: @@ -1106,11 +1163,11 @@ class opt_virtual_call_Relocation : public CallRelocation { class static_call_Relocation : public CallRelocation { public: static RelocationHolder spec(int method_index = 0) { - RelocationHolder rh = newHolder(); - new(rh) static_call_Relocation(method_index); - return rh; + return RelocationHolder::construct(method_index); } + void copy_into(RelocationHolder& holder) const; + private: jint _method_index; // resolved method for a Java call @@ -1118,7 +1175,7 @@ class static_call_Relocation : public CallRelocation { : CallRelocation(relocInfo::static_call_type), _method_index(method_index) { } - friend class RelocIterator; + friend class RelocationHolder; static_call_Relocation() : CallRelocation(relocInfo::static_call_type) {} public: @@ -1137,11 +1194,11 @@ class static_call_Relocation : public CallRelocation { class static_stub_Relocation : public Relocation { public: static RelocationHolder spec(address static_call) { - RelocationHolder rh = newHolder(); - new(rh) static_stub_Relocation(static_call); - return rh; + return RelocationHolder::construct(static_call); } + void copy_into(RelocationHolder& holder) const; + private: address _static_call; // location of corresponding static_call @@ -1149,7 +1206,7 @@ class static_stub_Relocation : public Relocation { : Relocation(relocInfo::static_stub_type), _static_call(static_call) { } - friend class RelocIterator; + friend class RelocationHolder; static_stub_Relocation() : Relocation(relocInfo::static_stub_type) { } public: @@ -1166,29 +1223,27 @@ class runtime_call_Relocation : public CallRelocation { public: static RelocationHolder spec() { - RelocationHolder rh = newHolder(); - new(rh) runtime_call_Relocation(); - return rh; + return RelocationHolder::construct(); } + void copy_into(RelocationHolder& holder) const; + private: - friend class RelocIterator; + friend class RelocationHolder; runtime_call_Relocation() : CallRelocation(relocInfo::runtime_call_type) { } - - public: }; class runtime_call_w_cp_Relocation : public CallRelocation { public: static RelocationHolder spec() { - RelocationHolder rh = newHolder(); - new(rh) runtime_call_w_cp_Relocation(); - return rh; + return RelocationHolder::construct(); } + void copy_into(RelocationHolder& holder) const; + private: - friend class RelocIterator; + friend class RelocationHolder; runtime_call_w_cp_Relocation() : CallRelocation(relocInfo::runtime_call_w_cp_type), _offset(-4) /* <0 = invalid */ { } @@ -1218,10 +1273,11 @@ class runtime_call_w_cp_Relocation : public CallRelocation { class trampoline_stub_Relocation : public Relocation { public: static RelocationHolder spec(address static_call) { - RelocationHolder rh = newHolder(); - return (new (rh) trampoline_stub_Relocation(static_call)); + return RelocationHolder::construct(static_call); } + void copy_into(RelocationHolder& holder) const; + private: address _owner; // Address of the NativeCall that owns the trampoline. @@ -1229,7 +1285,7 @@ class trampoline_stub_Relocation : public Relocation { : Relocation(relocInfo::trampoline_stub_type), _owner(owner) { } - friend class RelocIterator; + friend class RelocationHolder; trampoline_stub_Relocation() : Relocation(relocInfo::trampoline_stub_type) { } public: @@ -1248,19 +1304,17 @@ class external_word_Relocation : public DataRelocation { public: static RelocationHolder spec(address target) { assert(target != NULL, "must not be null"); - RelocationHolder rh = newHolder(); - new(rh) external_word_Relocation(target); - return rh; + return RelocationHolder::construct(target); } // Use this one where all 32/64 bits of the target live in the code stream. // The target must be an intptr_t, and must be absolute (not relative). static RelocationHolder spec_for_immediate() { - RelocationHolder rh = newHolder(); - new(rh) external_word_Relocation(NULL); - return rh; + return RelocationHolder::construct(nullptr); } + void copy_into(RelocationHolder& holder) const; + // Some address looking values aren't safe to treat as relocations // and should just be treated as constants. static bool can_be_relocated(address target) { @@ -1274,7 +1328,7 @@ class external_word_Relocation : public DataRelocation { external_word_Relocation(address target) : DataRelocation(relocInfo::external_word_type), _target(target) { } - friend class RelocIterator; + friend class RelocationHolder; external_word_Relocation() : DataRelocation(relocInfo::external_word_type) { } public: @@ -1296,18 +1350,16 @@ class internal_word_Relocation : public DataRelocation { public: static RelocationHolder spec(address target) { assert(target != NULL, "must not be null"); - RelocationHolder rh = newHolder(); - new(rh) internal_word_Relocation(target); - return rh; + return RelocationHolder::construct(target); } // use this one where all the bits of the target can fit in the code stream: static RelocationHolder spec_for_immediate() { - RelocationHolder rh = newHolder(); - new(rh) internal_word_Relocation(NULL); - return rh; + return RelocationHolder::construct(nullptr); } + void copy_into(RelocationHolder& holder) const; + // default section -1 means self-relative internal_word_Relocation(address target, int section = -1, relocInfo::relocType type = relocInfo::internal_word_type) @@ -1317,7 +1369,7 @@ class internal_word_Relocation : public DataRelocation { address _target; // address in CodeBlob int _section; // section providing base address, if any - friend class RelocIterator; + friend class RelocationHolder; internal_word_Relocation(relocInfo::relocType type = relocInfo::internal_word_type) : DataRelocation(type) { } @@ -1341,11 +1393,11 @@ class internal_word_Relocation : public DataRelocation { class section_word_Relocation : public internal_word_Relocation { public: static RelocationHolder spec(address target, int section) { - RelocationHolder rh = newHolder(); - new(rh) section_word_Relocation(target, section); - return rh; + return RelocationHolder::construct(target, section); } + void copy_into(RelocationHolder& holder) const; + section_word_Relocation(address target, int section) : internal_word_Relocation(target, section, relocInfo::section_word_type) { assert(target != NULL, "must not be null"); @@ -1356,7 +1408,7 @@ class section_word_Relocation : public internal_word_Relocation { void unpack_data(); private: - friend class RelocIterator; + friend class RelocationHolder; section_word_Relocation() : internal_word_Relocation(relocInfo::section_word_type) { } }; @@ -1366,25 +1418,32 @@ class poll_Relocation : public Relocation { void fix_relocation_after_move(const CodeBuffer* src, CodeBuffer* dest); public: poll_Relocation(relocInfo::relocType type = relocInfo::poll_type) : Relocation(type) { } + + void copy_into(RelocationHolder& holder) const; }; class poll_return_Relocation : public poll_Relocation { public: poll_return_Relocation() : poll_Relocation(relocInfo::relocInfo::poll_return_type) { } + + void copy_into(RelocationHolder& holder) const; }; // We know all the xxx_Relocation classes, so now we can define these: -#define EACH_CASE(name) \ -inline name##_Relocation* RelocIterator::name##_reloc() { \ - assert(type() == relocInfo::name##_type, "type must agree"); \ - /* The purpose of the placed "new" is to re-use the same */ \ - /* stack storage for each new iteration. */ \ - name##_Relocation* r = new(_rh) name##_Relocation(); \ - r->set_binding(this); \ - r->name##_Relocation::unpack_data(); \ - return r; \ +#define EACH_CASE_AUX(Accessor, Reloc) \ +inline Reloc* RelocIterator::Accessor() { \ + static const RelocationHolder proto = RelocationHolder::construct(); \ + assert(type() == proto.type(), "type must agree"); \ + _rh = proto; \ + Reloc* r = static_cast(_rh.reloc()); \ + r->set_binding(this); \ + r->Reloc::unpack_data(); \ + return r; \ } +#define EACH_CASE(name) \ + EACH_CASE_AUX(PASTE_TOKENS(name, _reloc), PASTE_TOKENS(name, _Relocation)) APPLY_TO_RELOCATIONS(EACH_CASE); +#undef EACH_CASE_AUX #undef EACH_CASE inline RelocIterator::RelocIterator(CompiledMethod* nm, address begin, address limit) {