gemini-code-assist[bot] commented on code in PR #287: URL: https://github.com/apache/tvm-ffi/pull/287#discussion_r2562027537
########## include/tvm/ffi/cuda/device_guard.h: ########## @@ -0,0 +1,73 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +/*! + * \file tvm/ffi/cuda/device_guard.h + * \brief Device guard structs. + */ +#ifndef TVM_FFI_CUDA_DEVICE_GUARD_H_ +#define TVM_FFI_CUDA_DEVICE_GUARD_H_ + +#include <cuda_runtime_api.h> +#include <tvm/ffi/error.h> + +namespace tvm { +namespace ffi { + +/*! + * \brief CUDA Device Guard. + * + * Example usage: + * \code + * void kernel(ffi::TensorView x) { + * ffi::CUDADeviceGuard guard(x.device().device_id); + * ... + * } + * \endcode + */ +struct CUDADeviceGuard { + CUDADeviceGuard() = delete; + /*! + * \brief Constructor from a device index, and backup the current device index. + * \param device_index The device index to guard. + */ + explicit CUDADeviceGuard(int device_index) { + cudaError_t err; + err = cudaGetDevice(&previous_device_index); + TVM_FFI_ICHECK_EQ(err, cudaSuccess) << "cudaGetDevice failed: " << cudaGetErrorString(err); + if (previous_device_index != device_index) { + err = cudaSetDevice(device_index); + TVM_FFI_ICHECK_EQ(err, cudaSuccess) << "cudaSetDevice failed: " << cudaGetErrorString(err); + } + } + + /*! + * \brief Destructor to set the current device index back to backup one. + */ + ~CUDADeviceGuard() { + cudaError_t err = cudaSetDevice(previous_device_index); + TVM_FFI_ICHECK_EQ(err, cudaSuccess) << "cudaSetDevice failed: " << cudaGetErrorString(err); + } Review Comment:  The destructor of `CUDADeviceGuard` can throw an exception if `cudaSetDevice` fails. Throwing exceptions from destructors is dangerous and can lead to program termination by calling `std::terminate`, especially if another exception is already being propagated during stack unwinding. Destructors are implicitly `noexcept` in C++11 and later. Throwing from a `noexcept` function will cause `std::terminate` to be called immediately. It's better to log the error without throwing. Since there isn't a readily available logging utility in this context, using `std::cerr` is a reasonable fallback. ```c ~CUDADeviceGuard() noexcept { cudaError_t err = cudaSetDevice(previous_device_index); if (err != cudaSuccess) { // Cannot throw from a destructor. // Log to stderr instead. std::cerr << "CUDADeviceGuard: failed to restore device " << previous_device_index << ": " << cudaGetErrorString(err) << std::endl; } } ``` ########## include/tvm/ffi/cuda/device_guard.h: ########## @@ -0,0 +1,73 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +/*! + * \file tvm/ffi/cuda/device_guard.h + * \brief Device guard structs. + */ +#ifndef TVM_FFI_CUDA_DEVICE_GUARD_H_ +#define TVM_FFI_CUDA_DEVICE_GUARD_H_ + +#include <cuda_runtime_api.h> +#include <tvm/ffi/error.h> + +namespace tvm { +namespace ffi { + +/*! + * \brief CUDA Device Guard. + * + * Example usage: + * \code + * void kernel(ffi::TensorView x) { + * ffi::CUDADeviceGuard guard(x.device().device_id); + * ... + * } + * \endcode + */ +struct CUDADeviceGuard { + CUDADeviceGuard() = delete; + /*! + * \brief Constructor from a device index, and backup the current device index. + * \param device_index The device index to guard. + */ + explicit CUDADeviceGuard(int device_index) { + cudaError_t err; + err = cudaGetDevice(&previous_device_index); + TVM_FFI_ICHECK_EQ(err, cudaSuccess) << "cudaGetDevice failed: " << cudaGetErrorString(err); + if (previous_device_index != device_index) { + err = cudaSetDevice(device_index); + TVM_FFI_ICHECK_EQ(err, cudaSuccess) << "cudaSetDevice failed: " << cudaGetErrorString(err); + } + } Review Comment:  The constructor for `CUDADeviceGuard` does not validate the `device_index`. A negative `device_index` could be passed. For example, `cudaSetDevice(-1)` is a valid call that detaches the current thread from any CUDA context, which is likely not the intended behavior for this guard. It would be safer to add a check to ensure `device_index` is non-negative. ```suggestion explicit CUDADeviceGuard(int device_index) { TVM_FFI_ICHECK_GE(device_index, 0) << "Device index must be non-negative, but got " << device_index; cudaError_t err; err = cudaGetDevice(&previous_device_index); TVM_FFI_ICHECK_EQ(err, cudaSuccess) << "cudaGetDevice failed: " << cudaGetErrorString(err); if (previous_device_index != device_index) { err = cudaSetDevice(device_index); TVM_FFI_ICHECK_EQ(err, cudaSuccess) << "cudaSetDevice failed: " << cudaGetErrorString(err); } } ``` -- 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]
