[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}

base/optional.h

@@ -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() {}
 
   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() {}
 
   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_;
   };
 };