File 05-basevalue.patch of Package chromium.openSUSE_Leap_15.0_Update
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
