diff options
| -rw-r--r-- | SConstruct | 3 | ||||
| -rw-r--r-- | src/mongo/db/pipeline/document_internal.h | 2 | ||||
| -rw-r--r-- | src/mongo/db/pipeline/value.h | 24 | ||||
| -rw-r--r-- | src/mongo/db/pipeline/value_internal.h | 63 | ||||
| -rw-r--r-- | src/mongo/platform/atomic_word.h | 198 | ||||
| -rw-r--r-- | src/mongo/platform/atomic_word_test.cpp | 38 | ||||
| -rw-r--r-- | src/third_party/s2/s2loop.cc | 8 | ||||
| -rw-r--r-- | src/third_party/s2/s2polyline.cc | 8 |
8 files changed, 124 insertions, 220 deletions
diff --git a/SConstruct b/SConstruct index d6f230df760..a5e943fd293 100644 --- a/SConstruct +++ b/SConstruct @@ -2234,9 +2234,6 @@ def doConfigure(myenv): # exceptionToStatus(). See https://bugs.llvm.org/show_bug.cgi?id=34804 AddToCCFLAGSIfSupported(myenv, "-Wno-exceptions") - # These warnings begin in gcc-8.2 and we should get rid of these disables at some point. - AddToCXXFLAGSIfSupported(env, '-Wno-class-memaccess') - # Check if we can set "-Wnon-virtual-dtor" when "-Werror" is set. The only time we can't set it is on # clang 3.4, where a class with virtual function(s) and a non-virtual destructor throws a warning when diff --git a/src/mongo/db/pipeline/document_internal.h b/src/mongo/db/pipeline/document_internal.h index 82310f15ecb..ced49bcbe60 100644 --- a/src/mongo/db/pipeline/document_internal.h +++ b/src/mongo/db/pipeline/document_internal.h @@ -380,7 +380,7 @@ private: /// Initialize empty hash table void hashTabInit() { - memset(_hashTab, -1, hashTabBytes()); + memset(static_cast<void*>(_hashTab), -1, hashTabBytes()); } static unsigned hashKey(StringData name) { diff --git a/src/mongo/db/pipeline/value.h b/src/mongo/db/pipeline/value.h index e840577af0f..e6db7e8f0c7 100644 --- a/src/mongo/db/pipeline/value.h +++ b/src/mongo/db/pipeline/value.h @@ -84,7 +84,8 @@ public: /** Construct a Value * - * All types not listed will be rejected rather than converted (see private for why) + * All types not listed will be rejected rather than converted, to prevent unexpected implicit + * conversions to the accepted argument types (e.g. bool accepts any pointer). * * Note: Currently these are all explicit conversions. * I'm not sure if we want implicit or not. @@ -122,8 +123,17 @@ public: : _storage(BinData, BSONBinData(uuid.toCDR().data(), uuid.toCDR().length(), BinDataType::newUUID)) {} - explicit Value(const char*) = delete; // Use StringData instead to prevent accidentally - // terminating the string at the first null byte. + /** + * Force the use of StringData to prevent accidental NUL-termination. + */ + explicit Value(const char*) = delete; + + /** + * Prevent implicit conversions to the accepted argument types. + */ + template <typename InvalidArgumentType> + explicit Value(const InvalidArgumentType&) = delete; + // TODO: add an unsafe version that can share storage with the BSONElement /// Deep-convert from BSONElement to Value @@ -353,14 +363,6 @@ public: static Value deserializeForIDL(const BSONElement& element); private: - /** This is a "honeypot" to prevent unexpected implicit conversions to the accepted argument - * types. bool is especially bad since without this it will accept any pointer. - * - * Template argument name was chosen to make produced error easier to read. - */ - template <typename InvalidArgumentType> - explicit Value(const InvalidArgumentType& invalidArgument); - explicit Value(const ValueStorage& storage) : _storage(storage) {} // May contain embedded NUL bytes, does not check the type. diff --git a/src/mongo/db/pipeline/value_internal.h b/src/mongo/db/pipeline/value_internal.h index ec83b3599e6..b6fbee372c3 100644 --- a/src/mongo/db/pipeline/value_internal.h +++ b/src/mongo/db/pipeline/value_internal.h @@ -83,10 +83,7 @@ public: // constructor. Much code relies on every byte being predictably initialized to zero. // This is a "missing" Value - ValueStorage() { - zero(); - type = EOO; - } + ValueStorage() : ValueStorage(EOO) {} explicit ValueStorage(BSONType t) { zero(); @@ -165,12 +162,12 @@ public: } ValueStorage(const ValueStorage& rhs) { - memcpy(this, &rhs, sizeof(*this)); + memcpy(bytes, rhs.bytes, sizeof(bytes)); memcpyed(); } ValueStorage(ValueStorage&& rhs) noexcept { - memcpy(this, &rhs, sizeof(*this)); + memcpy(bytes, rhs.bytes, sizeof(bytes)); rhs.zero(); // Reset rhs to the missing state. TODO consider only doing this if refCounter. } @@ -178,7 +175,7 @@ public: DEV verifyRefCountingIfShould(); if (refCounter) intrusive_ptr_release(genericRCPtr); - DEV memset(this, 0xee, sizeof(*this)); + DEV memset(bytes, 0xee, sizeof(bytes)); } ValueStorage& operator=(const ValueStorage& rhs) { @@ -193,7 +190,7 @@ public: if (refCounter) intrusive_ptr_release(genericRCPtr); - memmove(this, &rhs, sizeof(*this)); + memmove(bytes, rhs.bytes, sizeof(bytes)); return *this; } @@ -202,17 +199,17 @@ public: if (refCounter) intrusive_ptr_release(genericRCPtr); - memmove(this, &rhs, sizeof(*this)); + memmove(bytes, rhs.bytes, sizeof(bytes)); rhs.zero(); // Reset rhs to the missing state. TODO consider only doing this if refCounter. return *this; } void swap(ValueStorage& rhs) { // Don't need to update ref-counts because they will be the same in the end - char temp[sizeof(ValueStorage)]; - memcpy(temp, this, sizeof(*this)); - memcpy(this, &rhs, sizeof(*this)); - memcpy(&rhs, temp, sizeof(*this)); + char temp[sizeof(bytes)]; + memcpy(temp, bytes, sizeof(bytes)); + memcpy(bytes, rhs.bytes, sizeof(bytes)); + memcpy(rhs.bytes, temp, sizeof(bytes)); } /// Call this after memcpying to update ref counts if needed @@ -299,37 +296,34 @@ public: } void zero() { - memset(this, 0, sizeof(*this)); - } - - // Byte-for-byte identical - bool identical(const ValueStorage& other) const { - return (i64[0] == other.i64[0] && i64[1] == other.i64[1]); + memset(bytes, 0, sizeof(bytes)); } void verifyRefCountingIfShould() const; // This data is public because this should only be used by Value which would be a friend union { + // cover the whole ValueStorage + uint8_t bytes[16]; #pragma pack(1) struct { - // byte 1 + // bytes[0] signed char type; - // byte 2 + // bytes[1] struct { - bool refCounter : 1; // true if we need to refCount - bool shortStr : 1; // true if we are using short strings - // reservedFlags: 6; + uint8_t refCounter : 1; // bit 0: true if we need to refCount + uint8_t shortStr : 1; // bit 1: true if we are using short strings + uint8_t reservedFlags : 6; }; - // bytes 3-16; + // bytes[2:15] union { unsigned char oid[12]; struct { char shortStrSize; // TODO Consider moving into flags union (4 bits) - char shortStrStorage[16 /*total bytes*/ - 3 /*offset*/ - 1 /*NUL byte*/]; + char shortStrStorage[sizeof(bytes) - 3 /*offset*/ - 1 /*NUL byte*/]; union { char nulTerminator; }; @@ -357,14 +351,17 @@ public: }; #pragma pack() - // covers the whole ValueStorage - long long i64[2]; - - // Forces the ValueStorage type to have at least pointer alignment. Can't use alignas on the - // type since that causes issues on MSVC. - void* forcePointerAlignment; + // Select void* alignment without interfering with any active pack directives. Can't use + // alignas(void*) on this union because that would prohibit ValueStorage from being tightly + // packed into a packed struct (though GCC does the tight packing anyway). + // + // Note that MSVC's behavior is GCC-incompatible. It obeys alignas even when a pack pragma + // is active. That causes padding on MSVC when ValueStorage is used as a member of class + // Value, which in turn is used as a member of packed class ValueElement. + // http://lists.llvm.org/pipermail/cfe-dev/2014-July/thread.html#38174 + void* pointerAlignment; }; }; MONGO_STATIC_ASSERT(sizeof(ValueStorage) == 16); MONGO_STATIC_ASSERT(alignof(ValueStorage) >= alignof(void*)); -} +} // namespace mongo diff --git a/src/mongo/platform/atomic_word.h b/src/mongo/platform/atomic_word.h index 60c13368737..5edbe2ae334 100644 --- a/src/mongo/platform/atomic_word.h +++ b/src/mongo/platform/atomic_word.h @@ -39,32 +39,45 @@ namespace mongo { -/** - * Instantiations of AtomicWord must be Integral, or Trivally Copyable and less than 8 bytes. - */ -template <typename _WordType, typename = void> -class AtomicWord; +namespace atomic_word_detail { + +enum class Category { kBasic, kArithmetic }; + +template <typename T> +constexpr Category getCategory() { + if (std::is_integral<T>() && !std::is_same<T, bool>()) + return Category::kArithmetic; + return Category::kBasic; +} + +template <typename T, Category = getCategory<T>()> +class Base; /** * Implementation of the AtomicWord interface in terms of the C++11 Atomics. + * Defines the operations provided by a non-incrementable AtomicWord. + * All operations have sequentially consistent semantics unless otherwise noted. */ -template <typename _WordType> -class AtomicWord<_WordType, stdx::enable_if_t<std::is_integral<_WordType>::value>> { +template <typename T> +class Base<T, Category::kBasic> { public: /** * Underlying value type. */ - typedef _WordType WordType; + using WordType = T; + + /** + * Construct a new word, default-initialized. + */ + constexpr Base() : _value() {} /** * Construct a new word with the given initial value. */ - explicit constexpr AtomicWord(WordType value = WordType(0)) : _value(value) {} + explicit constexpr Base(WordType v) : _value(v) {} /** * Gets the current value of this AtomicWord. - * - * Has acquire and release semantics. */ WordType load() const { return _value.load(); @@ -81,19 +94,15 @@ public: /** * Sets the value of this AtomicWord to "newValue". - * - * Has acquire and release semantics. */ void store(WordType newValue) { - return _value.store(newValue); + _value.store(newValue); } /** * Atomically swaps the current value of this with "newValue". * * Returns the old value. - * - * Has acquire and release semantics. */ WordType swap(WordType newValue) { return _value.exchange(newValue); @@ -104,8 +113,6 @@ public: * * If this value equals "expected", sets this to "newValue". * Always returns the original of this. - * - * Has acquire and release semantics. */ WordType compareAndSwap(WordType expected, WordType newValue) { // NOTE: Subtle: compare_exchange mutates its first argument. @@ -113,12 +120,28 @@ public: return expected; } +protected: + // MONGO_STATIC_ASSERT(std::atomic<WordType>::is_always_lockfree); // TODO C++17 + std::atomic<WordType> _value; // NOLINT +}; + +/** + * Has the basic operations, plus some arithmetic operations. + */ +template <typename T> +class Base<T, Category::kArithmetic> : public Base<T, Category::kBasic> { +private: + using Parent = Base<T, Category::kBasic>; + using Parent::_value; + +public: + using WordType = typename Parent::WordType; + using Parent::Parent; + /** * Get the current value of this, add "increment" and store it, atomically. * * Returns the value of this before incrementing. - * - * Has acquire and release semantics. */ WordType fetchAndAdd(WordType increment) { return _value.fetch_add(increment); @@ -134,10 +157,7 @@ public: /** * Get the current value of this, subtract "decrement" and store it, atomically. - * * Returns the value of this before decrementing. - * - * Has acquire and release semantics. */ WordType fetchAndSubtract(WordType decrement) { return _value.fetch_sub(decrement); @@ -145,10 +165,7 @@ public: /** * Get the current value of this, add "increment" and store it, atomically. - * * Returns the value of this after incrementing. - * - * Has acquire and release semantics. */ WordType addAndFetch(WordType increment) { return fetchAndAdd(increment) + increment; @@ -156,133 +173,30 @@ public: /** * Get the current value of this, subtract "decrement" and store it, atomically. - * * Returns the value of this after decrementing. - * - * Has acquire and release semantics. */ WordType subtractAndFetch(WordType decrement) { return fetchAndSubtract(decrement) - decrement; } - -private: - std::atomic<WordType> _value; // NOLINT }; +} // namespace atomic_word_detail + /** - * Implementation of the AtomicWord interface for non-integral types that are trivially copyable and - * fit in 8 bytes. For that implementation we flow reads and writes through memcpy'ing bytes in and - * out of a uint64_t, then relying on std::atomic<uint64_t>. + * Instantiations of AtomicWord must be scalar types. */ -template <typename _WordType> -class AtomicWord<_WordType, - stdx::enable_if_t<sizeof(_WordType) <= sizeof(uint64_t) && - !std::is_integral<_WordType>::value && - std::is_trivially_copyable<_WordType>::value>> { - using StorageType = uint64_t; - +template <typename T> +class AtomicWord : public atomic_word_detail::Base<T> { public: - /** - * Underlying value type. - */ - typedef _WordType WordType; - - /** - * Construct a new word with the given initial value. - */ - explicit AtomicWord(WordType value = WordType{}) { - store(value); - } - - // Used in invoking a zero'd out non-integral atomic word - struct ZeroInitTag {}; - /** - * Construct a new word with zero'd out bytes. Useful if you need a constexpr AtomicWord of - * non-integral type. - */ - constexpr explicit AtomicWord(ZeroInitTag) : _storage(0) {} - - /** - * Gets the current value of this AtomicWord. - * - * Has acquire and release semantics. - */ - WordType load() const { - return _fromStorage(_storage.load()); - } - - /** - * Gets the current value of this AtomicWord. - * - * Has relaxed semantics. - */ - WordType loadRelaxed() const { - return _fromStorage(_storage.load(std::memory_order_relaxed)); - } - - /** - * Sets the value of this AtomicWord to "newValue". - * - * Has acquire and release semantics. - */ - void store(WordType newValue) { - _storage.store(_toStorage(newValue)); - } - - /** - * Atomically swaps the current value of this with "newValue". - * - * Returns the old value. - * - * Has acquire and release semantics. - */ - WordType swap(WordType newValue) { - return _fromStorage(_storage.exchange(_toStorage(newValue))); - } - - /** - * Atomic compare and swap. - * - * If this value equals "expected", sets this to "newValue". - * Always returns the original of this. - * - * Has acquire and release semantics. - */ - WordType compareAndSwap(WordType expected, WordType newValue) { - // NOTE: Subtle: compare_exchange mutates its first argument. - auto v = _toStorage(expected); - _storage.compare_exchange_strong(v, _toStorage(newValue)); - return _fromStorage(v); - } - -private: - static WordType _fromStorage(StorageType storage) noexcept { - WordType v; - std::memcpy(&v, &storage, sizeof(v)); - return v; - } - - static StorageType _toStorage(WordType wordType) noexcept { - StorageType v = 0; - std::memcpy(&v, &wordType, sizeof(wordType)); - return v; - } - - std::atomic<StorageType> _storage; // NOLINT + MONGO_STATIC_ASSERT(!std::is_integral<T>::value || + sizeof(T) == sizeof(atomic_word_detail::Base<T>)); + using atomic_word_detail::Base<T>::Base; }; -#define _ATOMIC_WORD_DECLARE(NAME, WTYPE) \ - typedef class AtomicWord<WTYPE> NAME; \ - namespace { \ - MONGO_STATIC_ASSERT(sizeof(NAME) == sizeof(WTYPE)); \ - MONGO_STATIC_ASSERT(std::is_standard_layout<WTYPE>::value); \ - } // namespace - -_ATOMIC_WORD_DECLARE(AtomicUInt32, unsigned); -_ATOMIC_WORD_DECLARE(AtomicUInt64, unsigned long long); -_ATOMIC_WORD_DECLARE(AtomicInt32, int); -_ATOMIC_WORD_DECLARE(AtomicInt64, long long); -_ATOMIC_WORD_DECLARE(AtomicBool, bool); -#undef _ATOMIC_WORD_DECLARE +using AtomicUInt32 = AtomicWord<unsigned>; +using AtomicUInt64 = AtomicWord<unsigned long long>; +using AtomicInt32 = AtomicWord<int>; +using AtomicInt64 = AtomicWord<long long>; +using AtomicBool = AtomicWord<bool>; } // namespace mongo diff --git a/src/mongo/platform/atomic_word_test.cpp b/src/mongo/platform/atomic_word_test.cpp index 152571e991a..ed00ec3c70c 100644 --- a/src/mongo/platform/atomic_word_test.cpp +++ b/src/mongo/platform/atomic_word_test.cpp @@ -65,6 +65,18 @@ void testAtomicWordBasicOperations() { ASSERT_EQUALS(WordType(0), w.load()); } +enum TestEnum { E0, E1, E2, E3 }; + +TEST(AtomicWordTests, BasicOperationsEnum) { + MONGO_STATIC_ASSERT(sizeof(AtomicWord<TestEnum>) == sizeof(TestEnum)); + AtomicWord<TestEnum> w; + ASSERT_EQUALS(E0, w.load()); + ASSERT_EQUALS(E0, w.compareAndSwap(E0, E1)); + ASSERT_EQUALS(E1, w.load()); + ASSERT_EQUALS(E1, w.compareAndSwap(E0, E2)); + ASSERT_EQUALS(E1, w.load()); +} + TEST(AtomicWordTests, BasicOperationsUnsigned32Bit) { typedef AtomicUInt32::WordType WordType; testAtomicWordBasicOperations<AtomicUInt32>(); @@ -161,32 +173,6 @@ std::ostream& operator<<(std::ostream& os, const Chars& chars) { return (os << chars._storage.data()); } -TEST(AtomicWordTests, BasicOperationsComplex) { - using WordType = Chars; - - AtomicWord<WordType> checkZero(AtomicWord<WordType>::ZeroInitTag{}); - ASSERT_EQUALS(WordType(""), checkZero.load()); - - AtomicWord<WordType> w; - - ASSERT_EQUALS(WordType(), w.load()); - - w.store("b"); - ASSERT_EQUALS(WordType("b"), w.load()); - - ASSERT_EQUALS(WordType("b"), w.swap("c")); - ASSERT_EQUALS(WordType("c"), w.load()); - - ASSERT_EQUALS(WordType("c"), w.compareAndSwap("a", "b")); - ASSERT_EQUALS(WordType("c"), w.load()); - ASSERT_EQUALS(WordType("c"), w.compareAndSwap("c", "b")); - ASSERT_EQUALS(WordType("b"), w.load()); - - w.store("foo"); - ASSERT_EQUALS(WordType("foo"), w.compareAndSwap("foo", "bar")); - ASSERT_EQUALS(WordType("bar"), w.load()); -} - template <typename T> void verifyAtomicityHelper() { ASSERT(std::atomic<T>{}.is_lock_free()); // NOLINT diff --git a/src/third_party/s2/s2loop.cc b/src/third_party/s2/s2loop.cc index 7522bafc701..4f20733e19f 100644 --- a/src/third_party/s2/s2loop.cc +++ b/src/third_party/s2/s2loop.cc @@ -94,7 +94,9 @@ void S2Loop::Init(vector<S2Point> const& vertices) { vertices_ = NULL; } else { vertices_ = new S2Point[num_vertices_]; - memcpy(vertices_, &vertices[0], num_vertices_ * sizeof(vertices_[0])); + // mongodb: void* casts to silence a -Wclass-memaccess warning. + memcpy(static_cast<void*>(vertices_), static_cast<const void*>(&vertices[0]), + num_vertices_ * sizeof(vertices_[0])); } owns_vertices_ = true; bound_ = S2LatLngRect::Full(); @@ -265,7 +267,9 @@ S2Loop::S2Loop(S2Loop const* src) depth_(src->depth_), index_(this), num_find_vertex_calls_(0) { - memcpy(vertices_, src->vertices_, num_vertices_ * sizeof(vertices_[0])); + // mongodb: void* casts to silence a -Wclass-memaccess warning. + memcpy(static_cast<void*>(vertices_), static_cast<const void*>(src->vertices_), + num_vertices_ * sizeof(vertices_[0])); } S2Loop* S2Loop::Clone() const { diff --git a/src/third_party/s2/s2polyline.cc b/src/third_party/s2/s2polyline.cc index 112c653872b..5cf44a2a115 100644 --- a/src/third_party/s2/s2polyline.cc +++ b/src/third_party/s2/s2polyline.cc @@ -54,7 +54,9 @@ void S2Polyline::Init(vector<S2Point> const& vertices) { vertices_ = new S2Point[num_vertices_]; // Check (num_vertices_ > 0) to avoid invalid reference to vertices[0]. if (num_vertices_ > 0) { - memcpy(vertices_, &vertices[0], num_vertices_ * sizeof(vertices_[0])); + // mongodb: void* casts to silence a -Wclass-memaccess warning. + memcpy(static_cast<void*>(vertices_), static_cast<const void*>(&vertices[0]), + num_vertices_ * sizeof(vertices_[0])); } } @@ -103,7 +105,9 @@ bool S2Polyline::IsValid(vector<S2Point> const& v, string* err) { S2Polyline::S2Polyline(S2Polyline const* src) : num_vertices_(src->num_vertices_), vertices_(new S2Point[num_vertices_]) { - memcpy(vertices_, src->vertices_, num_vertices_ * sizeof(vertices_[0])); + // mongodb: void* casts to silence a -Wclass-memaccess warning. + memcpy(static_cast<void*>(vertices_), static_cast<const void*>(src->vertices_), + num_vertices_ * sizeof(vertices_[0])); } S2Polyline* S2Polyline::Clone() const { |