From 569873f6cac257dda6fc342c16f27c4524fcbdfe Mon Sep 17 00:00:00 2001 From: AlexandreSinger Date: Wed, 27 Nov 2024 16:56:28 -0500 Subject: [PATCH 1/2] [NDMatrix] Resolved Dangling Reference Warning GCC13 introduced a warning for potentially dangling references. The NDMatrix class was creating temporary objects to store the offseted pointer and then returning refrences to these pointers. Although this is correct, since the pointer is owned by the base NDMatrix object, it was confusing the compiler which is not something you want to do in general. Changed how the NDMatrix proxy class works by storing the offset into the pointer directly and passing a reference to the base pointer instead. This makes it clear to the compiler that this pointer belongs to another class and we are returning a reference to a portion of that memory, which is allowed. --- libs/libvtrutil/src/vtr_ndmatrix.h | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/libs/libvtrutil/src/vtr_ndmatrix.h b/libs/libvtrutil/src/vtr_ndmatrix.h index 57571cc865..fb6a4ad39e 100644 --- a/libs/libvtrutil/src/vtr_ndmatrix.h +++ b/libs/libvtrutil/src/vtr_ndmatrix.h @@ -34,9 +34,10 @@ class NdMatrixProxy { * @param dim_stride: The stride of this dimension (i.e. how many element in memory between indicies of this dimension) * @param start: Pointer to the start of the sub-matrix this proxy represents */ - NdMatrixProxy(const size_t* dim_sizes, const size_t* dim_strides, T* start) + NdMatrixProxy(const size_t* dim_sizes, const size_t* dim_strides, size_t offset, const std::unique_ptr& start) : dim_sizes_(dim_sizes) , dim_strides_(dim_strides) + , offset_(offset) , start_(start) {} NdMatrixProxy& operator=(const NdMatrixProxy& other) = delete; @@ -50,7 +51,8 @@ class NdMatrixProxy { return NdMatrixProxy( dim_sizes_ + 1, // Pass the dimension information dim_strides_ + 1, // Pass the stride for the next dimension - start_ + dim_strides_[0] * index); // Advance to index in this dimension + offset_ + dim_strides_[0] * index, // Advance to index in this dimension + start_); // Pass the base pointer. } ///@brief [] operator @@ -62,7 +64,8 @@ class NdMatrixProxy { private: const size_t* dim_sizes_; const size_t* dim_strides_; - T* start_; + size_t offset_; + const std::unique_ptr& start_; }; ///@brief Base case: 1-dimensional array @@ -76,9 +79,10 @@ class NdMatrixProxy { * @param dim_stride: The stride of this dimension (i.e. how many element in memory between indicies of this dimension) * @param start: Pointer to the start of the sub-matrix this proxy represents */ - NdMatrixProxy(const size_t* dim_sizes, const size_t* dim_stride, T* start) + NdMatrixProxy(const size_t* dim_sizes, const size_t* dim_stride, size_t offset, const std::unique_ptr& start) : dim_sizes_(dim_sizes) , dim_strides_(dim_stride) + , offset_(offset) , start_(start) {} NdMatrixProxy& operator=(const NdMatrixProxy& other) = delete; @@ -89,7 +93,7 @@ class NdMatrixProxy { VTR_ASSERT_SAFE_MSG(index < dim_sizes_[0], "Index out of range (above dimension maximum)"); //Base case - return start_[index]; + return start_[offset_ + index]; } ///@brief [] operator @@ -108,7 +112,7 @@ class NdMatrixProxy { * not to clobber elements in other dimensions */ const T* data() const { - return start_; + return start_.get() + offset_; } ///@brief same as above but allow update the value @@ -120,7 +124,8 @@ class NdMatrixProxy { private: const size_t* dim_sizes_; const size_t* dim_strides_; - T* start_; + size_t offset_; + const std::unique_ptr& start_; }; /** @@ -359,7 +364,8 @@ class NdMatrix : public NdMatrixBase { return NdMatrixProxy( this->dim_sizes_.data() + 1, //Pass the dimension information this->dim_strides_.data() + 1, //Pass the stride for the next dimension - this->data_.get() + this->dim_strides_[0] * index); //Advance to index in this dimension + this->dim_strides_[0] * index, //Advance to index in this dimension + this->data_); //Pass the base pointer } /** From a2be7c565b538a43cc92578cb947b886850f302c Mon Sep 17 00:00:00 2001 From: AlexandreSinger Date: Thu, 28 Nov 2024 11:10:11 -0500 Subject: [PATCH 2/2] [NDMatrix] Added Comments to NDMatrixProxy Added comments based on Vaughn's suggestions. --- libs/libvtrutil/src/vtr_ndmatrix.h | 31 +++++++++++++++++++++++++++--- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/libs/libvtrutil/src/vtr_ndmatrix.h b/libs/libvtrutil/src/vtr_ndmatrix.h index fb6a4ad39e..b7d6f030d5 100644 --- a/libs/libvtrutil/src/vtr_ndmatrix.h +++ b/libs/libvtrutil/src/vtr_ndmatrix.h @@ -30,9 +30,9 @@ class NdMatrixProxy { * @brief Construct a matrix proxy object * * @param dim_sizes: Array of dimension sizes - * @param idim: The dimension associated with this proxy * @param dim_stride: The stride of this dimension (i.e. how many element in memory between indicies of this dimension) - * @param start: Pointer to the start of the sub-matrix this proxy represents + * @param offset: The offset from the start that this sub-matrix starts at. + * @param start: Pointer to the start of the base NDMatrix of this proxy */ NdMatrixProxy(const size_t* dim_sizes, const size_t* dim_strides, size_t offset, const std::unique_ptr& start) : dim_sizes_(dim_sizes) @@ -62,9 +62,21 @@ class NdMatrixProxy { } private: + /// @brief The sizes of each dimension of this proxy. This is an array of + /// length N. const size_t* dim_sizes_; + + /// @brief The stride of each dimension of this proxy. This is an array of + /// length N. const size_t* dim_strides_; + + /// @brief The offset from the base NDMatrix object that this sub-matrix + /// starts at. size_t offset_; + + /// @brief The pointer to the start of the base NDMatrix data. Since the + /// base NDMatrix object owns the memory, we hold onto a reference + /// to its unique pointer. This is safer than passing a bare pointer. const std::unique_ptr& start_; }; @@ -77,7 +89,8 @@ class NdMatrixProxy { * * @param dim_sizes: Array of dimension sizes * @param dim_stride: The stride of this dimension (i.e. how many element in memory between indicies of this dimension) - * @param start: Pointer to the start of the sub-matrix this proxy represents + * @param offset: The offset from the start that this sub-matrix starts at. + * @param start: Pointer to the start of the base NDMatrix of this proxy */ NdMatrixProxy(const size_t* dim_sizes, const size_t* dim_stride, size_t offset, const std::unique_ptr& start) : dim_sizes_(dim_sizes) @@ -122,9 +135,21 @@ class NdMatrixProxy { } private: + /// @brief The sizes of each dimension of this proxy. This is an array of + /// length N. const size_t* dim_sizes_; + + /// @brief The stride of each dimension of this proxy. This is an array of + /// length N. const size_t* dim_strides_; + + /// @brief The offset from the base NDMatrix object that this sub-matrix + /// starts at. size_t offset_; + + /// @brief The pointer to the start of the base NDMatrix data. Since the + /// base NDMatrix object owns the memory, we hold onto a reference + /// to its unique pointer. This is safer than passing a bare pointer. const std::unique_ptr& start_; };