diff --git base/values.cc base/values.cc index 2b0c6c8163d8..fc12313a7bb5 100644 --- base/values.cc +++ base/values.cc @@ -22,20 +22,6 @@ namespace base { -// base::Value must be standard layout to guarantee that writing to -// |bool_type_| then reading |type_| is defined behaviour. See: -// -// [class.union]: -// If a standard-layout union contains several standard-layout structs that -// share a common initial sequence (9.2), and if an object of this -// standard-layout union type contains one of the standard-layout structs, -// it is permitted to inspect the common initial sequence of any of -// standard-layout struct members; -// -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!"); - namespace { const char* const kTypeNames[] = {"null", "boolean", "integer", "double", @@ -90,7 +76,7 @@ std::unique_ptr CopyWithoutEmptyChildren(const Value& node) { } // namespace -constexpr uint16_t Value::kMagicIsAlive; +constexpr uint32_t Value::kMagicIsAlive; // static std::unique_ptr Value::CreateWithCopiedBuffer(const char* buffer, @@ -112,9 +98,9 @@ Value::Value(Value&& that) noexcept { InternalMoveConstructFrom(std::move(that)); } -Value::Value() noexcept : type_(Type::NONE), is_alive_(kMagicIsAlive) {} +Value::Value() noexcept : type_(Type::NONE) {} -Value::Value(Type type) : type_(type), is_alive_(kMagicIsAlive) { +Value::Value(Type type) : type_(type) { // Initialize with the default value. switch (type_) { case Type::NONE: @@ -144,20 +130,11 @@ Value::Value(Type type) : type_(type), is_alive_(kMagicIsAlive) { } } -Value::Value(bool in_bool) - : bool_type_(Type::BOOLEAN), - bool_is_alive_(kMagicIsAlive), - 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_is_alive_(kMagicIsAlive), - 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_is_alive_(kMagicIsAlive), - double_value_(in_double) { +Value::Value(double in_double) : type_(Type::DOUBLE), double_value_(in_double) { if (!std::isfinite(double_value_)) { NOTREACHED() << "Non-finite (i.e. NaN or positive/negative infinity) " << "values cannot be represented in JSON"; @@ -170,9 +147,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_is_alive_(kMagicIsAlive), - string_value_(std::move(in_string)) { + : type_(Type::STRING), string_value_(std::move(in_string)) { DCHECK(IsStringUTF8(string_value_)); } @@ -181,22 +156,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_is_alive_(kMagicIsAlive), - 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_is_alive_(kMagicIsAlive), - 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_is_alive_(kMagicIsAlive), - 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_is_alive_(kMagicIsAlive), 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, @@ -205,21 +173,16 @@ Value::Value(const DictStorage& in_dict) } Value::Value(DictStorage&& in_dict) noexcept - : dict_type_(Type::DICTIONARY), - dict_is_alive_(kMagicIsAlive), - dict_(std::move(in_dict)) {} + : type_(Type::DICTIONARY), dict_(std::move(in_dict)) {} -Value::Value(const ListStorage& in_list) - : list_type_(Type::LIST), list_is_alive_(kMagicIsAlive), 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_is_alive_(kMagicIsAlive), - list_(std::move(in_list)) {} + : type_(Type::LIST), list_(std::move(in_list)) {} Value& Value::operator=(Value&& that) noexcept { InternalCleanup(); @@ -733,7 +696,6 @@ size_t Value::EstimateMemoryUsage() const { void Value::InternalMoveConstructFrom(Value&& that) { type_ = that.type_; - is_alive_ = that.is_alive_; switch (type_) { case Type::NONE: diff --git base/values.h base/values.h index 7546fa53756d..00a357342d23 100644 --- base/values.h +++ base/values.h @@ -96,6 +96,10 @@ class BASE_EXPORT Value { // Note: Do not add more types. See the file-level comment above for why. }; + // Magic IsAlive signature to debug double frees. + // TODO(crbug.com/859477): Remove once root cause is found. + static constexpr uint32_t kMagicIsAlive = 0x15272f19; + // For situations where you want to keep ownership of your buffer, this // factory method creates a new BinaryValue by copying the contents of the // buffer that's passed in. @@ -375,100 +379,28 @@ class BASE_EXPORT Value { size_t EstimateMemoryUsage() const; protected: - // Magic IsAlive signature to debug double frees. - // TODO(crbug.com/859477): Remove once root cause is found. - static constexpr uint16_t kMagicIsAlive = 0x2f19; + // TODO(crbug.com/646113): Make these private once DictionaryValue and + // ListValue are properly inlined. + Type type_; - // 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: - // - // 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_| and |is_alive_| 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). union { - struct { - // TODO(crbug.com/646113): Make these private once DictionaryValue and - // ListValue are properly inlined. - Type type_ : 8; - - // IsAlive member to debug double frees. - // TODO(crbug.com/859477): Remove once root cause is found. - uint16_t is_alive_ = kMagicIsAlive; - }; - struct { - Type bool_type_ : 8; - uint16_t bool_is_alive_; - bool bool_value_; - }; - struct { - Type int_type_ : 8; - uint16_t int_is_alive_; - int int_value_; - }; - struct { - Type double_type_ : 8; - uint16_t double_is_alive_; - // 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; - uint16_t string_is_alive_; - std::string string_value_; - }; - struct { - Type binary_type_ : 8; - uint16_t binary_is_alive_; - BlobStorage binary_value_; - }; - struct { - Type dict_type_ : 8; - uint16_t dict_is_alive_; - DictStorage dict_; - }; - struct { - Type list_type_ : 8; - uint16_t list_is_alive_; - ListStorage list_; - }; + bool bool_value_; + int int_value_; + double double_value_; + std::string string_value_; + BlobStorage binary_value_; + DictStorage dict_; + ListStorage list_; }; private: - friend class ValuesTest_SizeOfValue_Test; void InternalMoveConstructFrom(Value&& that); void InternalCleanup(); + // IsAlive member to debug double frees. + // TODO(crbug.com/859477): Remove once root cause is found. + uint32_t is_alive_ = kMagicIsAlive; + DISALLOW_COPY_AND_ASSIGN(Value); }; diff --git base/values_unittest.cc base/values_unittest.cc index 0a641bcc7ef4..bedacd9b1dee 100644 --- base/values_unittest.cc +++ base/values_unittest.cc @@ -6,7 +6,6 @@ #include -#include #include #include #include @@ -16,7 +15,6 @@ #include #include "base/containers/adapters.h" -#include "base/logging.h" #include "base/strings/string16.h" #include "base/strings/string_piece.h" #include "base/strings/utf_string_conversions.h" @@ -25,43 +23,6 @@ namespace base { -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 |is_alive_| - // 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_) \ - X(Value::DictStorage, dict_) - -#define INNER_STRUCT_LIMIT(type, value) offsetof(Value, value) + sizeof(type), - - // 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)}); - - // 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 - // one of the inner fields. - EXPECT_EQ(max_inner_struct_limit, sizeof(Value)); - if (max_inner_struct_limit != 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); - - 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; - } -} - TEST(ValuesTest, TestNothrow) { static_assert(std::is_nothrow_move_constructible::value, "IsNothrowMoveConstructible");