Skip to content

Commit

Permalink
Fix adopting AnyDependency storage in case the target `AnyDependenc…
Browse files Browse the repository at this point in the history
…y` 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
  • Loading branch information
QrczakMK committed Apr 5, 2024
1 parent de3e548 commit 49821ff
Show file tree
Hide file tree
Showing 2 changed files with 119 additions and 48 deletions.
40 changes: 33 additions & 7 deletions riegeli/base/any_dependency.h
Original file line number Diff line number Diff line change
Expand Up @@ -170,9 +170,12 @@ class
using NullMethods = any_dependency_internal::NullMethods<Handle>;
template <typename Manager>
using MethodsFor = any_dependency_internal::MethodsFor<
Handle, Manager,
any_dependency_internal::IsInline<Handle, inline_size, inline_align,
Manager>::value>;
Handle, Manager, IsInline<Handle, Manager, inline_size, inline_align>()>;

static constexpr size_t kAvailableSize =
AvailableSize<Handle, inline_size, inline_align>();
static constexpr size_t kAvailableAlign =
AvailableAlign<Handle, inline_size, inline_align>();

template <typename DependentHandle = Handle,
std::enable_if_t<IsComparableAgainstNullptr<DependentHandle>::value,
Expand All @@ -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 <typename Handle, size_t inline_size, size_t inline_align>
constexpr size_t
AnyDependencyBase<Handle, inline_size, inline_align>::kAvailableSize;
constexpr size_t
AnyDependencyBase<Handle, inline_size, inline_align>::kAvailableAlign;
#endif

} // namespace any_dependency_internal

// `AnyDependency<Handle>` refers to an optionally owned object which is
Expand Down Expand Up @@ -575,10 +589,21 @@ template <typename Manager,
std::enable_if_t<IsAnyDependency<Handle, Manager>::value, int>>
inline void AnyDependencyBase<Handle, inline_size, inline_align>::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<Handle, inline_size, inline_align>()` then
// `manager.methods_->used_size <=
// AvailableSize<Handle, inline_size, inline_align>()`.
// No need to check possibly at runtime.
if ((Manager::kAvailableSize <=
AvailableSize<Handle, inline_size, inline_align>() ||
manager.methods_->used_size <=
AvailableSize<Handle, inline_size, inline_align>()) &&
// Same for alignment.
(Manager::kAvailableAlign <=
AvailableAlign<Handle, inline_size, inline_align>() ||
manager.methods_->used_align <=
AvailableAlign<Handle, inline_size, inline_align>())) {
// Adopt `manager` instead of wrapping it.
manager.handle_ = SentinelHandle<Handle>();
methods_ = std::exchange(manager.methods_, &NullMethods::kMethods);
Expand Down Expand Up @@ -615,6 +640,7 @@ inline void AnyDependencyBase<Handle, inline_size, inline_align>::Initialize(
new (this) Manager(std::move(manager).Construct());
return;
}
// Materialize `Manager` to consider adopting its storage.
Initialize(std::move(manager).Reference());
}

Expand Down
127 changes: 86 additions & 41 deletions riegeli/base/any_dependency_internal.h
Original file line number Diff line number Diff line change
Expand Up @@ -58,37 +58,83 @@ struct Repr {
using Storage = char[];

// A `Dependency<Handle, Manager>` is stored inline in
// `Repr<Handle, inline_size, inline_align>` if it fits in that storage and is
// movable. If `inline_size == 0`, the dependency is also required to be stable
// `Repr<Handle, inline_size, inline_align>` 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 <typename Handle, size_t inline_size, size_t inline_align,
typename Manager, typename Enable = void>
struct IsInline : std::false_type {};

template <typename Handle, size_t inline_size, size_t inline_align,
typename Manager>
struct IsInline<
Handle, inline_size, inline_align, Manager,
std::enable_if_t<absl::conjunction<
std::integral_constant<
bool, sizeof(Dependency<Handle, Manager>) <=
sizeof(Repr<Handle, inline_size, inline_align>) &&
alignof(Dependency<Handle, Manager>) <=
alignof(Repr<Handle, inline_size, inline_align>)>,
std::is_move_constructible<Dependency<Handle, Manager>>,
absl::disjunction<
std::integral_constant<bool, (inline_size > 0)>,
absl::conjunction<
std::integral_constant<bool,
Dependency<Handle, Manager>::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 <typename Handle, size_t inline_size, size_t inline_align>
constexpr size_t AvailableSize() {
if (inline_size == 0) return 0;
return sizeof(Repr<Handle, inline_size, inline_align>);
}

// Returns `available_align`: `alignof` the storage, except that 0 means the
// minimal alignment of any inline storage.
template <typename Handle, size_t inline_size, size_t inline_align>
constexpr size_t AvailableAlign() {
if (alignof(Repr<Handle, inline_size, inline_align>) ==
alignof(Repr<Handle, 0, 0>)) {
return 0;
}
return alignof(Repr<Handle, inline_size, inline_align>);
}

// 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 <typename Handle, typename Manager>
constexpr size_t UsedSize() {
if (sizeof(Dependency<Handle, Manager>) <=
sizeof(Repr<Handle, 0, alignof(Dependency<Handle, Manager>)>) &&
Dependency<Handle, Manager>::kIsStable
#ifdef ABSL_ATTRIBUTE_TRIVIAL_ABI
,
absl::is_trivially_relocatable<Dependency<Handle, Manager>>
&& absl::is_trivially_relocatable<Dependency<Handle, Manager>>::value
#endif
>>>::value>> : std::true_type {
};
) {
return 0;
}
return sizeof(Dependency<Handle, Manager>);
}

// 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 <typename Handle, typename Manager>
constexpr size_t UsedAlign() {
if (alignof(Dependency<Handle, Manager>) <= alignof(Repr<Handle, 0, 0>)) {
return 0;
}
return alignof(Dependency<Handle, Manager>);
}

template <typename Handle, typename Manager, size_t inline_size,
size_t inline_align>
constexpr bool IsInline() {
return std::is_move_constructible<Dependency<Handle, Manager>>::value &&
UsedSize<Handle, Manager>() <=
AvailableSize<Handle, inline_size, inline_align>() &&
UsedAlign<Handle, Manager>() <=
AvailableAlign<Handle, inline_size, inline_align>();
}

// 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
Expand All @@ -109,11 +155,11 @@ template <typename Handle>
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<Manager>*` if `type_id` matches
// `std::remove_reference_t<Manager>`, otherwise returns `nullptr`.
Expand Down Expand Up @@ -174,8 +220,8 @@ struct NullMethods {

public:
static constexpr Methods<Handle> 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
Expand Down Expand Up @@ -233,15 +279,14 @@ struct MethodsFor<Handle, Manager, true> {
}

public:
static constexpr Methods<Handle> kMethods = {
Destroy,
sizeof(Dependency<Handle, Manager>),
alignof(Dependency<Handle, Manager>),
Move,
IsOwning,
MutableGetIf,
ConstGetIf,
RegisterSubobjects};
static constexpr Methods<Handle> kMethods = {Destroy,
Move,
UsedSize<Handle, Manager>(),
UsedAlign<Handle, Manager>(),
IsOwning,
MutableGetIf,
ConstGetIf,
RegisterSubobjects};
};

// Before C++17 if a constexpr static data member is ODR-used, its definition at
Expand Down Expand Up @@ -293,8 +338,8 @@ struct MethodsFor<Handle, Manager, false> {

public:
static constexpr Methods<Handle> 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
Expand Down

0 comments on commit 49821ff

Please sign in to comment.