diff options
author | Antonio Sanchez <cantonios@google.com> | 2021-04-21 15:45:31 -0700 |
---|---|---|
committer | Rasmus Munk Larsen <rmlarsen@google.com> | 2021-04-22 18:45:19 +0000 |
commit | d213a0bcea2344aa3f6c9856da9f5b2a26ccec25 (patch) | |
tree | 2cc3668db7d8928455a0e051d6373873ba4a316e | |
parent | 85a76a16ea835fcfa7d4c185a338ae2aef9a272a (diff) |
DenseStorage safely copy/swap.
Fixes #2229.
For dynamic matrices with fixed-sized storage, only copy/swap
elements that have been set. Otherwise, this leads to inefficient
copying, and potential UB for non-initialized elements.
-rw-r--r-- | Eigen/src/Core/DenseStorage.h | 61 | ||||
-rw-r--r-- | Eigen/src/Core/util/Memory.h | 11 | ||||
-rw-r--r-- | test/SafeScalar.h | 30 | ||||
-rw-r--r-- | test/dense_storage.cpp | 144 | ||||
-rw-r--r-- | test/rvalue_types.cpp | 31 |
5 files changed, 199 insertions, 78 deletions
diff --git a/Eigen/src/Core/DenseStorage.h b/Eigen/src/Core/DenseStorage.h index f6e1d0af1..9acca6c90 100644 --- a/Eigen/src/Core/DenseStorage.h +++ b/Eigen/src/Core/DenseStorage.h @@ -163,6 +163,30 @@ struct plain_array<T, 0, MatrixOrArrayOptions, Alignment> EIGEN_DEVICE_FUNC plain_array(constructor_without_unaligned_array_assert) {} }; +struct plain_array_helper { + template<typename T, int Size, int MatrixOrArrayOptions, int Alignment> + EIGEN_DEVICE_FUNC EIGEN_STRONG_INLINE + static void copy(const plain_array<T, Size, MatrixOrArrayOptions, Alignment>& src, const Eigen::Index size, + plain_array<T, Size, MatrixOrArrayOptions, Alignment>& dst) { + smart_copy(src.array, src.array + size, dst.array); + } + + template<typename T, int Size, int MatrixOrArrayOptions, int Alignment> + EIGEN_DEVICE_FUNC EIGEN_STRONG_INLINE + static void swap(plain_array<T, Size, MatrixOrArrayOptions, Alignment>& a, const Eigen::Index a_size, + plain_array<T, Size, MatrixOrArrayOptions, Alignment>& b, const Eigen::Index b_size) { + if (a_size < b_size) { + std::swap_ranges(b.array, b.array + a_size, a.array); + smart_move(b.array + a_size, b.array + b_size, a.array + a_size); + } else if (a_size > b_size) { + std::swap_ranges(a.array, a.array + b_size, b.array); + smart_move(a.array + b_size, a.array + a_size, b.array + b_size); + } else { + std::swap_ranges(a.array, a.array + a_size, b.array); + } + } +}; + } // end namespace internal /** \internal @@ -268,21 +292,25 @@ template<typename T, int Size, int _Options> class DenseStorage<T, Size, Dynamic EIGEN_DEVICE_FUNC DenseStorage() : m_rows(0), m_cols(0) {} EIGEN_DEVICE_FUNC explicit DenseStorage(internal::constructor_without_unaligned_array_assert) : m_data(internal::constructor_without_unaligned_array_assert()), m_rows(0), m_cols(0) {} - EIGEN_DEVICE_FUNC DenseStorage(const DenseStorage& other) : m_data(other.m_data), m_rows(other.m_rows), m_cols(other.m_cols) {} + EIGEN_DEVICE_FUNC DenseStorage(const DenseStorage& other) + : m_data(internal::constructor_without_unaligned_array_assert()), m_rows(other.m_rows), m_cols(other.m_cols) + { + internal::plain_array_helper::copy(other.m_data, m_rows * m_cols, m_data); + } EIGEN_DEVICE_FUNC DenseStorage& operator=(const DenseStorage& other) { if (this != &other) { - m_data = other.m_data; m_rows = other.m_rows; m_cols = other.m_cols; + internal::plain_array_helper::copy(other.m_data, m_rows * m_cols, m_data); } return *this; } EIGEN_DEVICE_FUNC DenseStorage(Index, Index rows, Index cols) : m_rows(rows), m_cols(cols) {} EIGEN_DEVICE_FUNC void swap(DenseStorage& other) { - numext::swap(m_data,other.m_data); + internal::plain_array_helper::swap(m_data, m_rows * m_cols, other.m_data, other.m_rows * other.m_cols); numext::swap(m_rows,other.m_rows); numext::swap(m_cols,other.m_cols); } @@ -303,21 +331,26 @@ template<typename T, int Size, int _Cols, int _Options> class DenseStorage<T, Si EIGEN_DEVICE_FUNC DenseStorage() : m_rows(0) {} EIGEN_DEVICE_FUNC explicit DenseStorage(internal::constructor_without_unaligned_array_assert) : m_data(internal::constructor_without_unaligned_array_assert()), m_rows(0) {} - EIGEN_DEVICE_FUNC DenseStorage(const DenseStorage& other) : m_data(other.m_data), m_rows(other.m_rows) {} + EIGEN_DEVICE_FUNC DenseStorage(const DenseStorage& other) + : m_data(internal::constructor_without_unaligned_array_assert()), m_rows(other.m_rows) + { + internal::plain_array_helper::copy(other.m_data, m_rows * _Cols, m_data); + } + EIGEN_DEVICE_FUNC DenseStorage& operator=(const DenseStorage& other) { if (this != &other) { - m_data = other.m_data; m_rows = other.m_rows; + internal::plain_array_helper::copy(other.m_data, m_rows * _Cols, m_data); } return *this; } EIGEN_DEVICE_FUNC DenseStorage(Index, Index rows, Index) : m_rows(rows) {} EIGEN_DEVICE_FUNC void swap(DenseStorage& other) - { - numext::swap(m_data,other.m_data); - numext::swap(m_rows,other.m_rows); + { + internal::plain_array_helper::swap(m_data, m_rows * _Cols, other.m_data, other.m_rows * _Cols); + numext::swap(m_rows, other.m_rows); } EIGEN_DEVICE_FUNC Index rows(void) const EIGEN_NOEXCEPT {return m_rows;} EIGEN_DEVICE_FUNC EIGEN_CONSTEXPR Index cols(void) const EIGEN_NOEXCEPT {return _Cols;} @@ -336,20 +369,24 @@ template<typename T, int Size, int _Rows, int _Options> class DenseStorage<T, Si EIGEN_DEVICE_FUNC DenseStorage() : m_cols(0) {} EIGEN_DEVICE_FUNC explicit DenseStorage(internal::constructor_without_unaligned_array_assert) : m_data(internal::constructor_without_unaligned_array_assert()), m_cols(0) {} - EIGEN_DEVICE_FUNC DenseStorage(const DenseStorage& other) : m_data(other.m_data), m_cols(other.m_cols) {} + EIGEN_DEVICE_FUNC DenseStorage(const DenseStorage& other) + : m_data(internal::constructor_without_unaligned_array_assert()), m_cols(other.m_cols) + { + internal::plain_array_helper::copy(other.m_data, _Rows * m_cols, m_data); + } EIGEN_DEVICE_FUNC DenseStorage& operator=(const DenseStorage& other) { if (this != &other) { - m_data = other.m_data; m_cols = other.m_cols; + internal::plain_array_helper::copy(other.m_data, _Rows * m_cols, m_data); } return *this; } EIGEN_DEVICE_FUNC DenseStorage(Index, Index, Index cols) : m_cols(cols) {} EIGEN_DEVICE_FUNC void swap(DenseStorage& other) { - numext::swap(m_data,other.m_data); - numext::swap(m_cols,other.m_cols); + internal::plain_array_helper::swap(m_data, _Rows * m_cols, other.m_data, _Rows * other.m_cols); + numext::swap(m_cols, other.m_cols); } EIGEN_DEVICE_FUNC EIGEN_CONSTEXPR Index rows(void) const EIGEN_NOEXCEPT {return _Rows;} EIGEN_DEVICE_FUNC Index cols(void) const EIGEN_NOEXCEPT {return m_cols;} diff --git a/Eigen/src/Core/util/Memory.h b/Eigen/src/Core/util/Memory.h index 7cbe8a672..875318cdb 100644 --- a/Eigen/src/Core/util/Memory.h +++ b/Eigen/src/Core/util/Memory.h @@ -566,6 +566,17 @@ template<typename T> struct smart_memmove_helper<T,false> { } }; +#if EIGEN_HAS_RVALUE_REFERENCES +template<typename T> EIGEN_DEVICE_FUNC T* smart_move(T* start, T* end, T* target) +{ + return std::move(start, end, target); +} +#else +template<typename T> EIGEN_DEVICE_FUNC T* smart_move(T* start, T* end, T* target) +{ + return std::copy(start, end, target); +} +#endif /***************************************************************************** *** Implementation of runtime stack allocation (falling back to malloc) *** diff --git a/test/SafeScalar.h b/test/SafeScalar.h new file mode 100644 index 000000000..c5cb75717 --- /dev/null +++ b/test/SafeScalar.h @@ -0,0 +1,30 @@ + +// A Scalar that asserts for uninitialized access. +template<typename T> +class SafeScalar { + public: + SafeScalar() : initialized_(false) {} + SafeScalar(const SafeScalar& other) { + *this = other; + } + SafeScalar& operator=(const SafeScalar& other) { + val_ = T(other); + initialized_ = true; + return *this; + } + + SafeScalar(T val) : val_(val), initialized_(true) {} + SafeScalar& operator=(T val) { + val_ = val; + initialized_ = true; + } + + operator T() const { + VERIFY(initialized_ && "Uninitialized access."); + return val_; + } + + private: + T val_; + bool initialized_; +}; diff --git a/test/dense_storage.cpp b/test/dense_storage.cpp index 7fa25859d..36ccbb02c 100644 --- a/test/dense_storage.cpp +++ b/test/dense_storage.cpp @@ -8,17 +8,16 @@ // with this file, You can obtain one at http://mozilla.org/MPL/2.0/. #include "main.h" +#include "AnnoyingScalar.h" +#include "SafeScalar.h" #include <Eigen/Core> -template <typename T, int Rows, int Cols> -void dense_storage_copy() +template <typename T, int Size, int Rows, int Cols> +void dense_storage_copy(int rows, int cols) { - static const int Size = ((Rows==Dynamic || Cols==Dynamic) ? Dynamic : Rows*Cols); - typedef DenseStorage<T,Size, Rows,Cols, 0> DenseStorageType; + typedef DenseStorage<T, Size, Rows, Cols, 0> DenseStorageType; - const int rows = (Rows==Dynamic) ? 4 : Rows; - const int cols = (Cols==Dynamic) ? 3 : Cols; const int size = rows*cols; DenseStorageType reference(size, rows, cols); T* raw_reference = reference.data(); @@ -31,14 +30,11 @@ void dense_storage_copy() VERIFY_IS_EQUAL(raw_reference[i], raw_copied_reference[i]); } -template <typename T, int Rows, int Cols> -void dense_storage_assignment() +template <typename T, int Size, int Rows, int Cols> +void dense_storage_assignment(int rows, int cols) { - static const int Size = ((Rows==Dynamic || Cols==Dynamic) ? Dynamic : Rows*Cols); - typedef DenseStorage<T,Size, Rows,Cols, 0> DenseStorageType; + typedef DenseStorage<T, Size, Rows, Cols, 0> DenseStorageType; - const int rows = (Rows==Dynamic) ? 4 : Rows; - const int cols = (Cols==Dynamic) ? 3 : Cols; const int size = rows*cols; DenseStorageType reference(size, rows, cols); T* raw_reference = reference.data(); @@ -52,6 +48,34 @@ void dense_storage_assignment() VERIFY_IS_EQUAL(raw_reference[i], raw_copied_reference[i]); } +template <typename T, int Size, int Rows, int Cols> +void dense_storage_swap(int rows0, int cols0, int rows1, int cols1) +{ + typedef DenseStorage<T, Size, Rows, Cols, 0> DenseStorageType; + + const int size0 = rows0*cols0; + DenseStorageType a(size0, rows0, cols0); + for (int i=0; i<size0; ++i) { + a.data()[i] = static_cast<T>(i); + } + + const int size1 = rows1*cols1; + DenseStorageType b(size1, rows1, cols1); + for (int i=0; i<size1; ++i) { + b.data()[i] = static_cast<T>(-i); + } + + a.swap(b); + + for (int i=0; i<size0; ++i) { + VERIFY_IS_EQUAL(b.data()[i], static_cast<T>(i)); + } + + for (int i=0; i<size1; ++i) { + VERIFY_IS_EQUAL(a.data()[i], static_cast<T>(-i)); + } +} + template<typename T, int Size, std::size_t Alignment> void dense_storage_alignment() { @@ -78,30 +102,78 @@ void dense_storage_alignment() #endif } -EIGEN_DECLARE_TEST(dense_storage) -{ - dense_storage_copy<int,Dynamic,Dynamic>(); - dense_storage_copy<int,Dynamic,3>(); - dense_storage_copy<int,4,Dynamic>(); - dense_storage_copy<int,4,3>(); - - dense_storage_copy<float,Dynamic,Dynamic>(); - dense_storage_copy<float,Dynamic,3>(); - dense_storage_copy<float,4,Dynamic>(); - dense_storage_copy<float,4,3>(); +template<typename T> +void dense_storage_tests() { + // Dynamic Storage. + dense_storage_copy<T,Dynamic,Dynamic,Dynamic>(4, 3); + dense_storage_copy<T,Dynamic,Dynamic,3>(4, 3); + dense_storage_copy<T,Dynamic,4,Dynamic>(4, 3); + // Fixed Storage. + dense_storage_copy<T,12,4,3>(4, 3); + dense_storage_copy<T,12,Dynamic,Dynamic>(4, 3); + dense_storage_copy<T,12,4,Dynamic>(4, 3); + dense_storage_copy<T,12,Dynamic,3>(4, 3); + // Fixed Storage with Uninitialized Elements. + dense_storage_copy<T,18,Dynamic,Dynamic>(4, 3); + dense_storage_copy<T,18,4,Dynamic>(4, 3); + dense_storage_copy<T,18,Dynamic,3>(4, 3); - dense_storage_assignment<int,Dynamic,Dynamic>(); - dense_storage_assignment<int,Dynamic,3>(); - dense_storage_assignment<int,4,Dynamic>(); - dense_storage_assignment<int,4,3>(); - - dense_storage_assignment<float,Dynamic,Dynamic>(); - dense_storage_assignment<float,Dynamic,3>(); - dense_storage_assignment<float,4,Dynamic>(); - dense_storage_assignment<float,4,3>(); + // Dynamic Storage. + dense_storage_assignment<T,Dynamic,Dynamic,Dynamic>(4, 3); + dense_storage_assignment<T,Dynamic,Dynamic,3>(4, 3); + dense_storage_assignment<T,Dynamic,4,Dynamic>(4, 3); + // Fixed Storage. + dense_storage_assignment<T,12,4,3>(4, 3); + dense_storage_assignment<T,12,Dynamic,Dynamic>(4, 3); + dense_storage_assignment<T,12,4,Dynamic>(4, 3); + dense_storage_assignment<T,12,Dynamic,3>(4, 3); + // Fixed Storage with Uninitialized Elements. + dense_storage_assignment<T,18,Dynamic,Dynamic>(4, 3); + dense_storage_assignment<T,18,4,Dynamic>(4, 3); + dense_storage_assignment<T,18,Dynamic,3>(4, 3); + + // Dynamic Storage. + dense_storage_swap<T,Dynamic,Dynamic,Dynamic>(4, 3, 4, 3); + dense_storage_swap<T,Dynamic,Dynamic,Dynamic>(4, 3, 2, 1); + dense_storage_swap<T,Dynamic,Dynamic,Dynamic>(2, 1, 4, 3); + dense_storage_swap<T,Dynamic,Dynamic,3>(4, 3, 4, 3); + dense_storage_swap<T,Dynamic,Dynamic,3>(4, 3, 2, 3); + dense_storage_swap<T,Dynamic,Dynamic,3>(2, 3, 4, 3); + dense_storage_swap<T,Dynamic,4,Dynamic>(4, 3, 4, 3); + dense_storage_swap<T,Dynamic,4,Dynamic>(4, 3, 4, 1); + dense_storage_swap<T,Dynamic,4,Dynamic>(4, 1, 4, 3); + // Fixed Storage. + dense_storage_swap<T,12,4,3>(4, 3, 4, 3); + dense_storage_swap<T,12,Dynamic,Dynamic>(4, 3, 4, 3); + dense_storage_swap<T,12,Dynamic,Dynamic>(4, 3, 2, 1); + dense_storage_swap<T,12,Dynamic,Dynamic>(2, 1, 4, 3); + dense_storage_swap<T,12,4,Dynamic>(4, 3, 4, 3); + dense_storage_swap<T,12,4,Dynamic>(4, 3, 4, 1); + dense_storage_swap<T,12,4,Dynamic>(4, 1, 4, 3); + dense_storage_swap<T,12,Dynamic,3>(4, 3, 4, 3); + dense_storage_swap<T,12,Dynamic,3>(4, 3, 2, 3); + dense_storage_swap<T,12,Dynamic,3>(2, 3, 4, 3); + // Fixed Storage with Uninitialized Elements. + dense_storage_swap<T,18,Dynamic,Dynamic>(4, 3, 4, 3); + dense_storage_swap<T,18,Dynamic,Dynamic>(4, 3, 2, 1); + dense_storage_swap<T,18,Dynamic,Dynamic>(2, 1, 4, 3); + dense_storage_swap<T,18,4,Dynamic>(4, 3, 4, 3); + dense_storage_swap<T,18,4,Dynamic>(4, 3, 4, 1); + dense_storage_swap<T,18,4,Dynamic>(4, 1, 4, 3); + dense_storage_swap<T,18,Dynamic,3>(4, 3, 4, 3); + dense_storage_swap<T,18,Dynamic,3>(4, 3, 2, 3); + dense_storage_swap<T,18,Dynamic,3>(2, 3, 4, 3); + + dense_storage_alignment<T,16,8>(); + dense_storage_alignment<T,16,16>(); + dense_storage_alignment<T,16,32>(); + dense_storage_alignment<T,16,64>(); +} - dense_storage_alignment<float,16,8>(); - dense_storage_alignment<float,16,16>(); - dense_storage_alignment<float,16,32>(); - dense_storage_alignment<float,16,64>(); +EIGEN_DECLARE_TEST(dense_storage) +{ + dense_storage_tests<int>(); + dense_storage_tests<float>(); + dense_storage_tests<SafeScalar<float> >(); + dense_storage_tests<AnnoyingScalar>(); } diff --git a/test/rvalue_types.cpp b/test/rvalue_types.cpp index c20a32f79..2c9999ce8 100644 --- a/test/rvalue_types.cpp +++ b/test/rvalue_types.cpp @@ -13,41 +13,12 @@ #if EIGEN_HAS_CXX11 #include "MovableScalar.h" #endif +#include "SafeScalar.h" #include <Eigen/Core> using internal::UIntPtr; -// A Scalar that asserts for uninitialized access. -template<typename T> -class SafeScalar { - public: - SafeScalar() : initialized_(false) {} - SafeScalar(const SafeScalar& other) { - *this = other; - } - SafeScalar& operator=(const SafeScalar& other) { - val_ = T(other); - initialized_ = true; - return *this; - } - - SafeScalar(T val) : val_(val), initialized_(true) {} - SafeScalar& operator=(T val) { - val_ = val; - initialized_ = true; - } - - operator T() const { - VERIFY(initialized_ && "Uninitialized access."); - return val_; - } - - private: - T val_; - bool initialized_; -}; - #if EIGEN_HAS_RVALUE_REFERENCES template <typename MatrixType> void rvalue_copyassign(const MatrixType& m) |