path: root/www-client/chromium/files/chromium-74-2f28731.patch

From 2f28731c17b246bd70075f828dcafcd23547da5d Mon Sep 17 00:00:00 2001
From: David 'Digit' Turner <digit@google.com>
Date: Wed, 3 Apr 2019 14:32:09 +0000
Subject: [PATCH] base: Fix Value layout for GCC
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

It turns out that the previous changes to the base::Value
layout broke GCC compilation (see [1] for details).

This CL fixes the situation by using a new DoubleStorage
type that will store double values in a 4-byte aligned
struct, with bit_cast<> being used to convert between
double and DoubleStorage values in the implementation.

This ensures that base::Value remains as small as possible
in all cases. The small penalty is that loading/storing
double values on 32-bit ARM is slightly slower due to
the fact that the value is no longer 8-byte aligned.

+ Fix the ValuesTest.SizeOfValue test to work correctly,
  and disable it for debug builds, so it doesn't fail
  because debug versions of the internal containers
  are larger on certain systems.

[1] https://chromium-review.googlesource.com/c/chromium/src/+/1472716

BUG=646113
R=dcheng@chromium.org, pasko@chromium.org, lizeb@chromium.org, jdoerrie@chromium.org, jose.dapena@lge.com

Change-Id: I9a365407dc064ba1bdc19859706f4154a495921e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1550363
Commit-Queue: David Turner <digit@chromium.org>
Reviewed-by: Jan Wilken Dörrie <jdoerrie@chromium.org>
Cr-Commit-Position: refs/heads/master@{#647271}
---
 base/values.cc          | 67 +++++++++++++---------------
 base/values.h           | 94 ++++++++++------------------------------
 base/values_unittest.cc | 96 ++++++++++++++++++++++++++++++-----------
 3 files changed, 124 insertions(+), 133 deletions(-)

diff --git a/base/values.cc b/base/values.cc
index 9fed5b52d60e..16d686b0bee5 100644
--- a/base/values.cc
+++ b/base/values.cc
@@ -12,6 +12,7 @@
 #include <ostream>
 #include <utility>
 
+#include "base/bit_cast.h"
 #include "base/json/json_writer.h"
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
@@ -36,6 +37,9 @@ static_assert(std::is_standard_layout<Value>::value,
               "base::Value should be a standard-layout C++ class in order "
               "to avoid undefined behaviour in its implementation!");
 
+static_assert(sizeof(Value::DoubleStorage) == sizeof(double),
+              "The double and DoubleStorage types should have the same size");
+
 namespace {
 
 const char* const kTypeNames[] = {"null",   "boolean", "integer",    "double",
@@ -110,8 +114,6 @@ Value::Value(Value&& that) noexcept {
   InternalMoveConstructFrom(std::move(that));
 }
 
-Value::Value() noexcept : type_(Type::NONE) {}
-
 Value::Value(Type type) : type_(type) {
   // Initialize with the default value.
   switch (type_) {
@@ -125,7 +127,7 @@ Value::Value(Type type) : type_(type) {
       int_value_ = 0;
       return;
     case Type::DOUBLE:
-      double_value_ = 0.0;
+      double_value_ = bit_cast<DoubleStorage>(0.0);
       return;
     case Type::STRING:
       new (&string_value_) std::string();
@@ -149,21 +151,16 @@ Value::Value(Type type) : type_(type) {
   CHECK(false);
 }
 
-Value::Value(bool in_bool)
-    : bool_type_(Type::BOOLEAN),
-      bool_value_(in_bool) {}
+Value::Value(bool in_bool) : type_(Type::BOOLEAN), bool_value_(in_bool) {}
 
-Value::Value(int in_int)
-    : int_type_(Type::INTEGER),
-      int_value_(in_int) {}
+Value::Value(int in_int) : type_(Type::INTEGER), int_value_(in_int) {}
 
 Value::Value(double in_double)
-    : double_type_(Type::DOUBLE),
-      double_value_(in_double) {
-  if (!std::isfinite(double_value_)) {
+    : type_(Type::DOUBLE), double_value_(bit_cast<DoubleStorage>(in_double)) {
+  if (!std::isfinite(in_double)) {
     NOTREACHED() << "Non-finite (i.e. NaN or positive/negative infinity) "
                  << "values cannot be represented in JSON";
-    double_value_ = 0.0;
+    double_value_ = bit_cast<DoubleStorage>(0.0);
   }
 }
 
@@ -172,8 +169,7 @@ Value::Value(const char* in_string) : Value(std::string(in_string)) {}
 Value::Value(StringPiece in_string) : Value(std::string(in_string)) {}
 
 Value::Value(std::string&& in_string) noexcept
-    : string_type_(Type::STRING),
-      string_value_(std::move(in_string)) {
+    : type_(Type::STRING), string_value_(std::move(in_string)) {
   DCHECK(IsStringUTF8(string_value_));
 }
 
@@ -182,19 +178,15 @@ Value::Value(const char16* in_string16) : Value(StringPiece16(in_string16)) {}
 Value::Value(StringPiece16 in_string16) : Value(UTF16ToUTF8(in_string16)) {}
 
 Value::Value(const std::vector<char>& in_blob)
-    : binary_type_(Type::BINARY),
-      binary_value_(in_blob.begin(), in_blob.end()) {}
+    : type_(Type::BINARY), binary_value_(in_blob.begin(), in_blob.end()) {}
 
 Value::Value(base::span<const uint8_t> in_blob)
-    : binary_type_(Type::BINARY),
-      binary_value_(in_blob.begin(), in_blob.end()) {}
+    : type_(Type::BINARY), binary_value_(in_blob.begin(), in_blob.end()) {}
 
 Value::Value(BlobStorage&& in_blob) noexcept
-    : binary_type_(Type::BINARY),
-      binary_value_(std::move(in_blob)) {}
+    : type_(Type::BINARY), binary_value_(std::move(in_blob)) {}
 
-Value::Value(const DictStorage& in_dict)
-    : dict_type_(Type::DICTIONARY), dict_() {
+Value::Value(const DictStorage& in_dict) : type_(Type::DICTIONARY), dict_() {
   dict_.reserve(in_dict.size());
   for (const auto& it : in_dict) {
     dict_.try_emplace(dict_.end(), it.first,
@@ -203,18 +195,16 @@ Value::Value(const DictStorage& in_dict)
 }
 
 Value::Value(DictStorage&& in_dict) noexcept
-    : dict_type_(Type::DICTIONARY),
-      dict_(std::move(in_dict)) {}
+    : type_(Type::DICTIONARY), dict_(std::move(in_dict)) {}
 
-Value::Value(const ListStorage& in_list) : list_type_(Type::LIST), list_() {
+Value::Value(const ListStorage& in_list) : type_(Type::LIST), list_() {
   list_.reserve(in_list.size());
   for (const auto& val : in_list)
     list_.emplace_back(val.Clone());
 }
 
 Value::Value(ListStorage&& in_list) noexcept
-    : list_type_(Type::LIST),
-      list_(std::move(in_list)) {}
+    : type_(Type::LIST), list_(std::move(in_list)) {}
 
 Value& Value::operator=(Value&& that) noexcept {
   InternalCleanup();
@@ -223,6 +213,10 @@ Value& Value::operator=(Value&& that) noexcept {
   return *this;
 }
 
+double Value::AsDoubleInternal() const {
+  return bit_cast<double>(double_value_);
+}
+
 Value Value::Clone() const {
   switch (type_) {
     case Type::NONE:
@@ -232,7 +226,7 @@ Value Value::Clone() const {
     case Type::INTEGER:
       return Value(int_value_);
     case Type::DOUBLE:
-      return Value(double_value_);
+      return Value(AsDoubleInternal());
     case Type::STRING:
       return Value(string_value_);
     case Type::BINARY:
@@ -277,7 +271,7 @@ int Value::GetInt() const {
 
 double Value::GetDouble() const {
   if (is_double())
-    return double_value_;
+    return AsDoubleInternal();
   if (is_int())
     return int_value_;
   CHECK(false);
@@ -342,9 +336,10 @@ base::Optional<double> Value::FindDoubleKey(StringPiece key) const {
   const Value* result = FindKey(key);
   if (result) {
     if (result->is_int())
-      return base::make_optional(static_cast<double>(result->int_value_));
-    if (result->is_double())
-      return base::make_optional(result->double_value_);
+      return static_cast<double>(result->int_value_);
+    if (result->is_double()) {
+      return result->AsDoubleInternal();
+    }
   }
   return base::nullopt;
 }
@@ -601,7 +596,7 @@ bool Value::GetAsInteger(int* out_value) const {
 
 bool Value::GetAsDouble(double* out_value) const {
   if (out_value && is_double()) {
-    *out_value = double_value_;
+    *out_value = AsDoubleInternal();
     return true;
   }
   if (out_value && is_int()) {
@@ -696,7 +691,7 @@ bool operator==(const Value& lhs, const Value& rhs) {
     case Value::Type::INTEGER:
       return lhs.int_value_ == rhs.int_value_;
     case Value::Type::DOUBLE:
-      return lhs.double_value_ == rhs.double_value_;
+      return lhs.AsDoubleInternal() == rhs.AsDoubleInternal();
     case Value::Type::STRING:
       return lhs.string_value_ == rhs.string_value_;
     case Value::Type::BINARY:
@@ -741,7 +736,7 @@ bool operator<(const Value& lhs, const Value& rhs) {
     case Value::Type::INTEGER:
       return lhs.int_value_ < rhs.int_value_;
     case Value::Type::DOUBLE:
-      return lhs.double_value_ < rhs.double_value_;
+      return lhs.AsDoubleInternal() < rhs.AsDoubleInternal();
     case Value::Type::STRING:
       return lhs.string_value_ < rhs.string_value_;
     case Value::Type::BINARY:
diff --git a/base/values.h b/base/values.h
index 486fe7ff3976..c455936d4961 100644
--- a/base/values.h
+++ b/base/values.h
@@ -83,6 +83,8 @@ class BASE_EXPORT Value {
   using BlobStorage = std::vector<uint8_t>;
   using DictStorage = flat_map<std::string, std::unique_ptr<Value>>;
   using ListStorage = std::vector<Value>;
+  // See technical note below explaining why this is used.
+  using DoubleStorage = struct { alignas(4) char v[sizeof(double)]; };
 
   enum class Type {
     NONE = 0,
@@ -111,7 +113,10 @@ class BASE_EXPORT Value {
   static std::unique_ptr<Value> ToUniquePtrValue(Value val);
 
   Value(Value&& that) noexcept;
-  Value() noexcept;  // A null value.
+  Value() noexcept {}  // A null value
+  // Fun fact: using '= default' above instead of '{}' does not work because
+  // the compiler complains that the default constructor was deleted since
+  // the inner union contains fields with non-default constructors.
 
   // Value's copy constructor and copy assignment operator are deleted. Use this
   // to obtain a deep copy explicitly.
@@ -405,82 +410,29 @@ class BASE_EXPORT Value {
   size_t EstimateMemoryUsage() const;
 
  protected:
-  // Technical note:
-  // The naive way to implement a tagged union leads to wasted bytes
-  // in the object on CPUs like ARM ones, which impose an 8-byte alignment
-  // for double values. I.e. if one does something like:
+  // Special case for doubles, which are aligned to 8 bytes on some
+  // 32-bit architectures. In this case, a simple declaration as a
+  // double member would make the whole union 8 byte-aligned, which
+  // would also force 4 bytes of wasted padding space before it in
+  // the Value layout.
   //
-  //    struct TaggedValue {
-  //      int type_;                    // size = 1, align = 4
-  //      union {
-  //        bool bool_value_;           // size = 1, align = 1
-  //        int int_value_;             // size = 4, align = 4
-  //        double double_value_;       // size = 8, align = 8
-  //        std::string string_value_;  // size = 12, align = 4  (32-bit)
-  //      };
-  //    };
-  //
-  // The end result is that the union will have an alignment of 8, and a size
-  // of 16, due to 4 extra padding bytes following |string_value_| to respect
-  // the alignment requirement.
-  //
-  // As a consequence, the struct TaggedValue will have a size of 24 bytes,
-  // due to the size of the union (16), the size of |type_| (4) and 4 bytes
-  // of padding between |type_| and the union to respect its alignment.
-  //
-  // This means 8 bytes of unused memory per instance on 32-bit ARM!
-  //
-  // To reclaim these, a union of structs is used instead, in order to ensure
-  // that |double_value_| below is always located at an offset that is a
-  // multiple of 8, relative to the start of the overall data structure.
-  //
-  // Each struct must declare its own |type_| field, which must have a different
-  // name, to appease the C++ compiler.
-  //
-  // Using this technique sizeof(base::Value) == 16 on 32-bit ARM instead
-  // of 24, without losing any information. Results are unchanged for x86,
-  // x86_64 and arm64 (16, 32 and 32 bytes respectively).
+  // To override this, store the value as an array of 32-bit integers, and
+  // perform the appropriate bit casts when reading / writing to it.
+  Type type_ = Type::NONE;
+
   union {
-    struct {
-      // TODO(crbug.com/646113): Make these private once DictionaryValue and
-      // ListValue are properly inlined.
-      Type type_ : 8;
-    };
-    struct {
-      Type bool_type_ : 8;
-      bool bool_value_;
-    };
-    struct {
-      Type int_type_ : 8;
-      int int_value_;
-    };
-    struct {
-      Type double_type_ : 8;
-      // Subtle: On architectures that require it, the compiler will ensure
-      // that |double_value_|'s offset is a multiple of 8 (e.g. 32-bit ARM).
-      // See technical note above to understand why it is important.
-      double double_value_;
-    };
-    struct {
-      Type string_type_ : 8;
-      std::string string_value_;
-    };
-    struct {
-      Type binary_type_ : 8;
-      BlobStorage binary_value_;
-    };
-    struct {
-      Type dict_type_ : 8;
-      DictStorage dict_;
-    };
-    struct {
-      Type list_type_ : 8;
-      ListStorage list_;
-    };
+    bool bool_value_;
+    int int_value_;
+    DoubleStorage double_value_;
+    std::string string_value_;
+    BlobStorage binary_value_;
+    DictStorage dict_;
+    ListStorage list_;
   };
 
  private:
   friend class ValuesTest_SizeOfValue_Test;
+  double AsDoubleInternal() const;
   void InternalMoveConstructFrom(Value&& that);
   void InternalCleanup();
 
diff --git a/base/values_unittest.cc b/base/values_unittest.cc
index 2dd1c76afaa9..f3536a8612b1 100644
--- a/base/values_unittest.cc
+++ b/base/values_unittest.cc
@@ -26,45 +26,89 @@
 
 namespace base {
 
-// Test is currently incorrect on Windows x86.
-#if !defined(OS_WIN) || !defined(ARCH_CPU_X86)
+// Ensure that base::Value is as small as possible, i.e. that there is
+// no wasted space after the inner value due to alignment constraints.
+// Distinguish between the 'header' that includes |type_| and and the inner
+// value that follows it, which can be a bool, int, double, string, blob, list
+// or dict.
+//
+// This test is only enabled when NDEBUG is defined. This way the test will not
+// fail in debug builds that sometimes contain larger versions of the standard
+// containers used inside base::Value.
+#if defined(NDEBUG)
+
+static size_t AlignSizeTo(size_t size, size_t alignment) {
+  EXPECT_TRUE((alignment & (alignment - 1)) == 0)
+      << "Alignment " << alignment << " is not a power of 2!";
+  return (size + (alignment - 1u)) & ~(alignment - 1u);
+}
+
 TEST(ValuesTest, SizeOfValue) {
-  // Ensure that base::Value is as small as possible, i.e. that there is
-  // no wasted space after the inner value due to alignment constraints.
-  // Distinguish between the 'header' that includes |type_| and and the inner
-  // value that follows it, which can be a bool, int, double, string, blob, list
-  // or dict.
-#define INNER_TYPES_LIST(X)            \
-  X(bool, bool_value_)                 \
-  X(int, int_value_)                   \
-  X(double, double_value_)             \
-  X(std::string, string_value_)        \
-  X(Value::BlobStorage, binary_value_) \
-  X(Value::ListStorage, list_)         \
+#define INNER_TYPES_LIST(X)              \
+  X(bool, bool_value_)                   \
+  X(int, int_value_)                     \
+  X(Value::DoubleStorage, double_value_) \
+  X(std::string, string_value_)          \
+  X(Value::BlobStorage, binary_value_)   \
+  X(Value::ListStorage, list_)           \
   X(Value::DictStorage, dict_)
 
-#define INNER_STRUCT_LIMIT(type, value) offsetof(Value, value) + sizeof(type),
+#define INNER_FIELD_ALIGNMENT(type, value) alignof(type),
+
+  // The maximum alignment of each inner struct value field inside base::Value
+  size_t max_inner_value_alignment =
+      std::max({INNER_TYPES_LIST(INNER_FIELD_ALIGNMENT)});
+
+  // Check that base::Value has the smallest alignment possible. This would
+  // fail if the header would contain something that has a larger alignment
+  // than necessary.
+  EXPECT_EQ(max_inner_value_alignment, alignof(Value));
+
+  // Find the offset of each inner value. Which should normally not be
+  // larger than 4. Note that we use std::max(4, ...) because bool_value_
+  // could be stored just after the |bool_type_| field, with an offset of
+  // 1, and that would be ok.
+#define INNER_VALUE_START_OFFSET(type, value) offsetof(Value, value),
+
+  size_t min_inner_value_offset =
+      std::min({INNER_TYPES_LIST(INNER_VALUE_START_OFFSET)});
 
-  // Return the maximum size in bytes of each inner struct inside base::Value
-  size_t max_inner_struct_limit =
-      std::max({INNER_TYPES_LIST(INNER_STRUCT_LIMIT)});
+  // Inner fields may contain pointers, which have an alignment of 8
+  // on most 64-bit platforms.
+  size_t expected_min_offset = alignof(void*);
+
+  EXPECT_EQ(expected_min_offset, min_inner_value_offset);
 
   // Ensure that base::Value is not larger than necessary, i.e. that there is
-  // no un-necessary padding afte the structs due to alignment constraints of
+  // no un-necessary padding after the structs due to alignment constraints of
   // one of the inner fields.
-  EXPECT_EQ(max_inner_struct_limit, sizeof(Value));
-  if (max_inner_struct_limit != sizeof(Value)) {
+#define INNER_STRUCT_END_OFFSET(type, value) \
+  offsetof(Value, value) + sizeof(type),
+
+  // The maximum size in bytes of each inner struct inside base::Value,
+  size_t max_inner_struct_end_offset =
+      std::max({INNER_TYPES_LIST(INNER_STRUCT_END_OFFSET)});
+
+  // The expected value size.
+  size_t expected_value_size =
+      AlignSizeTo(max_inner_struct_end_offset, alignof(Value));
+
+  EXPECT_EQ(expected_value_size, sizeof(Value));
+  if (min_inner_value_offset != expected_min_offset ||
+      expected_value_size != sizeof(Value)) {
     // The following are useful to understand what's wrong when the EXPECT_EQ()
-    // above actually fails.
-#define PRINT_INNER_FIELD_INFO(x, y) \
-  LOG(INFO) << #y " type=" #x " size=" << sizeof(x) << " align=" << alignof(x);
+    // above actually fail.
+#define PRINT_INNER_FIELD_INFO(x, y)                           \
+  LOG(INFO) << #y " type=" #x " offset=" << offsetof(Value, y) \
+            << " size=" << sizeof(x) << " align=" << alignof(x);
 
     LOG(INFO) << "Value size=" << sizeof(Value) << " align=" << alignof(Value);
     INNER_TYPES_LIST(PRINT_INNER_FIELD_INFO)
-    LOG(INFO) << "max_inner_struct_limit=" << max_inner_struct_limit;
+    LOG(INFO) << "max_inner_struct_end_offset=" << max_inner_struct_end_offset;
   }
 }
-#endif
+
+#endif  // NDEBUG
 
 TEST(ValuesTest, TestNothrow) {
   static_assert(std::is_nothrow_move_constructible<Value>::value,
-- 
2.21.0