From 1b57799ad62f79f83ac3a7f6e3ba4bd7fc08a113 Mon Sep 17 00:00:00 2001 From: Scott McKay Date: Tue, 16 Apr 2024 13:06:48 +1000 Subject: [PATCH 1/5] Add helper to get the errno message. --- onnxruntime/core/platform/env.cc | 25 +++++++++++++ onnxruntime/core/platform/env.h | 7 ++++ onnxruntime/core/platform/posix/env.cc | 47 +++++------------------- onnxruntime/core/platform/windows/env.cc | 7 ++-- onnxruntime/test/platform/env_test.cc | 8 ++++ 5 files changed, 53 insertions(+), 41 deletions(-) diff --git a/onnxruntime/core/platform/env.cc b/onnxruntime/core/platform/env.cc index 5ebda75b7c536..12787c70544b7 100644 --- a/onnxruntime/core/platform/env.cc +++ b/onnxruntime/core/platform/env.cc @@ -34,4 +34,29 @@ std::ostream& operator<<(std::ostream& os, gsl::span af Env::Env() = default; +std::pair GetErrnoInfo() { + auto err = errno; + std::string msg; + + if (err != 0) { + char buf[512]; + +#if defined(_WIN32) + auto ret = strerror_s(buf, sizeof(buf), err); + msg = ret == 0 ? buf : "Failed to get error message"; // buf is guaranteed to be null terminated by strerror_s +#else + // strerror_r return type differs by platform. + auto ret = strerror_r(err, buf, buflen); + if (std::is_same_v) { // POSIX returns int + msg = ret == 0 ? buf : "Failed to get error message"; + } else { + // GNU returns char* + msg = ret; + } +#endif + } + + return {err, msg}; +} + } // namespace onnxruntime diff --git a/onnxruntime/core/platform/env.h b/onnxruntime/core/platform/env.h index 0425b2972f872..fe72cf7ffd0a8 100644 --- a/onnxruntime/core/platform/env.h +++ b/onnxruntime/core/platform/env.h @@ -96,6 +96,13 @@ struct ThreadOptions { std::ostream& operator<<(std::ostream& os, const LogicalProcessors&); std::ostream& operator<<(std::ostream& os, gsl::span); +/// +/// Get errno and the corresponding error message. +/// +/// errno and the error message string if errno indicates an error. +std::pair GetErrnoInfo(); + + /// \brief An interface used by the onnxruntime implementation to /// access operating system functionality like the filesystem etc. /// diff --git a/onnxruntime/core/platform/posix/env.cc b/onnxruntime/core/platform/posix/env.cc index 7cd81d89d7d4d..a6699c48a5e13 100644 --- a/onnxruntime/core/platform/posix/env.cc +++ b/onnxruntime/core/platform/posix/env.cc @@ -62,39 +62,11 @@ class UnmapFileParam { size_t len; }; -/** - * @brief Get System Error - * - * @return a pair of {errno, error message} - */ -static std::pair GetSystemError(int e) { - char buf[1024]; - const char* msg = ""; - if (e > 0) { -#if defined(__GLIBC__) && defined(_GNU_SOURCE) && !defined(__ANDROID__) - msg = strerror_r(e, buf, sizeof(buf)); -#else - // for Mac OS X and Android lower than API 23 - if (strerror_r(e, buf, sizeof(buf)) != 0) { - buf[0] = '\0'; - } - msg = buf; -#endif - } - - return std::make_pair(e, msg); -} - -static std::pair GetSystemError() { - auto e = errno; - return GetSystemError(e); -} - static void UnmapFile(void* param) noexcept { std::unique_ptr p(reinterpret_cast(param)); int ret = munmap(p->addr, p->len); if (ret != 0) { - auto [err_no, err_msg] = GetSystemError(); + auto [err_no, err_msg] = GetErrnoInfo(); LOGS_DEFAULT(ERROR) << "munmap failed. error code: " << err_no << " error msg: " << err_msg; } } @@ -104,8 +76,9 @@ struct FileDescriptorTraits { static Handle GetInvalidHandleValue() { return -1; } static void CleanUp(Handle h) { if (close(h) == -1) { - auto [err_no, err_msg] = GetSystemError(); - LOGS_DEFAULT(ERROR) << "Failed to close file descriptor " << h << " - error code: " << err_no << " error msg: " << err_msg; + auto [err_no, err_msg] = GetErrnoInfo(); + LOGS_DEFAULT(ERROR) << "Failed to close file descriptor " << h << " - error code: " << err_no + << " error msg: " << err_msg; } } }; @@ -131,7 +104,7 @@ int nftw_remove( int /*typeflag*/, struct FTW* /*ftwbuf*/) { const auto result = remove(fpath); if (result != 0) { - auto [err_no, err_msg] = GetSystemError(); + auto [err_no, err_msg] = GetErrnoInfo(); LOGS_DEFAULT(WARNING) << "remove() failed. Error code: " << err_no << " error msg: " << err_msg << ", path: " << fpath; } @@ -188,7 +161,7 @@ class PosixThread : public EnvThread { pthread_attr_t attr; int s = pthread_attr_init(&attr); if (s != 0) { - auto [err_no, err_msg] = GetSystemError(); + auto [err_no, err_msg] = GetErrnoInfo(); ORT_THROW("pthread_attr_init failed, error code: ", err_no, " error msg: ", err_msg); } @@ -196,14 +169,14 @@ class PosixThread : public EnvThread { if (stack_size > 0) { s = pthread_attr_setstacksize(&attr, stack_size); if (s != 0) { - auto [err_no, err_msg] = GetSystemError(); + auto [err_no, err_msg] = GetErrnoInfo(); ORT_THROW("pthread_attr_setstacksize failed, error code: ", err_no, " error msg: ", err_msg); } } s = pthread_create(&hThread, &attr, ThreadMain, param_ptr.get()); if (s != 0) { - auto [err_no, err_msg] = GetSystemError(); + auto [err_no, err_msg] = GetErrnoInfo(); ORT_THROW("pthread_create failed, error code: ", err_no, " error msg: ", err_msg); } param_ptr.release(); @@ -249,7 +222,7 @@ class PosixThread : public EnvThread { << ", index: " << p->index << ", mask: " << *p->affinity; } else { - auto [err_no, err_msg] = GetSystemError(ret); + auto [err_no, err_msg] = GetErrnoInfo(ret); #if !defined(USE_MIGRAPHX) LOGS_DEFAULT(ERROR) << "pthread_setaffinity_np failed for thread: " << syscall(SYS_gettid) << ", index: " << p->index @@ -461,7 +434,7 @@ class PosixEnv : public Env { } static common::Status ReportSystemError(const char* operation_name, const std::string& path) { - auto [err_no, err_msg] = GetSystemError(); + auto [err_no, err_msg] = GetErrnoInfo(); std::ostringstream oss; oss << operation_name << " file \"" << path << "\" failed: " << err_msg; return common::Status(common::SYSTEM, err_no, oss.str()); diff --git a/onnxruntime/core/platform/windows/env.cc b/onnxruntime/core/platform/windows/env.cc index 0b2db3f33f4b9..dc090e446e60f 100644 --- a/onnxruntime/core/platform/windows/env.cc +++ b/onnxruntime/core/platform/windows/env.cc @@ -110,11 +110,10 @@ class WindowsThread : public EnvThread { local_param.get(), 0, &threadID); if (th_handle == 0) { - auto err = errno; auto dos_error = _doserrno; - char message_buf[256]; - strerror_s(message_buf, sizeof(message_buf), err); - ORT_THROW("WindowThread:_beginthreadex failed with message: ", message_buf, " doserrno: ", dos_error); + auto [err, msg] = GetErrnoInfo(); + ORT_THROW("WindowThread:_beginthreadex failed with errno:", err, " message:", msg, + " doserrno:", dos_error); } local_param.release(); hThread.reset(reinterpret_cast(th_handle)); diff --git a/onnxruntime/test/platform/env_test.cc b/onnxruntime/test/platform/env_test.cc index e0bffed8c4c17..d5c19c708da8e 100644 --- a/onnxruntime/test/platform/env_test.cc +++ b/onnxruntime/test/platform/env_test.cc @@ -33,5 +33,13 @@ TEST(PlatformEnvTest, DirectoryCreationAndDeletion) { ASSERT_FALSE(env.FolderExists(root_dir)); } +TEST(PlatformEnvTest, GetErrnoInfo) { + // command that should generate an errno error + std::ifstream file("non_existent_file"); + ASSERT_TRUE(file.fail()); + auto [err, msg] = GetErrnoInfo(); + ASSERT_NE(err, 0); + ASSERT_EQ(msg, "No such file or directory"); +} } // namespace test } // namespace onnxruntime From 905130dec7177be009b83df4d6207283df4c4ffa Mon Sep 17 00:00:00 2001 From: Scott McKay Date: Tue, 16 Apr 2024 13:24:55 +1000 Subject: [PATCH 2/5] Fix linux build error --- onnxruntime/core/platform/env.cc | 2 +- onnxruntime/core/platform/posix/env.cc | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/onnxruntime/core/platform/env.cc b/onnxruntime/core/platform/env.cc index 12787c70544b7..6726f7c3b7efc 100644 --- a/onnxruntime/core/platform/env.cc +++ b/onnxruntime/core/platform/env.cc @@ -46,7 +46,7 @@ std::pair GetErrnoInfo() { msg = ret == 0 ? buf : "Failed to get error message"; // buf is guaranteed to be null terminated by strerror_s #else // strerror_r return type differs by platform. - auto ret = strerror_r(err, buf, buflen); + auto ret = strerror_r(err, buf, sizeof(buf)); if (std::is_same_v) { // POSIX returns int msg = ret == 0 ? buf : "Failed to get error message"; } else { diff --git a/onnxruntime/core/platform/posix/env.cc b/onnxruntime/core/platform/posix/env.cc index a6699c48a5e13..9999550c241c8 100644 --- a/onnxruntime/core/platform/posix/env.cc +++ b/onnxruntime/core/platform/posix/env.cc @@ -222,7 +222,8 @@ class PosixThread : public EnvThread { << ", index: " << p->index << ", mask: " << *p->affinity; } else { - auto [err_no, err_msg] = GetErrnoInfo(ret); + errno = ret; + auto [err_no, err_msg] = GetErrnoInfo(); #if !defined(USE_MIGRAPHX) LOGS_DEFAULT(ERROR) << "pthread_setaffinity_np failed for thread: " << syscall(SYS_gettid) << ", index: " << p->index From fdc27a2e6b6d1ddc7cbdc5bd62e66238489d3d55 Mon Sep 17 00:00:00 2001 From: Scott McKay Date: Wed, 17 Apr 2024 08:22:34 +1000 Subject: [PATCH 3/5] Update onnxruntime/core/platform/env.cc Co-authored-by: Edward Chen <18449977+edgchen1@users.noreply.github.com> --- onnxruntime/core/platform/env.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/onnxruntime/core/platform/env.cc b/onnxruntime/core/platform/env.cc index 6726f7c3b7efc..bb34e02e7c113 100644 --- a/onnxruntime/core/platform/env.cc +++ b/onnxruntime/core/platform/env.cc @@ -47,7 +47,7 @@ std::pair GetErrnoInfo() { #else // strerror_r return type differs by platform. auto ret = strerror_r(err, buf, sizeof(buf)); - if (std::is_same_v) { // POSIX returns int + if constexpr (std::is_same_v) { // POSIX returns int msg = ret == 0 ? buf : "Failed to get error message"; } else { // GNU returns char* From 331209c81313e2a7512c1446bb9190cf095689e3 Mon Sep 17 00:00:00 2001 From: Scott McKay Date: Wed, 17 Apr 2024 11:31:40 +1000 Subject: [PATCH 4/5] Update unit test to address PR comments --- onnxruntime/test/platform/env_test.cc | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/onnxruntime/test/platform/env_test.cc b/onnxruntime/test/platform/env_test.cc index d5c19c708da8e..9ea8586af693b 100644 --- a/onnxruntime/test/platform/env_test.cc +++ b/onnxruntime/test/platform/env_test.cc @@ -38,8 +38,20 @@ TEST(PlatformEnvTest, GetErrnoInfo) { std::ifstream file("non_existent_file"); ASSERT_TRUE(file.fail()); auto [err, msg] = GetErrnoInfo(); - ASSERT_NE(err, 0); - ASSERT_EQ(msg, "No such file or directory"); + ASSERT_EQ(err, ENOENT); + +#if defined(_WIN32) +#pragma warning(push) +#pragma warning(disable : 4996) +#endif + + // GetErrnoInfo uses strerror_r or strerror_s depending on the platform. use the unsafe std::sterror to get the + // expected value given this is a unit test so doesn't have to be as robust. + ASSERT_EQ(msg, std::strerror(ENOENT)); + +#if defined(_WIN32) +#pragma warning(pop) +#endif } } // namespace test } // namespace onnxruntime From 26880ef8b053cd49e1cded4cc27229545d707eb0 Mon Sep 17 00:00:00 2001 From: Scott McKay Date: Wed, 17 Apr 2024 11:38:35 +1000 Subject: [PATCH 5/5] lint --- onnxruntime/core/platform/env.h | 1 - 1 file changed, 1 deletion(-) diff --git a/onnxruntime/core/platform/env.h b/onnxruntime/core/platform/env.h index fe72cf7ffd0a8..6917f42091bf3 100644 --- a/onnxruntime/core/platform/env.h +++ b/onnxruntime/core/platform/env.h @@ -102,7 +102,6 @@ std::ostream& operator<<(std::ostream& os, gsl::span); /// errno and the error message string if errno indicates an error. std::pair GetErrnoInfo(); - /// \brief An interface used by the onnxruntime implementation to /// access operating system functionality like the filesystem etc. ///