From b2873dbb1d84dcd3f47ca718c5b0aa69318dd25c Mon Sep 17 00:00:00 2001 From: bluss Date: Thu, 13 May 2021 08:56:09 +0200 Subject: [PATCH 1/3] Add test for offset computation in empty array The first of the added tests failed because of bug #998, and the fix is verified by the test. The second and third testcases did not have an error, but were added for completeness. The first testcase is by SparrowLii, from the initial mention of the bug. --- tests/array-construct.rs | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/tests/array-construct.rs b/tests/array-construct.rs index 2b72ab039..c8948d1a3 100644 --- a/tests/array-construct.rs +++ b/tests/array-construct.rs @@ -7,6 +7,7 @@ use defmac::defmac; use ndarray::prelude::*; +use ndarray::arr3; use ndarray::Zip; #[test] @@ -164,6 +165,43 @@ fn test_ones() { assert_eq!(a, b); } +#[test] +fn test_from_shape_empty_with_neg_stride() { + // Issue #998, negative strides for an axis where it doesn't matter. + let s = [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13]; + let v = s[..12].to_vec(); + let v_ptr = v.as_ptr(); + let a = Array::from_shape_vec((2, 0, 2).strides((1, -4isize as usize, 2)), v).unwrap(); + assert_eq!(a, arr3(&[[[0; 2]; 0]; 2])); + assert_eq!(a.as_ptr(), v_ptr); +} + +#[test] +fn test_from_shape_with_neg_stride() { + // Issue #998, negative strides for an axis where it doesn't matter. + let s = [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13]; + let v = s[..12].to_vec(); + let v_ptr = v.as_ptr(); + let a = Array::from_shape_vec((2, 1, 2).strides((1, -4isize as usize, 2)), v).unwrap(); + assert_eq!(a, arr3(&[[[0, 2]], + [[1, 3]]])); + assert_eq!(a.as_ptr(), v_ptr); +} + +#[test] +fn test_from_shape_2_2_2_with_neg_stride() { + // Issue #998, negative strides for an axis where it doesn't matter. + let s = [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13]; + let v = s[..12].to_vec(); + let v_ptr = v.as_ptr(); + let a = Array::from_shape_vec((2, 2, 2).strides((1, -4isize as usize, 2)), v).unwrap(); + assert_eq!(a, arr3(&[[[4, 6], + [0, 2]], + [[5, 7], + [1, 3]]])); + assert_eq!(a.as_ptr(), v_ptr.wrapping_add(4)); +} + #[should_panic] #[test] fn deny_wraparound_zeros() { From 664ce2db7502eeff390de4198320a815d49f2f1d Mon Sep 17 00:00:00 2001 From: Jim Turner Date: Wed, 12 May 2021 19:55:00 -0400 Subject: [PATCH 2/3] Fix offset_from_ptr_to_memory for axis length 0 --- src/dimension/mod.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/dimension/mod.rs b/src/dimension/mod.rs index 4aa7c6641..17e5a30b8 100644 --- a/src/dimension/mod.rs +++ b/src/dimension/mod.rs @@ -410,9 +410,10 @@ fn to_abs_slice(axis_len: usize, slice: Slice) -> (usize, usize, isize) { /// This function computes the offset from the logically first element to the first element in /// memory of the array. The result is always <= 0. pub fn offset_from_ptr_to_memory(dim: &D, strides: &D) -> isize { - let offset = izip!(dim.slice(), strides.slice()).fold(0, |_offset, (d, s)| { - if (*s as isize) < 0 { - _offset + *s as isize * (*d as isize - 1) + let offset = izip!(dim.slice(), strides.slice()).fold(0, |_offset, (&d, &s)| { + let s = s as isize; + if s < 0 && d > 1 { + _offset + s * (d as isize - 1) } else { _offset } From 4ca0f498cd0094fc248aefe7f3740f55004a2f97 Mon Sep 17 00:00:00 2001 From: Jim Turner Date: Sun, 9 May 2021 14:57:13 -0400 Subject: [PATCH 3/3] Change offset_from_ptr_to_memory to offset_from_low_addr_ptr_to_logical_ptr --- src/dimension/mod.rs | 22 ++++++++++++---------- src/impl_constructors.rs | 4 ++-- src/impl_methods.rs | 10 +++++----- src/impl_views/constructors.rs | 6 +++--- 4 files changed, 22 insertions(+), 20 deletions(-) diff --git a/src/dimension/mod.rs b/src/dimension/mod.rs index 17e5a30b8..732006e2b 100644 --- a/src/dimension/mod.rs +++ b/src/dimension/mod.rs @@ -235,11 +235,12 @@ where /// units of `A` and in units of bytes between the least address and greatest /// address accessible by moving along all axes does not exceed `isize::MAX`. /// -/// Warning: This function is sufficient to check the invariants of ArrayBase only -/// if the pointer to the first element of the array is chosen such that the element -/// with the smallest memory address is at the start of data. (In other words, the -/// pointer to the first element of the array must be computed using offset_from_ptr_to_memory -/// so that negative strides are correctly handled.) +/// Warning: This function is sufficient to check the invariants of ArrayBase +/// only if the pointer to the first element of the array is chosen such that +/// the element with the smallest memory address is at the start of the +/// allocation. (In other words, the pointer to the first element of the array +/// must be computed using `offset_from_low_addr_ptr_to_logical_ptr` so that +/// negative strides are correctly handled.) pub(crate) fn can_index_slice( data: &[A], dim: &D, @@ -407,18 +408,19 @@ fn to_abs_slice(axis_len: usize, slice: Slice) -> (usize, usize, isize) { (start, end, step) } -/// This function computes the offset from the logically first element to the first element in -/// memory of the array. The result is always <= 0. -pub fn offset_from_ptr_to_memory(dim: &D, strides: &D) -> isize { +/// Returns the offset from the lowest-address element to the logically first +/// element. +pub fn offset_from_low_addr_ptr_to_logical_ptr(dim: &D, strides: &D) -> usize { let offset = izip!(dim.slice(), strides.slice()).fold(0, |_offset, (&d, &s)| { let s = s as isize; if s < 0 && d > 1 { - _offset + s * (d as isize - 1) + _offset - s * (d as isize - 1) } else { _offset } }); - offset + debug_assert!(offset >= 0); + offset as usize } /// Modify dimension, stride and return data pointer offset diff --git a/src/impl_constructors.rs b/src/impl_constructors.rs index 3336ec336..40a54d1da 100644 --- a/src/impl_constructors.rs +++ b/src/impl_constructors.rs @@ -20,7 +20,7 @@ use alloc::vec; use alloc::vec::Vec; use crate::dimension; -use crate::dimension::offset_from_ptr_to_memory; +use crate::dimension::offset_from_low_addr_ptr_to_logical_ptr; use crate::error::{self, ShapeError}; use crate::extension::nonnull::nonnull_from_vec_data; use crate::imp_prelude::*; @@ -492,7 +492,7 @@ where // debug check for issues that indicates wrong use of this constructor debug_assert!(dimension::can_index_slice(&v, &dim, &strides).is_ok()); - let ptr = nonnull_from_vec_data(&mut v).offset(-offset_from_ptr_to_memory(&dim, &strides)); + let ptr = nonnull_from_vec_data(&mut v).add(offset_from_low_addr_ptr_to_logical_ptr(&dim, &strides)); ArrayBase::from_data_ptr(DataOwned::new(v), ptr).with_strides_dim(strides, dim) } diff --git a/src/impl_methods.rs b/src/impl_methods.rs index 6c51b4515..c441efccd 100644 --- a/src/impl_methods.rs +++ b/src/impl_methods.rs @@ -20,7 +20,7 @@ use crate::dimension; use crate::dimension::IntoDimension; use crate::dimension::{ abs_index, axes_of, do_slice, merge_axes, move_min_stride_axis_to_last, - offset_from_ptr_to_memory, size_of_shape_checked, stride_offset, Axes, + offset_from_low_addr_ptr_to_logical_ptr, size_of_shape_checked, stride_offset, Axes, }; use crate::dimension::broadcast::co_broadcast; use crate::dimension::reshape_dim; @@ -1539,10 +1539,10 @@ where S: Data, { if self.is_contiguous() { - let offset = offset_from_ptr_to_memory(&self.dim, &self.strides); + let offset = offset_from_low_addr_ptr_to_logical_ptr(&self.dim, &self.strides); unsafe { Some(slice::from_raw_parts( - self.ptr.offset(offset).as_ptr(), + self.ptr.sub(offset).as_ptr(), self.len(), )) } @@ -1568,10 +1568,10 @@ where { if self.is_contiguous() { self.ensure_unique(); - let offset = offset_from_ptr_to_memory(&self.dim, &self.strides); + let offset = offset_from_low_addr_ptr_to_logical_ptr(&self.dim, &self.strides); unsafe { Ok(slice::from_raw_parts_mut( - self.ptr.offset(offset).as_ptr(), + self.ptr.sub(offset).as_ptr(), self.len(), )) } diff --git a/src/impl_views/constructors.rs b/src/impl_views/constructors.rs index fd9457fa1..98cb81fcc 100644 --- a/src/impl_views/constructors.rs +++ b/src/impl_views/constructors.rs @@ -13,7 +13,7 @@ use crate::error::ShapeError; use crate::extension::nonnull::nonnull_debug_checked_from_ptr; use crate::imp_prelude::*; use crate::{is_aligned, StrideShape}; -use crate::dimension::offset_from_ptr_to_memory; +use crate::dimension::offset_from_low_addr_ptr_to_logical_ptr; /// Methods for read-only array views. impl<'a, A, D> ArrayView<'a, A, D> @@ -57,7 +57,7 @@ where let dim = shape.dim; dimension::can_index_slice_with_strides(xs, &dim, &shape.strides)?; let strides = shape.strides.strides_for_dim(&dim); - unsafe { Ok(Self::new_(xs.as_ptr().offset(-offset_from_ptr_to_memory(&dim, &strides)), dim, strides)) } + unsafe { Ok(Self::new_(xs.as_ptr().add(offset_from_low_addr_ptr_to_logical_ptr(&dim, &strides)), dim, strides)) } } /// Create an `ArrayView` from shape information and a raw pointer to @@ -154,7 +154,7 @@ where let dim = shape.dim; dimension::can_index_slice_with_strides(xs, &dim, &shape.strides)?; let strides = shape.strides.strides_for_dim(&dim); - unsafe { Ok(Self::new_(xs.as_mut_ptr().offset(-offset_from_ptr_to_memory(&dim, &strides)), dim, strides)) } + unsafe { Ok(Self::new_(xs.as_mut_ptr().add(offset_from_low_addr_ptr_to_logical_ptr(&dim, &strides)), dim, strides)) } } /// Create an `ArrayViewMut` from shape information and a