[Runtime][Bugfix] Added type-checking for Array::insert #12691
Conversation
|
Breakage is in the WASM build, as the EMCC flags were still using |
|
Breakage is in the iOS build, as the clang flags were still using |
|
we probably should resolve https://discuss.tvm.apache.org/t/downgrade-tvm-runtime-to-c-11/13492/5 before merging this |
|
Sounds reasonable, and I've added a comment weighing in on that thread. If it becomes a sticking point on this PR, I can rewrite to avoid C++17, but C++17 has enough benefits that it would be disappointing to revert entirely. |
|
From the discussion, it sounds like C++17 shouldn't be a blocker anymore, since there are no specific use cases at the moment, and multiple options to resolve it if/when they occur. |
Prior to this commit, the following code would compile and run without
error. This occurs because the typed `Array<T>::insert` calls the
untyped `ArrayNode::InitRange`, with no type-checking done before the
call.
```c++
Var x("x");
Var y("y");
Array<Var> var_arr{x, y};
Array<PrimExpr> expr_arr{x + 1, y + 2};
// Erroneously inserts static-type PrimExpr, runtime-type Add, even
// though neither PrimExpr is a type of Var.
var_arr.insert(var_arr.begin(), expr_arr.begin(), expr_arr.end());
```
After this commit, a `static_assert` in `Array<T>::insert` and in
`Array<T>::Array(IterType,IterTYpe)` restricts the iterators, such
that they must dereference to `T`, `Optional<T>`, a subclass of `T`,
or `Optional<U>` where `U` is a subclass of `T`.
The public method `ArrayNode::SetItem` exposes a similar issue. In
the future, we may want to make it be private, accessed only through
type-safe method in `Array<T>::Set`.
Lunderberg
force-pushed
the
array_type_safety
branch
from
aace765 to
6834e79
Compare
|
Rebased onto main, as #12692 also needed and implemented the same type-checking structs, which makes this PR much smaller. |
| Array<IterVar>& leaf_vars = self->leaf_iter_vars; | ||
| Array<IterVar>& all_vars = self->all_iter_vars; | ||
| std::vector<ObjectRef> temp; | ||
| std::vector<IterVar> temp; |
There was a problem hiding this comment.
this change seems unrelated?
There was a problem hiding this comment.
This was actually the exact type of error that I wanted to catch with the static_assert statements. This was a std::vector<ObjectRef> which was copied into an Array<IterVar>, with no type-checking during the conversion. It didn't cause any runtime issues, because the std::vector was originally populated with ObjectRef instances that contain IterVarNode, but there were no compile-time checks to ensure that was the case. Had it been otherwise, the later use of ObjectRef::DowncastNoCheck<IterVar> would have invoked undefined behavior.
There was a problem hiding this comment.
oh i see, the diff hid line 207 by default, but now i see that temp gets placed into leaf_vars. that makes much more sense :)
* [TVM] Update Submodule * Compatible apache/tvm#12691 Co-authored-by: SubmoduleUpdaterBot <submodule-updater-bot@users.noreply.github.com> Co-authored-by: Cody Yu <comaniac0422@gmail.com>
Prior to this commit, the following code would compile and run without
error. This occurs because the typed `Array<T>::insert` calls the
untyped `ArrayNode::InitRange`, with no type-checking done before the
call.
```c++
Var x("x");
Var y("y");
Array<Var> var_arr{x, y};
Array<PrimExpr> expr_arr{x + 1, y + 2};
// Erroneously inserts static-type PrimExpr, runtime-type Add, even
// though neither PrimExpr is a type of Var.
var_arr.insert(var_arr.begin(), expr_arr.begin(), expr_arr.end());
```
After this commit, a `static_assert` in `Array<T>::insert` and in
`Array<T>::Array(IterType,IterTYpe)` restricts the iterators, such
that they must dereference to `T`, `Optional<T>`, a subclass of `T`,
or `Optional<U>` where `U` is a subclass of `T`.
The public method `ArrayNode::SetItem` exposes a similar issue. In
the future, we may want to make it be private, accessed only through
type-safe method in `Array<T>::Set`.
2 participants
Prior to this commit, the following code would compile and run without error. This occurs because the typed
Array<T>::insertcalls the untypedArrayNode::InitRange, with no type-checking done before the call.After this commit, a
static_assertinArray<T>::insertand inArray<T>::Array(IterType,IterTYpe)restricts the iterators, such that they must dereference toT,Optional<T>, a subclass ofT, orOptional<U>whereUis a subclass ofT.The public method
ArrayNode::SetItemexposes a similar issue. In the future, we may want to make it be private, accessed only through type-safe method inArray<T>::Set.cc @areusch