Skip to content

Commit faa7591

Browse files
GregoryComerfacebook-github-bot
authored andcommitted
Validate that method inputs have the expected dim order
Summary: Currently, there is no check for the correct dim order when setting method inputs. If the input tensor has a different dim order than was used in export, it will silently give garbage outputs by treating the input tensor as having a different dim order than it does. To catch this case, I've added an additional check in `method.set_input` to enforce that the dim order matches. I've also added a `dim_order_to_c_string` utility method in "dim_order_util.h" to aid in printing usable error messages. Differential Revision: D72106471
1 parent 7d35c68 commit faa7591

File tree

4 files changed

+121
-0
lines changed

4 files changed

+121
-0
lines changed

runtime/core/exec_aten/util/dim_order_util.h

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,11 +9,13 @@
99
#pragma once
1010

1111
#include <c10/util/irange.h>
12+
#include <array>
1213
#include <cstdint>
1314
#include <cstdio>
1415
#include <cstring>
1516

1617
#include <executorch/runtime/core/error.h>
18+
#include <executorch/runtime/core/exec_aten/util/tensor_dimension_limit.h>
1719
#include <executorch/runtime/platform/assert.h>
1820
#include <executorch/runtime/platform/compiler.h>
1921

@@ -261,6 +263,49 @@ ET_NODISCARD inline Error stride_to_dim_order(
261263
return Error::Ok;
262264
}
263265

266+
/**
267+
* Maximum size of a string returned by dim_order_to_c_string, for
268+
* stack allocation.
269+
*/
270+
constexpr size_t kDimOrderStringSizeLimit = 1 + /* opening parenthesis */
271+
4 * kTensorDimensionLimit + /* maximum size of each printed element,
272+
* including comma and space */
273+
1; /* padding for NULL terminator */
274+
template <typename DimOrderType>
275+
inline std::array<char, kDimOrderStringSizeLimit> dim_order_to_c_string(
276+
const DimOrderType* dim_order,
277+
const size_t dims) {
278+
std::array<char, kDimOrderStringSizeLimit> result = {0};
279+
char* p = result.data();
280+
static_assert(
281+
kDimOrderStringSizeLimit >= 3,
282+
"Invalid value for kDimOrderStringSizeLimit");
283+
size_t remaining_size = kDimOrderStringSizeLimit - 1;
284+
285+
auto chars_written = snprintf(p, remaining_size, "(");
286+
if (chars_written >= remaining_size) {
287+
return result;
288+
}
289+
remaining_size -= chars_written;
290+
p += chars_written;
291+
292+
for (size_t i = 0; i < dims; ++i) {
293+
chars_written = snprintf(
294+
p,
295+
remaining_size,
296+
i != dims - 1 ? "%d, " : "%d",
297+
static_cast<int>(dim_order[i]));
298+
if (chars_written >= remaining_size) {
299+
return result;
300+
}
301+
remaining_size -= chars_written;
302+
p += chars_written;
303+
}
304+
305+
snprintf(p, remaining_size, ")");
306+
return result;
307+
}
308+
264309
} // namespace runtime
265310
} // namespace executorch
266311

runtime/core/exec_aten/util/test/dim_order_util_test.cpp

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414

1515
#include <gtest/gtest.h>
1616

17+
using executorch::runtime::dim_order_to_c_string;
1718
using executorch::runtime::dim_order_to_stride;
1819
using executorch::runtime::Error;
1920
using executorch::runtime::is_channels_last_dim_order;
@@ -286,3 +287,19 @@ TEST(DimOrderUtilTest, IsChannelsLastDimOrderFailCasesTest) {
286287
EXPECT_FALSE(is_channels_last_dim_order(dim_order_4d, 4));
287288
EXPECT_FALSE(is_channels_last_dim_order(dim_order_5d, 5));
288289
}
290+
291+
TEST(DimOrderUtilTest, DimOrderToCStringSimple) {
292+
std::array<executorch::aten::DimOrderType, 4> dim_order = {0, 1, 2, 3};
293+
const auto expected = "(0, 1, 2, 3)";
294+
auto c_str = dim_order_to_c_string(dim_order.data(), dim_order.size());
295+
296+
EXPECT_TRUE(strcmp(c_str.data(), expected) == 0);
297+
}
298+
TEST(DimOrderUtilTest, DimOrderToCStringHandlesOverflow) {
299+
const auto dim_count = 1000;
300+
std::vector<executorch::aten::DimOrderType> dim_order(dim_count);
301+
std::iota(dim_order.begin(), dim_order.end(), 0);
302+
303+
auto c_str = dim_order_to_c_string(dim_order.data(), dim_count);
304+
EXPECT_EQ(c_str[c_str.size() - 1], '\0');
305+
}

runtime/executor/method.cpp

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1032,6 +1032,7 @@ Method::set_input(const EValue& input_evalue, size_t input_idx) {
10321032
const auto& t_dst = e.toTensor();
10331033
const auto& t_src = input_evalue.toTensor();
10341034

1035+
// Validate scalar type and dim order
10351036
ET_CHECK_OR_RETURN_ERROR(
10361037
t_dst.scalar_type() == t_src.scalar_type(),
10371038
InvalidArgument,
@@ -1040,6 +1041,32 @@ Method::set_input(const EValue& input_evalue, size_t input_idx) {
10401041
input_idx,
10411042
executorch::runtime::toString(t_dst.scalar_type()),
10421043
executorch::runtime::toString(t_src.scalar_type()));
1044+
1045+
if (!tensors_have_same_dim_order(t_dst, t_src)) {
1046+
#if ET_LOG_ENABLED
1047+
std::array<exec_aten::DimOrderType, kTensorDimensionLimit> dst_dim_order;
1048+
std::array<exec_aten::DimOrderType, kTensorDimensionLimit> src_dim_order;
1049+
ET_CHECK_OK_OR_RETURN_ERROR(
1050+
get_dim_order(t_dst, dst_dim_order.data(), dst_dim_order.size()));
1051+
ET_CHECK_OK_OR_RETURN_ERROR(
1052+
get_dim_order(t_src, src_dim_order.data(), src_dim_order.size()));
1053+
1054+
auto dst_dim_order_c_str =
1055+
dim_order_to_c_string(dst_dim_order.data(), t_dst.dim());
1056+
auto src_dim_order_c_str =
1057+
dim_order_to_c_string(src_dim_order.data(), t_src.dim());
1058+
1059+
ET_LOG(
1060+
Error,
1061+
"Input %zu has unexpected dim order: expected %s but was %s.",
1062+
input_idx,
1063+
dst_dim_order_c_str.data(),
1064+
src_dim_order_c_str.data());
1065+
1066+
#endif
1067+
return Error::InvalidArgument;
1068+
}
1069+
10431070
// Reset the shape for the Method's input as the size of forwarded input
10441071
// tensor for shape dynamism. Also is a safety check if need memcpy.
10451072
Error err = resize_tensor(t_dst, t_src.sizes());

runtime/executor/test/method_test.cpp

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -339,6 +339,38 @@ TEST_F(MethodTest, ProgramDataSeparationTest) {
339339
ASSERT_EQ(err, Error::Ok);
340340
}
341341

342+
TEST_F(MethodTest, InputDimOrderMismatchTest) {
343+
/*
344+
* Verify the input tensor dim order is checked against the expected value.
345+
*/
346+
347+
ManagedMemoryManager mmm(kDefaultNonConstMemBytes, kDefaultRuntimeMemBytes);
348+
Result<Method> method = programs_["cat"]->load_method("forward", &mmm.get());
349+
ASSERT_EQ(method.error(), Error::Ok);
350+
351+
// Set up io. Input and Output should share the same memory.
352+
constexpr int buffer_size = 16;
353+
float buffer[buffer_size]; // Initial input is (2,4) we then cat a (1,4) to it
354+
// twice for a final shape of (4,4)
355+
for (int i = 0; i < buffer_size; ++i) {
356+
buffer[i] = 0.f;
357+
}
358+
int32_t sizes[2] = {2, 4};
359+
uint8_t dim_order[2] = {1, 0};
360+
int32_t strides[2] = {1, 4};
361+
executorch::aten::TensorImpl impl(
362+
executorch::aten::ScalarType::Float,
363+
2,
364+
sizes,
365+
buffer,
366+
dim_order,
367+
strides);
368+
369+
auto input_err =
370+
method->set_input(EValue(executorch::aten::Tensor(&impl)), 0);
371+
ASSERT_EQ(input_err, Error::InvalidArgument);
372+
}
373+
342374
/*
343375
* TODO(T161163608): Test is disabled due to a resize bug in tensor_index_out of
344376
* the portable op lib

0 commit comments

Comments
 (0)