Skip to content

Preserve caller-provided non-contiguous strides in make_tensor_ptr #19006

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
manuelcandales wants to merge 1 commit into
base: main
Choose a base branch
from
+49 −29
Open

Preserve caller-provided non-contiguous strides in make_tensor_ptr #19006

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 12 additions & 11 deletions extension/tensor/tensor_ptr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,21 +86,22 @@ TensorPtr make_tensor_ptr(
ET_CHECK_MSG(error == runtime::Error::Ok, "Failed to compute strides.");

if (!strides.empty()) {
bool is_contiguous = true;
for (size_t i = 0; i < dim; i++) {
ET_CHECK_MSG(
strides[i] == computed_strides[i] || sizes[i] == 1,
"invalid strides for dim %zu: %" ET_PRI_SIZES_AND_STRIDES
"!= %" ET_PRI_SIZES_AND_STRIDES
" while its size is %" ET_PRI_SIZES_AND_STRIDES " != 1",
i,
strides[i],
computed_strides[i],
sizes[i]);
if (strides[i] != computed_strides[i] && sizes[i] != 1) {
is_contiguous = false;
break;
}
}
if (is_contiguous) {
strides = std::move(computed_strides);
}
Comment on lines 88 to +98
Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This removes the only enforcement that strides are a safe/expected layout. As a result, callers can now pass arbitrary strides (including negative/zero strides, or extremely large strides) which can lead to out-of-bounds accesses or integer overflow when computing element offsets/required storage size downstream. At minimum, add validation for obviously unsafe stride patterns (e.g., non-negative strides for non-broadcast dims if negative strides aren’t supported, and overflow-safe computation of the maximum addressable offset for the view). If the API can’t validate against the actual backing allocation size, consider adding an overload that takes the buffer length in bytes (or an explicit storage descriptor) so from_blob/make_tensor_ptr can enforce that the view is within bounds.

Copilot uses AI. Check for mistakes.
// else: keep the caller-provided non-contiguous strides (e.g. from
// reinterpret_tensor views like chunk/split).
} else {
strides = std::move(computed_strides);
}
Comment on lines 88 to 103
Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this change, make_tensor_ptr can accept dim_order and strides that disagree (i.e., strides not matching dim_order_to_stride even up to size-1 dims). That makes the precedence/contract between dim_order and strides ambiguous and can break any code that assumes dim_order is consistent with the tensor’s stride order. Consider defining an explicit rule, e.g. (mandatory): when caller provides non-empty strides, either (a) derive/normalize dim_order from the provided strides (similar to the existing dim_order.empty() path), or (b) validate that the provided dim_order matches the ordering implied by the provided strides (ignoring size-1 dims) and reject otherwise. Update the tests accordingly (the new TensorMismatchStridesAndDimOrder behavior currently relies on this ambiguity).

Copilot uses AI. Check for mistakes.

strides = std::move(computed_strides);

#ifndef USE_ATEN_LIB
executorch::aten::TensorImpl tensor_impl(
type,
Expand Down
16 changes: 14 additions & 2 deletions extension/tensor/test/tensor_ptr_maker_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,9 +132,21 @@ TEST_F(TensorPtrMakerTest, CreateTensorUsingFromBlobWithLegalStrides) {
EXPECT_EQ(tensor->const_data_ptr<float>()[0], 3);
}

TEST_F(TensorPtrMakerTest, FailedCreateTensorUsingFromBlobWithIllegalStrides) {
TEST_F(TensorPtrMakerTest, CreateTensorUsingFromBlobWithNonContiguousStrides) {
float data[20] = {3};
ET_EXPECT_DEATH(from_blob(data, {2, 2, 2}, {10, 2, 1}), "");
auto tensor = from_blob(data, {2, 2, 2}, {10, 2, 1});

EXPECT_EQ(tensor->dim(), 3);
EXPECT_EQ(tensor->size(0), 2);
EXPECT_EQ(tensor->size(1), 2);
EXPECT_EQ(tensor->size(2), 2);

// Non-contiguous strides are preserved (e.g. from reinterpret_tensor views).
EXPECT_EQ(tensor->strides()[0], 10);
EXPECT_EQ(tensor->strides()[1], 2);
EXPECT_EQ(tensor->strides()[2], 1);
EXPECT_EQ(tensor->const_data_ptr<float>(), data);
EXPECT_EQ(tensor->const_data_ptr<float>()[0], 3);
}

TEST_F(TensorPtrMakerTest, TensorMakerConversionOperator) {
Expand Down
18 changes: 13 additions & 5 deletions extension/tensor/test/tensor_ptr_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -420,14 +420,19 @@ TEST_F(TensorPtrTest, MakeViewReuseMetadataWhenShapeSame) {
EXPECT_EQ(view->strides()[1], 3);
}

TEST_F(TensorPtrTest, MakeViewShapeChangeWithExplicitOldStridesExpectDeath) {
TEST_F(TensorPtrTest, MakeViewShapeChangeWithNonContiguousStrides) {
float data[12] = {0};
auto tensor = make_tensor_ptr({3, 4}, data);
std::vector<executorch::aten::StridesType> old_strides(
tensor->strides().begin(), tensor->strides().end());

ET_EXPECT_DEATH(
{ auto _ = make_tensor_ptr(tensor, {2, 6}, {}, old_strides); }, "");
// Reshaping [3,4] to [2,6] with old strides [4,1] creates a non-contiguous
// view (stride[0]=4 != contiguous 6). This is allowed for reinterpret_tensor.
auto view = make_tensor_ptr(tensor, {2, 6}, {}, old_strides);
EXPECT_EQ(view->size(0), 2);
EXPECT_EQ(view->size(1), 6);
EXPECT_EQ(view->strides()[0], 4);
EXPECT_EQ(view->strides()[1], 1);
}

TEST_F(TensorPtrTest, MakeViewInvalidDimOrderExpectDeath) {
Expand Down Expand Up @@ -967,8 +972,11 @@ TEST_F(TensorPtrTest, TensorDefaultDimOrderAndStrides) {

TEST_F(TensorPtrTest, TensorMismatchStridesAndDimOrder) {
float data[12] = {0};
ET_EXPECT_DEATH(
{ auto _ = make_tensor_ptr({3, 4}, data, {1, 0}, {1, 4}); }, "");
// dim_order={1,0} implies strides={1,3}, but caller provides {1,4}.
// Non-contiguous strides are preserved for reinterpret_tensor views.
Comment on lines +975 to +976
Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test comment frames this as a reinterpret_tensor-style view, but it constructs a fresh tensor from raw {sizes, dim_order, strides} where dim_order and strides disagree. This is a different scenario than preserving strides from a view and can confuse readers about the intended contract. Suggest clarifying the comment to describe the actual case being tested (mismatched dim_order/strides), and/or adjusting the test to use a reinterpret_tensor-like construction path if that’s the intended coverage.

Suggested change
// dim_order={1,0} implies strides={1,3}, but caller provides {1,4}.
// Non-contiguous strides are preserved for reinterpret_tensor views.
// This directly constructs a tensor from explicit sizes, dim_order, and
// strides. dim_order={1,0} would normally imply strides={1,3}, but the
// caller provides mismatched strides {1,4}; verify make_tensor_ptr preserves
// the provided strides rather than inferring them from dim_order.

Copilot uses AI. Check for mistakes.
auto tensor = make_tensor_ptr({3, 4}, data, {1, 0}, {1, 4});
EXPECT_EQ(tensor->strides()[0], 1);
EXPECT_EQ(tensor->strides()[1], 4);
}

TEST_F(TensorPtrTest, TensorCustomDimOrderAndStrides) {
Expand Down
21 changes: 10 additions & 11 deletions extension/wasm/wasm_bindings.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,20 +151,19 @@ void assert_dim_order_and_strides_valid(
THROW_IF_ERROR(error, "Failed to compute strides.");

if (!strides.empty()) {
bool is_contiguous = true;
for (size_t i = 0; i < sizes.size(); i++) {
THROW_IF_FALSE(
strides[i] == computed_strides[i] || sizes[i] == 1,
"invalid strides for dim %zu: %" ET_PRI_SIZES_AND_STRIDES
"!= %" ET_PRI_SIZES_AND_STRIDES
" while its size is %" ET_PRI_SIZES_AND_STRIDES " != 1",
i,
strides[i],
computed_strides[i],
sizes[i]);
if (strides[i] != computed_strides[i] && sizes[i] != 1) {
is_contiguous = false;
break;
}
}
if (is_contiguous) {
strides = std::move(computed_strides);
}
} else {
strides = std::move(computed_strides);
}
Comment on lines 153 to 166
Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This duplicates the updated stride canonicalization logic from tensor_ptr.cpp, which risks behavior drift between WASM and non-WASM paths over time. Consider extracting a shared helper (e.g., a small utility that takes sizes, computed_strides, and optional strides and returns the normalized/preserved strides plus any validation) and using it in both places.

Copilot uses AI. Check for mistakes.

strides = std::move(computed_strides);
}

/**
Expand Down
Loading