[TIR] Fix segfaults from ordering of Let/Assert in MakePackedAPI #16543
Conversation
|
This came about while debugging the implementation of #16542, but is otherwise unrelated. |
|
Merged from main to PR branch, to resolve CI breakage that was fixed in #16546. |
Lunderberg
force-pushed
the
tir_reorder_dlpack_asserts
branch
2 times, most recently
from
a7f822a to
4a064de
Compare
Prior to this commit, the `MakePackedAPI` pass would output steps in the following order: 1. Check the number of arguments. 2. All `LetStmt` produced by the `ArgBinder` 3. `AssertStmt` for the Type code checks for each argument. 4. Additional `AssertStmt` produced by the `ArgBinder`. This order can cause segfaults if a function was provided incorrect arguments. For example, an integer argument passed to a function expecting a `DLTensor*` would be dereferenced to find the tensor's data pointer (step (2)) before checking if it is valid to perform that dereference (step (3)). The same would occur when reading the size of a tensor's axes (step (2)) before checking whether the tensor is the correct dimensionality (step (4)). This commit updates the steps to the following order. 1. Check the number of arguments. 2. Check the type code of each argument. 3. All `LetStmt` and `AssertStmt` produced by the `ArgBinder`, in the order in which they are generated.
Lunderberg
force-pushed
the
tir_reorder_dlpack_asserts
branch
from
4a064de to
e2b7871
Compare
| .trim() | ||
| .split("\n") | ||
| .last() | ||
| .unwrap_or("") |
There was a problem hiding this comment.
Wonder how this came up. Just for readability?
There was a problem hiding this comment.
Oh, that's really weird. I'm guessing it was from bouncing over to the PR branch of #16183, which touched a number of the FFI bindings. I've removed this delta from the PR.
| @@ -186,18 +191,8 @@ void ArgBinder::BindDLTensor(const Buffer& buffer, const PrimExpr& device_type, | |||
| if (!(buffer->dtype == DataType::Int(1) || buffer->dtype == DataType::Int(4) || | |||
| buffer->dtype == DataType::UInt(4))) { | |||
| auto type_msg = tvm::tir::StringImm(type_err_msg.str()); | |||
| asserts_.emplace_back(AssertStmt(a_ndim == v_ndim, msg, nop)); | |||
There was a problem hiding this comment.
Was this just a duplicate?
There was a problem hiding this comment.
Yup. The buffer's dimensionality is checked earlier, so this is entirely a duplicate check on the dimensionality.
|
Last CI failure is due to a flaky hexagon test. I've added a |
Prior to this commit, the
MakePackedAPIpass would output steps in the following order:LetStmtproduced by theArgBinderAssertStmtfor the Type code checks for each argument.AssertStmtproduced by theArgBinder.This order can cause segfaults if a function was provided incorrect arguments. For example, an integer argument passed to a function expecting a
DLTensor*would be dereferenced to find the tensor's data pointer (step (2)) before checking if it is valid to perform that dereference (step (3)). The same would occur when reading the size of a tensor's axes (step (2)) before checking whether the tensor is the correct dimensionality (step (4)).This commit updates the steps to the following order.
LetStmtandAssertStmtproduced by theArgBinder, in the order in which they are generated.