From 2f28731c17b246bd70075f828dcafcd23547da5d Mon Sep 17 00:00:00 2001 From: David 'Digit' Turner 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 Reviewed-by: Jan Wilken Dörrie 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 #include +#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, "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(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(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(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& 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 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_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 Value::FindDoubleKey(StringPiece key) const { const Value* result = FindKey(key); if (result) { if (result->is_int()) - return base::make_optional(static_cast(result->int_value_)); - if (result->is_double()) - return base::make_optional(result->double_value_); + return static_cast(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; using DictStorage = flat_map>; using ListStorage = std::vector; + // 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 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, -- 2.21.0