diff options
Diffstat (limited to 'www-client/chromium/files/chromium-74-2f28731.patch')
| -rw-r--r-- | www-client/chromium/files/chromium-74-2f28731.patch | 483 |
1 files changed, 0 insertions, 483 deletions
diff --git a/www-client/chromium/files/chromium-74-2f28731.patch b/www-client/chromium/files/chromium-74-2f28731.patch deleted file mode 100644 index 83452619fe06..000000000000 --- a/www-client/chromium/files/chromium-74-2f28731.patch +++ /dev/null @@ -1,483 +0,0 @@ -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 - |