From 49821ffd3adb4ea0ae50d6c3f59527b2abf878fe Mon Sep 17 00:00:00 2001 From: Marcin Kowalczyk Date: Fri, 5 Apr 2024 18:58:02 +0200 Subject: [PATCH] Fix adopting `AnyDependency` storage in case the target `AnyDependency` has `inline_size == 0` (which imposes additional constraints on the stored `Dependency` regarding its stability and trivial relocatability), while the source `AnyDependency` stores a `Dependency` which does not satisfy the additional constraints, yet its size fits. In this case the storage used to be adopted, and the resulting `AnyDependency` was declaring itself stable, even though the stored `Dependency` was not necessarily stable. Also, change `IsInline` from a type trait to a constexpr function. This will simplify a future change when available size and alignment are known only at runtime. PiperOrigin-RevId: 622207722 --- riegeli/base/any_dependency.h | 40 ++++++-- riegeli/base/any_dependency_internal.h | 127 +++++++++++++++++-------- 2 files changed, 119 insertions(+), 48 deletions(-) diff --git a/riegeli/base/any_dependency.h b/riegeli/base/any_dependency.h index 73a78756..6ef47710 100644 --- a/riegeli/base/any_dependency.h +++ b/riegeli/base/any_dependency.h @@ -170,9 +170,12 @@ class using NullMethods = any_dependency_internal::NullMethods; template using MethodsFor = any_dependency_internal::MethodsFor< - Handle, Manager, - any_dependency_internal::IsInline::value>; + Handle, Manager, IsInline()>; + + static constexpr size_t kAvailableSize = + AvailableSize(); + static constexpr size_t kAvailableAlign = + AvailableAlign(); template ::value, @@ -194,6 +197,17 @@ class Repr repr_; }; +// Before C++17 if a constexpr static data member is ODR-used, its definition at +// namespace scope is required. Since C++17 these definitions are deprecated: +// http://en.cppreference.com/w/cpp/language/static +#if !__cpp_inline_variables +template +constexpr size_t + AnyDependencyBase::kAvailableSize; +constexpr size_t + AnyDependencyBase::kAvailableAlign; +#endif + } // namespace any_dependency_internal // `AnyDependency` refers to an optionally owned object which is @@ -575,10 +589,21 @@ template ::value, int>> inline void AnyDependencyBase::Initialize( Manager&& manager) { - if ((sizeof(typename Manager::Repr) <= sizeof(Repr) || - manager.methods_->inline_size_used <= sizeof(Repr)) && - (alignof(typename Manager::Repr) <= alignof(Repr) || - manager.methods_->inline_align_used <= alignof(Repr))) { + // `manager.methods_->used_size <= Manager::kAvailableSize`, hence if + // `Manager::kAvailableSize <= + // AvailableSize()` then + // `manager.methods_->used_size <= + // AvailableSize()`. + // No need to check possibly at runtime. + if ((Manager::kAvailableSize <= + AvailableSize() || + manager.methods_->used_size <= + AvailableSize()) && + // Same for alignment. + (Manager::kAvailableAlign <= + AvailableAlign() || + manager.methods_->used_align <= + AvailableAlign())) { // Adopt `manager` instead of wrapping it. manager.handle_ = SentinelHandle(); methods_ = std::exchange(manager.methods_, &NullMethods::kMethods); @@ -615,6 +640,7 @@ inline void AnyDependencyBase::Initialize( new (this) Manager(std::move(manager).Construct()); return; } + // Materialize `Manager` to consider adopting its storage. Initialize(std::move(manager).Reference()); } diff --git a/riegeli/base/any_dependency_internal.h b/riegeli/base/any_dependency_internal.h index 6f22ce37..3e7bd851 100644 --- a/riegeli/base/any_dependency_internal.h +++ b/riegeli/base/any_dependency_internal.h @@ -58,37 +58,83 @@ struct Repr { using Storage = char[]; // A `Dependency` is stored inline in -// `Repr` if it fits in that storage and is -// movable. If `inline_size == 0`, the dependency is also required to be stable +// `Repr` if it is movable and it fits there. +// +// If `inline_size == 0`, the dependency is also required to be stable // (because then `AnyDependency` declares itself stable) and trivially // relocatable (because then `AnyDependency` declares itself with trivial ABI). -template -struct IsInline : std::false_type {}; - -template -struct IsInline< - Handle, inline_size, inline_align, Manager, - std::enable_if_t) <= - sizeof(Repr) && - alignof(Dependency) <= - alignof(Repr)>, - std::is_move_constructible>, - absl::disjunction< - std::integral_constant 0)>, - absl::conjunction< - std::integral_constant::kIsStable> +// Properties of inline storage in an `AnyDepenency` instance are expressed as +// two numbers: `available_size` and `available_align`, while constraints of a +// movable `Dependency` instance on its storage are expressed as two numbers: +// `used_size` and `used_align`, such that +// `used_size <= available_size && used_align <= available_align` implies that +// the movable `Dependency` can be stored inline in the `AnyDependency`. +// +// This formulation allows reevaluating the condition with different values of +// `available_size` and `available_align` when considering adopting the storage +// for a different `AnyDependency` instance, at either compile time or runtime. + +// Returns `available_size`: `sizeof` the storage, except that 0 indicates +// `inline_size == 0`, which means the minimal size of any inline storage with +// the given alignment, while also putting additional constraints on the +// `Dependency` (stability and trivial relocatability). +template +constexpr size_t AvailableSize() { + if (inline_size == 0) return 0; + return sizeof(Repr); +} + +// Returns `available_align`: `alignof` the storage, except that 0 means the +// minimal alignment of any inline storage. +template +constexpr size_t AvailableAlign() { + if (alignof(Repr) == + alignof(Repr)) { + return 0; + } + return alignof(Repr); +} + +// Returns `used_size`: `sizeof` the `Dependency`, except that 0 indicates +// compatibility with `inline_size == 0`, which means fitting under the minimal +// size of any inline storage with the given alignment, and satisfying +// additional constraints (stability and trivial relocatability). +template +constexpr size_t UsedSize() { + if (sizeof(Dependency) <= + sizeof(Repr)>) && + Dependency::kIsStable #ifdef ABSL_ATTRIBUTE_TRIVIAL_ABI - , - absl::is_trivially_relocatable> + && absl::is_trivially_relocatable>::value #endif - >>>::value>> : std::true_type { -}; + ) { + return 0; + } + return sizeof(Dependency); +} + +// Returns `used_align`: `alignof` the storage, except that 0 means fitting +// under the minimal alignment of any inline storage. Making this a special +// case allows to optimize out comparisons of a compile time `used_align` +// against a runtime `available_align`. +template +constexpr size_t UsedAlign() { + if (alignof(Dependency) <= alignof(Repr)) { + return 0; + } + return alignof(Dependency); +} + +template +constexpr bool IsInline() { + return std::is_move_constructible>::value && + UsedSize() <= + AvailableSize() && + UsedAlign() <= + AvailableAlign(); +} // Conditionally make the ABI trivial. To be used as a base class, with the // derived class having an unconditional `ABSL_ATTRIBUTE_TRIVIAL_ABI` (it will @@ -109,11 +155,11 @@ template struct Methods { // Destroys `self`. void (*destroy)(Storage self); - size_t inline_size_used; // Or 0 if inline storage is not used. - size_t inline_align_used; // Or 0 if inline storage is not used. // Constructs `self` and `*self_handle` by moving from `that`, and destroys // `that`. void (*move)(Storage self, Handle* self_handle, Storage that); + size_t used_size; + size_t used_align; bool (*is_owning)(const Storage self); // Returns the `std::remove_reference_t*` if `type_id` matches // `std::remove_reference_t`, otherwise returns `nullptr`. @@ -174,8 +220,8 @@ struct NullMethods { public: static constexpr Methods kMethods = { - Destroy, 0, 0, Move, IsOwning, MutableGetIf, ConstGetIf, - RegisterSubobjects}; + Destroy, Move, 0, 0, + IsOwning, MutableGetIf, ConstGetIf, RegisterSubobjects}; }; // Before C++17 if a constexpr static data member is ODR-used, its definition at @@ -233,15 +279,14 @@ struct MethodsFor { } public: - static constexpr Methods kMethods = { - Destroy, - sizeof(Dependency), - alignof(Dependency), - Move, - IsOwning, - MutableGetIf, - ConstGetIf, - RegisterSubobjects}; + static constexpr Methods kMethods = {Destroy, + Move, + UsedSize(), + UsedAlign(), + IsOwning, + MutableGetIf, + ConstGetIf, + RegisterSubobjects}; }; // Before C++17 if a constexpr static data member is ODR-used, its definition at @@ -293,8 +338,8 @@ struct MethodsFor { public: static constexpr Methods kMethods = { - Destroy, 0, 0, Move, IsOwning, MutableGetIf, ConstGetIf, - RegisterSubobjects}; + Destroy, Move, 0, 0, + IsOwning, MutableGetIf, ConstGetIf, RegisterSubobjects}; }; // Before C++17 if a constexpr static data member is ODR-used, its definition at