gemini-code-assist[bot] commented on code in PR #19778:
URL: https://github.com/apache/tvm/pull/19778#discussion_r3411715454
##########
apps/cpp_rpc/rpc_env.cc:
##########
@@ -199,7 +177,11 @@ std::vector<std::string> ListDir(const std::string&
dirname) {
DIR* dp = opendir(dirname.c_str());
if (dp == nullptr) {
int errsv = errno;
- TVM_FFI_THROW(InternalError) << "ListDir " << dirname << " error: " <<
strerror(errsv);
+ if (errsv == ENOENT) {
+ return vec;
+ }
+ TVM_FFI_THROW(InternalError) << "ListDir " << dirname << " error: "
+ << (strerror(errsv), _exit(EXIT_FAILURE), "");
Review Comment:
Calling `_exit(EXIT_FAILURE)` inside the stream insertion of `TVM_FFI_THROW`
is highly problematic for several reasons:\n\n1. **No Error Logging**: Because
`_exit` is evaluated during the stream construction, the process terminates
immediately before the error message is fully constructed, logged, or thrown.
This makes debugging extremely difficult as no error message will be
printed.\n2. **Side Effects in Utility Functions**: `ListDir` is a general
utility function. Hardcoding process termination inside it prevents other
callers from catching and handling the
exception.\n\n**Recommendation**:\nRemove `_exit` from `ListDir` and instead
wrap the worker process execution (e.g., in `ServerLoopProc` inside
`rpc_server.cc`) in a `try-catch` block to handle exceptions and call `_exit`
there.
```c
TVM_FFI_THROW(InternalError) << "ListDir " << dirname << " error: " <<
strerror(errsv);
```
##########
apps/cpp_rpc/rpc_server.cc:
##########
@@ -205,10 +205,12 @@ class RPCServer {
// Logging.
if (finished_first == timer_pid) {
- LOG(INFO) << "Child pid=" << worker_pid << " killed (timeout = " <<
timeout
- << "), Process status = " << status_second;
+ LOG(INFO) << "Child pid=" << worker_pid << " killed"
+ << " (timeout = " << timeout << " sec)"
+ << ", status = " << status_second;
} else if (finished_first == worker_pid) {
- LOG(INFO) << "Child pid=" << timer_pid << " killed, Process status =
" << status_second;
+ LOG(INFO) << "Child pid=" << timer_pid << " finished"
+ << ", status = "<< status_first;
}
Review Comment:
There is a bug in the logging logic here. When `finished_first ==
worker_pid`, it means the worker process finished successfully, and its exit
status is stored in `status_first`. However, the log message incorrectly prints
`timer_pid` as the finished process while associating it with
`status_first`.\n\nIt should log `worker_pid` (or `finished_first`) instead of
`timer_pid`.
```c
} else if (finished_first == worker_pid) {\n LOG(INFO) <<
"Child pid=" << worker_pid << " finished"\n << ", status = "
<< status_first;\n }
```
##########
apps/cpp_rpc/rpc_env.cc:
##########
@@ -220,7 +202,11 @@ std::vector<std::string> ListDir(const std::string&
dirname) {
HANDLE handle = FindFirstFileA(pattern.c_str(), &fd);
if (handle == INVALID_HANDLE_VALUE) {
const int errsv = GetLastError();
- TVM_FFI_THROW(InternalError) << "ListDir " << dirname << " error: " <<
strerror(errsv);
+ if (errsv == ERROR_FILE_NOT_FOUND || errsv == ERROR_PATH_NOT_FOUND) {
+ return vec;
+ }
+ TVM_FFI_THROW(InternalError) << "ListDir " << dirname << " error: "
+ << (strerror(errsv), _exit(EXIT_FAILURE), "");
Review Comment:
Calling `_exit(EXIT_FAILURE)` inside the stream insertion of `TVM_FFI_THROW`
is highly problematic for several reasons:\n\n1. **No Error Logging**: Because
`_exit` is evaluated during the stream construction, the process terminates
immediately before the error message is fully constructed, logged, or thrown.
This makes debugging extremely difficult as no error message will be
printed.\n2. **Side Effects in Utility Functions**: `ListDir` is a general
utility function. Hardcoding process termination inside it prevents other
callers from catching and handling the
exception.\n\n**Recommendation**:\nRemove `_exit` from `ListDir` and instead
wrap the worker process execution (e.g., in `ServerLoopProc` inside
`rpc_server.cc`) in a `try-catch` block to handle exceptions and call `_exit`
there.
```c
TVM_FFI_THROW(InternalError) << "ListDir " << dirname << " error: " <<
strerror(errsv);
```
--
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]
