gemini-code-assist[bot] commented on code in PR #308:
URL: https://github.com/apache/tvm-ffi/pull/308#discussion_r2586283686
##########
python/tvm_ffi/cython/tvm_ffi_python_helpers.h:
##########
@@ -394,20 +396,47 @@ class TVMFFIPyCallManager {
return -1;
}
}
+
+ /*!
+ * \brief Set an py_arg to out using the __tvm_ffi_value__ protocol.
+ * \param setter_factory The factory function to create the setter
+ * \param ctx The call context
+ * \param py_arg The python argument to be set
+ * \param out The output argument
+ * \return 0 on success, -1 on failure
+ */
+ TVM_FFI_INLINE int SetArgumentFFIValueProtocol(TVMFFIPyArgSetterFactory
setter_factory,
+ TVMFFIPyCallContext* ctx,
PyObject* py_arg,
+ TVMFFIAny* out) {
+ ++ctx->nested_tvm_ffi_value_count;
+ int ret_code = SetArgument(setter_factory, ctx, py_arg, out);
+ --ctx->nested_tvm_ffi_value_count;
+ // retain the static object if the return value only if it is the
outermost nested level
+ // NOTE: we still keep the invariance that each ArgSetter can have at most
one temporary FFI
+ // object so it ensures that we won't overflow the temporary FFI object
stack
+ // The counter mechanism is applied per TVMFFIPyCallContext
+ // so nested call into tuple conversion will be handled in another call
context
+ if (ret_code == 0 && ctx->nested_tvm_ffi_value_count == 0 &&
+ out->type_index >= kTVMFFIStaticObjectBegin) {
+ if (TVMFFIObjectIncRef(out->v_ptr) != 0) {
+ PyErr_SetString(PyExc_RuntimeError, "Failed to retain temp object");
+ return -1;
+ }
+ // TVMFFIPyPushTempFFIObject(ctx, out->v_ptr);
+ // push the temporary FFI object to the call context
+ ctx->temp_ffi_objects[ctx->num_temp_ffi_objects++] = out->v_ptr;
+ return 0;
+ }
+ return ret_code;
+ }
Review Comment:
The manual increment and decrement of `nested_tvm_ffi_value_count` is not
exception-safe. If `SetArgument` throws a C++ exception (which can happen if a
Python exception is raised in a Cython `except -1` function called from C++),
the counter will not be decremented. This can lead to a resource leak and
incorrect behavior in subsequent calls.
To make this robust, I recommend using a RAII guard to manage the counter's
lifetime. This ensures that the counter is always decremented when the function
scope is exited, whether normally or by an exception.
Additionally, the commented-out call to `TVMFFIPyPushTempFFIObject` should
be removed to improve code clarity.
Note that with the RAII guard, the condition for the outermost call check
changes from `ctx->nested_tvm_ffi_value_count == 0` to
`ctx->nested_tvm_ffi_value_count == 1` because the counter is decremented at
the end of the scope.
```c
TVM_FFI_INLINE int SetArgumentFFIValueProtocol(TVMFFIPyArgSetterFactory
setter_factory,
TVMFFIPyCallContext* ctx,
PyObject* py_arg,
TVMFFIAny* out) {
// RAII guard to ensure counter is decremented even if exceptions occur.
class NestedFFIValueGuard {
public:
explicit NestedFFIValueGuard(int* count) : count_(count) {
++(*count_); }
~NestedFFIValueGuard() { --(*count_); }
private:
int* count_;
};
NestedFFIValueGuard guard(&ctx->nested_tvm_ffi_value_count);
int ret_code = SetArgument(setter_factory, ctx, py_arg, out);
// retain the static object if the return value only if it is the
outermost nested level
// NOTE: we still keep the invariance that each ArgSetter can have at
most one temporary FFI
// object so it ensures that we won't overflow the temporary FFI object
stack
// The counter mechanism is applied per TVMFFIPyCallContext
// so nested call into tuple conversion will be handled in another call
context
if (ret_code == 0 && ctx->nested_tvm_ffi_value_count == 1 &&
out->type_index >= kTVMFFIStaticObjectBegin) {
if (TVMFFIObjectIncRef(out->v_ptr) != 0) {
PyErr_SetString(PyExc_RuntimeError, "Failed to retain temp object");
return -1;
}
// push the temporary FFI object to the call context
ctx->temp_ffi_objects[ctx->num_temp_ffi_objects++] = out->v_ptr;
return 0;
}
return ret_code;
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
