Commit 0343623f authored by Ovidio de Jesús Ruiz-Henríquez's avatar Ovidio de Jesús Ruiz-Henríquez Committed by Commit Bot

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

This reverts commit 0744e76c.

Reason for revert: Suspect CL in causing compile errors on
ios-device-xcode-clang and ios-simulator-xcode-clang builders.

https://ci.chromium.org/p/chromium/builders/ci/ios-device-xcode-clang/121716
https://ci.chromium.org/p/chromium/builders/ci/ios-simulator-xcode-clang/98510


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}

TBR=gab@chromium.org,fdoray@chromium.org,mlippautz@chromium.org,bikineev@chromium.org

Change-Id: I6a1f3a20d2c19716f51a45a4a1bb0bcafb7a558a
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1046776
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2033439Reviewed-by: default avatarOvidio de Jesús Ruiz-Henríquez <odejesush@chromium.org>
Commit-Queue: Ovidio de Jesús Ruiz-Henríquez <odejesush@chromium.org>
Cr-Commit-Position: refs/heads/master@{#737535}
parent 959282b8
......@@ -30,13 +30,11 @@ class Optional;
namespace internal {
struct DummyUnionMember {};
template <typename T, bool = std::is_trivially_destructible<T>::value>
struct OptionalStorageBase {
// Provide non-defaulted default ctor to make sure it's not deleted by
// non-trivial T::T() in the union.
constexpr OptionalStorageBase() {}
// Initializing |empty_| here instead of using default member initializing
// to avoid errors in g++ 4.8.
constexpr OptionalStorageBase() : empty_('\0') {}
template <class... Args>
constexpr explicit OptionalStorageBase(in_place_t, Args&&... args)
......@@ -67,28 +65,19 @@ struct OptionalStorageBase {
bool is_populated_ = false;
union {
// |dummy_| exists so that the union will always be initialized, even when
// |empty_| 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'. 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_;
// constructor to be 'constexpr'.
char empty_;
T value_;
};
};
template <typename T>
struct OptionalStorageBase<T, true /* trivially destructible */> {
// Provide non-defaulted default ctor to make sure it's not deleted by
// non-trivial T::T() in the union.
constexpr OptionalStorageBase() {}
// Initializing |empty_| here instead of using default member initializing
// to avoid errors in g++ 4.8.
constexpr OptionalStorageBase() : empty_('\0') {}
template <class... Args>
constexpr explicit OptionalStorageBase(in_place_t, Args&&... args)
......@@ -117,19 +106,10 @@ struct OptionalStorageBase<T, true /* trivially destructible */> {
bool is_populated_ = false;
union {
// |dummy_| exists so that the union will always be initialized, even when
// |empty_| 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'. 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_;
// constructor to be 'constexpr'.
char empty_;
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