Commit 70bd0916 authored by Anton Bikineev's avatar Anton Bikineev Committed by Commit Bot

Reland "[base] Use dummy class to avoid superfluous initialization in Optional"

Apple-clang complains about missing initializer in union. This fixes it.

Original change's description:
> [base] Use dummy class to avoid superfluous initialization in Optional
>
> The initialization of "char empty_" union-member in Optional is
> redundant and causes the compiler to generate extra code to ensure
> zero-initialization. Using an empty Dummy class for the union member is
> more optimal, since it guarantees that no code for its initialization is
> generated.
>
> Explicitly touching this char member in the constructor also causes a
> problem for conservative GC. Compiler is free to split shared and not
> shared parts of the union in separate memory locations on stack (or keep
> them in different registers). If a conservative GC is triggered at this
> moment, stack scanning may not find the correct object (this is exactly
> what happened in the bug 1044331).
>
> This can be tested in a minimized example: https://godbolt.org/z/t5q5ry.
>
> Bug: 1046776
>
> Change-Id: Idab1a0acf7d7d4f45dc9435a611b23f64e3c3c79
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2027948
> Commit-Queue: Anton Bikineev <bikineev@chromium.org>
> Reviewed-by: François Doray <fdoray@chromium.org>
> Reviewed-by: Gabriel Charette <gab@chromium.org>
> Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#737494}

Bug: 1046776

Change-Id: I751878a515647e244059d0ad1ba7bc6542fd1f37
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2033357Reviewed-by: default avatarFrançois Doray <fdoray@chromium.org>
Commit-Queue: Anton Bikineev <bikineev@chromium.org>
Cr-Commit-Position: refs/heads/master@{#737785}
parent 10a2b5de
......@@ -30,11 +30,13 @@ class Optional;
namespace internal {
struct DummyUnionMember {};
template <typename T, bool = std::is_trivially_destructible<T>::value>
struct OptionalStorageBase {
// Initializing |empty_| here instead of using default member initializing
// to avoid errors in g++ 4.8.
constexpr OptionalStorageBase() : empty_('\0') {}
// Provide non-defaulted default ctor to make sure it's not deleted by
// non-trivial T::T() in the union.
constexpr OptionalStorageBase() : dummy_() {}
template <class... Args>
constexpr explicit OptionalStorageBase(in_place_t, Args&&... args)
......@@ -65,19 +67,28 @@ struct OptionalStorageBase {
bool is_populated_ = false;
union {
// |empty_| exists so that the union will always be initialized, even when
// |dummy_| exists so that the union will always be initialized, even when
// it doesn't contain a value. Union members must be initialized for the
// constructor to be 'constexpr'.
char empty_;
// constructor to be 'constexpr'. Having a special trivial class for it is
// better than e.g. using char, because the latter will have to be
// zero-initialized, and the compiler can't optimize this write away, since
// it assumes this might be a programmer's invariant. This can also cause
// problems for conservative GC in Oilpan. Compiler is free to split shared
// and non-shared parts of the union in separate memory locations (or
// registers). If conservative GC is triggered at this moment, the stack
// scanning routine won't find the correct object pointed from
// Optional<HeapObject*>. This dummy valueless struct lets the compiler know
// that we don't care about the value of this union member.
DummyUnionMember dummy_;
T value_;
};
};
template <typename T>
struct OptionalStorageBase<T, true /* trivially destructible */> {
// Initializing |empty_| here instead of using default member initializing
// to avoid errors in g++ 4.8.
constexpr OptionalStorageBase() : empty_('\0') {}
// Provide non-defaulted default ctor to make sure it's not deleted by
// non-trivial T::T() in the union.
constexpr OptionalStorageBase() : dummy_() {}
template <class... Args>
constexpr explicit OptionalStorageBase(in_place_t, Args&&... args)
......@@ -106,10 +117,19 @@ struct OptionalStorageBase<T, true /* trivially destructible */> {
bool is_populated_ = false;
union {
// |empty_| exists so that the union will always be initialized, even when
// |dummy_| exists so that the union will always be initialized, even when
// it doesn't contain a value. Union members must be initialized for the
// constructor to be 'constexpr'.
char empty_;
// constructor to be 'constexpr'. Having a special trivial class for it is
// better than e.g. using char, because the latter will have to be
// zero-initialized, and the compiler can't optimize this write away, since
// it assumes this might be a programmer's invariant. This can also cause
// problems for conservative GC in Oilpan. Compiler is free to split shared
// and non-shared parts of the union in separate memory locations (or
// registers). If conservative GC is triggered at this moment, the stack
// scanning routine won't find the correct object pointed from
// Optional<HeapObject*>. This dummy valueless struct lets the compiler know
// that we don't care about the value of this union member.
DummyUnionMember dummy_;
T value_;
};
};
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment