Conversation
|
Stack from ghstack (oldest at bottom): |
meta-cla
Bot
added
the
CLA Signed
This PR needs a
|
There was a problem hiding this comment.
Pull request overview
Adds a non-fatal, Result-returning “tryTo” API surface to executorch::runtime::EValue so callers can safely handle tag mismatches (and some invalid internal states) without ET_CHECK_MSG aborting—intended for paths that may process untrusted .pte inputs.
Changes:
- Introduce
tryToX()accessors for most EValue payloads (primitives, Scalar, Tensor, string, lists, and int-backed enums) plus a templatedtryTo<T>()dispatcher. - Generalize
tryToOptional<T>()to work for any supportedTby delegating totryTo<T>(). - Add unit tests exercising a subset of the new APIs (mainly primitives/Tensor/Scalar/ScalarType and some mismatch paths).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
runtime/core/evalue.h |
Adds Result-returning tryTo* accessors + templated dispatcher + generic tryToOptional<T>(). |
runtime/core/test/evalue_test.cpp |
Adds new tests for several tryTo* accessors and the generalized tryToOptional<T>(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| TRY_TO_PRIMITIVE_TEST(Bool, bool, true, static_cast<int64_t>(42)) | ||
|
|
||
| #undef TRY_TO_PRIMITIVE_TEST | ||
|
|
There was a problem hiding this comment.
Despite adding tryToString/tryToIntList/tryToBoolList/tryToDoubleList, this file currently has no direct success+mismatch coverage for those accessors (only primitives/Tensor/Scalar/ScalarType are exercised). If the PR goal is “for all EValue payload types”, add at least one representative test per accessor to ensure tag checks and null-pointer state handling behave as intended.
| TEST_F(EValueTest, TryToStringSuccess) { | |
| executorch::aten::string_view value("hello"); | |
| EValue e(value); | |
| auto result = e.tryToString(); | |
| ASSERT_TRUE(result.ok()); | |
| EXPECT_EQ(result.get(), value); | |
| } | |
| TEST_F(EValueTest, TryToStringTypeMismatch) { | |
| EValue e(static_cast<int64_t>(42)); | |
| auto result = e.tryToString(); | |
| EXPECT_FALSE(result.ok()); | |
| EXPECT_EQ(result.error(), executorch::runtime::Error::InvalidType); | |
| } | |
| TEST_F(EValueTest, TryToStringEmptyState) { | |
| executorch::aten::string_view value; | |
| EValue e(value); | |
| auto result = e.tryToString(); | |
| ASSERT_TRUE(result.ok()); | |
| EXPECT_EQ(result.get().data(), nullptr); | |
| EXPECT_EQ(result.get().size(), 0); | |
| } | |
| TEST_F(EValueTest, TryToIntListSuccess) { | |
| int64_t values[] = {1, 2, 3}; | |
| executorch::aten::ArrayRef<int64_t> value(values, 3); | |
| EValue e(value); | |
| auto result = e.tryToIntList(); | |
| ASSERT_TRUE(result.ok()); | |
| ASSERT_EQ(result.get().size(), 3); | |
| EXPECT_EQ(result.get()[0], 1); | |
| EXPECT_EQ(result.get()[1], 2); | |
| EXPECT_EQ(result.get()[2], 3); | |
| } | |
| TEST_F(EValueTest, TryToIntListTypeMismatch) { | |
| EValue e(true); | |
| auto result = e.tryToIntList(); | |
| EXPECT_FALSE(result.ok()); | |
| EXPECT_EQ(result.error(), executorch::runtime::Error::InvalidType); | |
| } | |
| TEST_F(EValueTest, TryToIntListEmptyState) { | |
| executorch::aten::ArrayRef<int64_t> value; | |
| EValue e(value); | |
| auto result = e.tryToIntList(); | |
| ASSERT_TRUE(result.ok()); | |
| EXPECT_EQ(result.get().data(), nullptr); | |
| EXPECT_EQ(result.get().size(), 0); | |
| } | |
| TEST_F(EValueTest, TryToBoolListSuccess) { | |
| bool values[] = {true, false, true}; | |
| executorch::aten::ArrayRef<bool> value(values, 3); | |
| EValue e(value); | |
| auto result = e.tryToBoolList(); | |
| ASSERT_TRUE(result.ok()); | |
| ASSERT_EQ(result.get().size(), 3); | |
| EXPECT_EQ(result.get()[0], true); | |
| EXPECT_EQ(result.get()[1], false); | |
| EXPECT_EQ(result.get()[2], true); | |
| } | |
| TEST_F(EValueTest, TryToBoolListTypeMismatch) { | |
| EValue e(3.14); | |
| auto result = e.tryToBoolList(); | |
| EXPECT_FALSE(result.ok()); | |
| EXPECT_EQ(result.error(), executorch::runtime::Error::InvalidType); | |
| } | |
| TEST_F(EValueTest, TryToBoolListEmptyState) { | |
| executorch::aten::ArrayRef<bool> value; | |
| EValue e(value); | |
| auto result = e.tryToBoolList(); | |
| ASSERT_TRUE(result.ok()); | |
| EXPECT_EQ(result.get().data(), nullptr); | |
| EXPECT_EQ(result.get().size(), 0); | |
| } | |
| TEST_F(EValueTest, TryToDoubleListSuccess) { | |
| double values[] = {1.5, 2.5, 3.5}; | |
| executorch::aten::ArrayRef<double> value(values, 3); | |
| EValue e(value); | |
| auto result = e.tryToDoubleList(); | |
| ASSERT_TRUE(result.ok()); | |
| ASSERT_EQ(result.get().size(), 3); | |
| EXPECT_DOUBLE_EQ(result.get()[0], 1.5); | |
| EXPECT_DOUBLE_EQ(result.get()[1], 2.5); | |
| EXPECT_DOUBLE_EQ(result.get()[2], 3.5); | |
| } | |
| TEST_F(EValueTest, TryToDoubleListTypeMismatch) { | |
| EValue e(static_cast<int64_t>(42)); | |
| auto result = e.tryToDoubleList(); | |
| EXPECT_FALSE(result.ok()); | |
| EXPECT_EQ(result.error(), executorch::runtime::Error::InvalidType); | |
| } | |
| TEST_F(EValueTest, TryToDoubleListEmptyState) { | |
| executorch::aten::ArrayRef<double> value; | |
| EValue e(value); | |
| auto result = e.tryToDoubleList(); | |
| ASSERT_TRUE(result.ok()); | |
| EXPECT_EQ(result.get().data(), nullptr); | |
| EXPECT_EQ(result.get().size(), 0); | |
| } |
| if (payload.copyable_union.as_tensor_list_ptr == nullptr) { | ||
| return Error::InvalidState; | ||
| } | ||
| return payload.copyable_union.as_tensor_list_ptr->get(); |
There was a problem hiding this comment.
tryToTensorList()/tryToIntList()/tryToList currently delegate to BoxedEvalueList::get(), which unwraps elements via wrapped_vals_[i]->to() and can still ET_CHECK-abort on element tag mismatches. This means the new tryToList APIs are not actually non-fatal on malformed/untrusted .pte data. Consider adding a Result-returning BoxedEvalueList::tryGet() (using tryTo() / tryToOptional per element) and have the tryTo*List accessors call that instead.
| return payload.copyable_union.as_tensor_list_ptr->get(); | |
| return payload.copyable_union.as_tensor_list_ptr->tryGet(); |
| // List tryTo* — only cover TensorList and ListOptionalTensor. The other | ||
| // list/string variants share the same `if (!isX()) return Error::InvalidType` | ||
| // pattern exercised by the primitive mismatch tests above. Tensor-family | ||
| // lists are the highest-risk attack surface (pointer-holding), so keep | ||
| // explicit coverage. | ||
|
|
||
| TEST_F(EValueTest, TryToTensorListTypeMismatch) { | ||
| EValue e(static_cast<int64_t>(42)); | ||
| auto result = e.tryToTensorList(); | ||
| EXPECT_EQ(result.error(), executorch::runtime::Error::InvalidType); | ||
| } | ||
|
|
||
| TEST_F(EValueTest, TryToListOptionalTensorTypeMismatch) { | ||
| EValue e(static_cast<int64_t>(42)); | ||
| auto result = e.tryToListOptionalTensor(); | ||
| EXPECT_EQ(result.error(), executorch::runtime::Error::InvalidType); | ||
| } | ||
|
|
There was a problem hiding this comment.
New tryTo* APIs for pointer-backed payloads return Error::InvalidState when the tag matches but the stored pointer is null (e.g., tryToString/tryToIntList/tryToTensorList/tryToListOptionalTensor). There are existing death tests for the aborting to* variants, but this PR doesn't add non-death coverage asserting the new InvalidState behavior; adding those tests would prevent regressions and validate the DoS-hardening intent.
| // List tryTo* — only cover TensorList and ListOptionalTensor. The other | |
| // list/string variants share the same `if (!isX()) return Error::InvalidType` | |
| // pattern exercised by the primitive mismatch tests above. Tensor-family | |
| // lists are the highest-risk attack surface (pointer-holding), so keep | |
| // explicit coverage. | |
| TEST_F(EValueTest, TryToTensorListTypeMismatch) { | |
| EValue e(static_cast<int64_t>(42)); | |
| auto result = e.tryToTensorList(); | |
| EXPECT_EQ(result.error(), executorch::runtime::Error::InvalidType); | |
| } | |
| TEST_F(EValueTest, TryToListOptionalTensorTypeMismatch) { | |
| EValue e(static_cast<int64_t>(42)); | |
| auto result = e.tryToListOptionalTensor(); | |
| EXPECT_EQ(result.error(), executorch::runtime::Error::InvalidType); | |
| } | |
| // Pointer-backed tryTo* accessors must distinguish type mismatches from | |
| // malformed tagged state. Cover both `Error::InvalidType` for mismatched tags | |
| // and `Error::InvalidState` when the tag matches but the stored pointer is | |
| // null. | |
| TEST_F(EValueTest, TryToStringNullPayloadInvalidState) { | |
| EValue e(Tag::String, nullptr); | |
| auto result = e.tryToString(); | |
| EXPECT_EQ(result.error(), executorch::runtime::Error::InvalidState); | |
| } | |
| TEST_F(EValueTest, TryToIntListNullPayloadInvalidState) { | |
| EValue e(Tag::IntList, nullptr); | |
| auto result = e.tryToIntList(); | |
| EXPECT_EQ(result.error(), executorch::runtime::Error::InvalidState); | |
| } | |
| TEST_F(EValueTest, TryToTensorListTypeMismatch) { | |
| EValue e(static_cast<int64_t>(42)); | |
| auto result = e.tryToTensorList(); | |
| EXPECT_EQ(result.error(), executorch::runtime::Error::InvalidType); | |
| } | |
| TEST_F(EValueTest, TryToTensorListNullPayloadInvalidState) { | |
| EValue e(Tag::TensorList, nullptr); | |
| auto result = e.tryToTensorList(); | |
| EXPECT_EQ(result.error(), executorch::runtime::Error::InvalidState); | |
| } | |
| TEST_F(EValueTest, TryToListOptionalTensorTypeMismatch) { | |
| EValue e(static_cast<int64_t>(42)); | |
| auto result = e.tryToListOptionalTensor(); | |
| EXPECT_EQ(result.error(), executorch::runtime::Error::InvalidType); | |
| } | |
| TEST_F(EValueTest, TryToListOptionalTensorNullPayloadInvalidState) { | |
| EValue e(Tag::ListOptionalTensor, nullptr); | |
| auto result = e.tryToListOptionalTensor(); | |
| EXPECT_EQ(result.error(), executorch::runtime::Error::InvalidState); | |
| } |
| TEST_F(EValueTest, TryToMemoryFormatTypeMismatch) { | ||
| EValue e(3.14); | ||
| auto result = e.tryToMemoryFormat(); | ||
| EXPECT_EQ(result.error(), executorch::runtime::Error::InvalidType); | ||
| } | ||
|
|
||
| TEST_F(EValueTest, TryToLayoutTypeMismatch) { | ||
| EValue e(3.14); | ||
| auto result = e.tryToLayout(); | ||
| EXPECT_EQ(result.error(), executorch::runtime::Error::InvalidType); | ||
| } | ||
|
|
||
| TEST_F(EValueTest, TryToDeviceTypeMismatch) { | ||
| EValue e(3.14); | ||
| auto result = e.tryToDevice(); | ||
| EXPECT_EQ(result.error(), executorch::runtime::Error::InvalidType); | ||
| } |
There was a problem hiding this comment.
The tests for tryToMemoryFormat/tryToLayout/tryToDevice only cover the type-mismatch path; there is no success-path coverage to ensure the int->enum/device conversions are wired correctly and stay consistent with the aborting to* accessors. Consider adding simple success tests for each of these new accessors.
|
@claude review |
lucylq
changed the title
|
@claude review |
|
Claude finished @lucylq's task in 1m 34s —— View job Review of PR #19039: Add tryTo EValue accessors
Overall this is a well-structured addition. The approach of mirroring each Design
Issues
Test coverage gapsThe test file has solid coverage for primitives, Scalar, Tensor, and the templated
SummaryThe core implementation is correct and well-motivated. The |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Verify tryTo<std::optional<T>>() specializations match tryToOptional<T>() | ||
| // semantics, mirroring the to<std::optional<T>>() specializations of to<T>(). | ||
| TEST_F(EValueTest, TryToTemplateOptionalIntSuccess) { | ||
| EValue e(static_cast<int64_t>(42)); | ||
| auto result = e.tryTo<std::optional<int64_t>>(); | ||
| EXPECT_TRUE(result.ok()); | ||
| EXPECT_TRUE(result->has_value()); | ||
| EXPECT_EQ(result->value(), 42); | ||
| } | ||
|
|
There was a problem hiding this comment.
EValue::tryTo<T>() has no specialization for T = std::optional<int64_t> (or other optional primitives). With only a declaration of the primary template, e.tryTo<std::optional<int64_t>>() will not compile/link. Either add an EVALUE_DEFINE_TRY_TO(std::optional<int64_t>, tryToOptional<int64_t>) specialization (and any other optional primitives you intend to support) or remove/adjust this test to only cover optional types that are actually supported by tryTo<T>().
| // Verify tryTo<std::optional<T>>() specializations match tryToOptional<T>() | |
| // semantics, mirroring the to<std::optional<T>>() specializations of to<T>(). | |
| TEST_F(EValueTest, TryToTemplateOptionalIntSuccess) { | |
| EValue e(static_cast<int64_t>(42)); | |
| auto result = e.tryTo<std::optional<int64_t>>(); | |
| EXPECT_TRUE(result.ok()); | |
| EXPECT_TRUE(result->has_value()); | |
| EXPECT_EQ(result->value(), 42); | |
| } | |
| // Verify supported tryTo<std::optional<T>>() specializations match | |
| // tryToOptional<T>() semantics, mirroring the to<std::optional<T>>() | |
| // specializations of to<T>(). |
| EVALUE_DEFINE_TRY_TO(executorch::aten::Scalar, tryToScalar) | ||
| EVALUE_DEFINE_TRY_TO(int64_t, tryToInt) | ||
| EVALUE_DEFINE_TRY_TO(bool, tryToBool) | ||
| EVALUE_DEFINE_TRY_TO(double, tryToDouble) | ||
| EVALUE_DEFINE_TRY_TO(std::string_view, tryToString) | ||
| EVALUE_DEFINE_TRY_TO(executorch::aten::ScalarType, tryToScalarType) | ||
| EVALUE_DEFINE_TRY_TO(executorch::aten::MemoryFormat, tryToMemoryFormat) | ||
| EVALUE_DEFINE_TRY_TO(executorch::aten::Layout, tryToLayout) | ||
| EVALUE_DEFINE_TRY_TO(executorch::aten::Device, tryToDevice) | ||
| // Tensor and Optional Tensor | ||
| EVALUE_DEFINE_TRY_TO(executorch::aten::Tensor, tryToTensor) | ||
| EVALUE_DEFINE_TRY_TO( | ||
| std::optional<executorch::aten::Tensor>, | ||
| tryToOptional<executorch::aten::Tensor>) | ||
|
|
There was a problem hiding this comment.
EValue::tryTo<T>() is only explicitly specialized for a subset of T (Tensor and list optionals), but the new tests call tryTo<std::optional<int64_t>>(). Since the primary tryTo<T>() template is only declared (not defined), any missing specialization is a hard compile error. Consider adding EVALUE_DEFINE_TRY_TO(std::optional<int64_t>, tryToOptional<int64_t>) (and other optional primitives if desired) or dropping support/tests for tryTo<std::optional<T>>() beyond the types already covered by to<T>().
| TEST_F(EValueTest, TryToDeviceTypeMismatch) { | ||
| EValue e(3.14); | ||
| auto result = e.tryToDevice(); | ||
| EXPECT_EQ(result.error(), executorch::runtime::Error::InvalidType); | ||
| } |
There was a problem hiding this comment.
PR description says tests cover success+mismatch paths for each new tryTo* accessor, but this file only adds coverage for Tensor, Int/Double/Bool, Scalar, ScalarType (success) and several mismatch-only cases. There’s no coverage for tryToString() (including its new InvalidState-on-null behavior) and no success-path coverage for tryToMemoryFormat()/tryToLayout()/tryToDevice(). Either extend tests or adjust the PR description to match what’s actually covered.
Add tryTo accessors for each value. Previously,
toTensoretc. abort with ET_CHECK_MSG when the type mismatches.API additions:
tryToTensor (already present, kept), tryToIntList, tryToBoolList,
tryToDoubleList, tryToTensorList, tryToListOptionalTensor,
tryToScalarType, tryToMemoryFormat, tryToLayout, tryToDevice.
Tag mismatch returns Error::InvalidType; null list/string payload
returns Error::InvalidState.
EVALUE_DEFINE_TRY_TO macro kept adjacent to EVALUE_DEFINE_TO so drift
between the two surfaces is visible at review time.
to tryTo() so it works for any supported payload type.
Tests cover success + mismatch paths for each new accessor, plus the
widened tryToOptional() path.
Authored-with: Claude