tvm icon indicating copy to clipboard operation
tvm copied to clipboard

[Runtime][Bugfix] Added type-checking for Array::insert

Open Lunderberg opened this issue 3 years ago • 5 comments
trafficstars

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.

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.

cc @areusch

Lunderberg avatar Sep 02 '22 18:09 Lunderberg

Breakage is in the WASM build, as the EMCC flags were still using -std=c++14. Fixed in https://github.com/apache/tvm/pull/12693, will re-run CI after it lands.

Lunderberg avatar Sep 02 '22 19:09 Lunderberg

Breakage is in the iOS build, as the clang flags were still using -std=gnu++14. Fixed in https://github.com/apache/tvm/pull/12712, will re-run CI after it lands.

Lunderberg avatar Sep 06 '22 13:09 Lunderberg

we probably should resolve https://discuss.tvm.apache.org/t/downgrade-tvm-runtime-to-c-11/13492/5 before merging this

areusch avatar Sep 07 '22 20:09 areusch

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.

Lunderberg avatar Sep 08 '22 17:09 Lunderberg

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.

Lunderberg avatar Sep 19 '22 15:09 Lunderberg

Rebased onto main, as https://github.com/apache/tvm/pull/12692 also needed and implemented the same type-checking structs, which makes this PR much smaller.

Lunderberg avatar Sep 22 '22 16:09 Lunderberg