From 1e6250e18339306a16be4a57b211d7938661c340 Mon Sep 17 00:00:00 2001 From: Gabi Melman Date: Sun, 1 Dec 2024 14:16:52 +0200 Subject: [PATCH] Gabime/fwrite unlocked (#3276) * Use locking fwrite_unlocked if possible * Added compile definitions to header_only --- CMakeLists.txt | 24 ++++++++++++---- include/spdlog/details/file_helper-inl.h | 3 +- include/spdlog/details/os-inl.h | 12 ++++++++ include/spdlog/details/os.h | 4 +++ include/spdlog/sinks/ansicolor_sink-inl.h | 4 +-- include/spdlog/sinks/stdout_sinks-inl.h | 9 +++--- tests/test_misc.cpp | 35 +++++++++++++++++++++++ 7 files changed, 79 insertions(+), 12 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 611c9426..e56c5699 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -108,15 +108,15 @@ endif() if(WIN32) option(SPDLOG_WCHAR_SUPPORT "Support wchar api" OFF) option(SPDLOG_WCHAR_FILENAMES "Support wchar filenames" OFF) - option(SPDLOG_WCHAR_CONSOLE "Support wchar output to console" OFF) + option(SPDLOG_WCHAR_CONSOLE "Support wchar output to console" OFF) else() set(SPDLOG_WCHAR_SUPPORT OFF CACHE BOOL "non supported option" FORCE) set(SPDLOG_WCHAR_FILENAMES OFF CACHE BOOL "non supported option" FORCE) set(SPDLOG_WCHAR_CONSOLE OFF CACHE BOOL "non supported option" FORCE) endif() -if(MSVC) - option(SPDLOG_MSVC_UTF8 "Enable/disable msvc /utf-8 flag required by fmt lib" ON) +if(MSVC) + option(SPDLOG_MSVC_UTF8 "Enable/disable msvc /utf-8 flag required by fmt lib" ON) endif() if(${CMAKE_SYSTEM_NAME} STREQUAL "Linux") @@ -238,6 +238,20 @@ if(SPDLOG_FMT_EXTERNAL OR SPDLOG_FMT_EXTERNAL_HO) set(PKG_CONFIG_REQUIRES fmt) # add dependency to pkg-config endif() +# --------------------------------------------------------------------------------------- +# Check if fwrite_unlocked/_fwrite_nolock is available +# --------------------------------------------------------------------------------------- +include(CheckSymbolExists) +if(WIN32) + check_symbol_exists(_fwrite_nolock "stdio.h" HAVE_FWRITE_UNLOCKED) +else () + check_symbol_exists(fwrite_unlocked "stdio.h" HAVE_FWRITE_UNLOCKED) +endif() +if(HAVE_FWRITE_UNLOCKED) + target_compile_definitions(spdlog PRIVATE SPDLOG_FWRITE_UNLOCKED) + target_compile_definitions(spdlog_header_only INTERFACE SPDLOG_FWRITE_UNLOCKED) +endif() + # --------------------------------------------------------------------------------------- # Add required libraries for Android CMake build # --------------------------------------------------------------------------------------- @@ -276,9 +290,9 @@ if(MSVC) if(SPDLOG_MSVC_UTF8) # fmtlib requires the /utf-8 flag when building with msvc. # see https://github.com/fmtlib/fmt/pull/4159 on the purpose of the additional - # "$<$,$>" + # "$<$,$>" target_compile_options(spdlog PUBLIC $<$,$>:/utf-8>) - target_compile_options(spdlog_header_only INTERFACE $<$,$>:/utf-8>) + target_compile_options(spdlog_header_only INTERFACE $<$,$>:/utf-8>) endif() endif() diff --git a/include/spdlog/details/file_helper-inl.h b/include/spdlog/details/file_helper-inl.h index 37d1d46f..8742b96c 100644 --- a/include/spdlog/details/file_helper-inl.h +++ b/include/spdlog/details/file_helper-inl.h @@ -101,7 +101,8 @@ SPDLOG_INLINE void file_helper::write(const memory_buf_t &buf) { if (fd_ == nullptr) return; size_t msg_size = buf.size(); auto data = buf.data(); - if (std::fwrite(data, 1, msg_size, fd_) != msg_size) { + + if (!details::os::fwrite_bytes(data, msg_size, fd_)) { throw_spdlog_ex("Failed writing to file " + os::filename_to_str(filename_), errno); } } diff --git a/include/spdlog/details/os-inl.h b/include/spdlog/details/os-inl.h index e4d4771b..8ee42304 100644 --- a/include/spdlog/details/os-inl.h +++ b/include/spdlog/details/os-inl.h @@ -589,6 +589,18 @@ SPDLOG_INLINE bool fsync(FILE *fp) { #endif } +// Do non-locking fwrite if possible by the os or use the regular locking fwrite +// Return true on success. +SPDLOG_INLINE bool fwrite_bytes(const void *ptr, const size_t n_bytes, FILE *fp) { + #if defined(_WIN32) && defined(SPDLOG_FWRITE_UNLOCKED) + return _fwrite_nolock(ptr, 1, n_bytes, fp) == n_bytes; + #elif defined(SPDLOG_FWRITE_UNLOCKED) + return ::fwrite_unlocked(ptr, 1, n_bytes, fp) == n_bytes; + #else + return std::fwrite(ptr, 1, n_bytes, fp) == n_bytes; + #endif +} + } // namespace os } // namespace details } // namespace spdlog diff --git a/include/spdlog/details/os.h b/include/spdlog/details/os.h index b1069edc..5fd12bac 100644 --- a/include/spdlog/details/os.h +++ b/include/spdlog/details/os.h @@ -114,6 +114,10 @@ SPDLOG_API std::string getenv(const char *field); // Return true on success. SPDLOG_API bool fsync(FILE *fp); +// Do non-locking fwrite if possible by the os or use the regular locking fwrite +// Return true on success. +SPDLOG_API bool fwrite_bytes(const void *ptr, const size_t n_bytes, FILE *fp); + } // namespace os } // namespace details } // namespace spdlog diff --git a/include/spdlog/sinks/ansicolor_sink-inl.h b/include/spdlog/sinks/ansicolor_sink-inl.h index 2194f67b..8fd3078a 100644 --- a/include/spdlog/sinks/ansicolor_sink-inl.h +++ b/include/spdlog/sinks/ansicolor_sink-inl.h @@ -106,14 +106,14 @@ SPDLOG_INLINE void ansicolor_sink::set_color_mode(color_mode mode) template SPDLOG_INLINE void ansicolor_sink::print_ccode_(const string_view_t &color_code) { - fwrite(color_code.data(), sizeof(char), color_code.size(), target_file_); + details::os::fwrite_bytes(color_code.data(), color_code.size(), target_file_); } template SPDLOG_INLINE void ansicolor_sink::print_range_(const memory_buf_t &formatted, size_t start, size_t end) { - fwrite(formatted.data() + start, sizeof(char), end - start, target_file_); + details::os::fwrite_bytes(formatted.data() + start, end - start, target_file_); } template diff --git a/include/spdlog/sinks/stdout_sinks-inl.h b/include/spdlog/sinks/stdout_sinks-inl.h index f98244db..dcb21d84 100644 --- a/include/spdlog/sinks/stdout_sinks-inl.h +++ b/include/spdlog/sinks/stdout_sinks-inl.h @@ -10,6 +10,7 @@ #include #include #include +#include #ifdef _WIN32 // under windows using fwrite to non-binary stream results in \r\r\n (see issue #1675) @@ -22,7 +23,7 @@ #include // _get_osfhandle(..) #include // _fileno(..) -#endif // WIN32 +#endif // _WIN32 namespace spdlog { @@ -44,7 +45,7 @@ SPDLOG_INLINE stdout_sink_base::stdout_sink_base(FILE *file) if (handle_ == INVALID_HANDLE_VALUE && file != stdout && file != stderr) { throw_spdlog_ex("spdlog::stdout_sink_base: _get_osfhandle() failed", errno); } -#endif // WIN32 +#endif // _WIN32 } template @@ -67,8 +68,8 @@ SPDLOG_INLINE void stdout_sink_base::log(const details::log_msg &m std::lock_guard lock(mutex_); memory_buf_t formatted; formatter_->format(msg, formatted); - ::fwrite(formatted.data(), sizeof(char), formatted.size(), file_); -#endif // WIN32 + details::os::fwrite_bytes(formatted.data(), formatted.size(), file_); +#endif // _WIN32 ::fflush(file_); // flush every line to terminal } diff --git a/tests/test_misc.cpp b/tests/test_misc.cpp index 5ae3c045..80ba51d6 100644 --- a/tests/test_misc.cpp +++ b/tests/test_misc.cpp @@ -1,3 +1,7 @@ +#ifdef _WIN32 // to prevent fopen warning on windows +#define _CRT_SECURE_NO_WARNINGS +#endif + #include "includes.h" #include "test_sink.h" @@ -185,3 +189,34 @@ TEST_CASE("utf8 to utf16 conversion using windows api", "[windows utf]") { REQUIRE(std::wstring(buffer.data(), buffer.size()) == std::wstring(L"\x306d\x3053")); } #endif + +struct auto_closer { + FILE* fp = nullptr; + explicit auto_closer(FILE* f) : fp(f) {} + auto_closer(const auto_closer&) = delete; + auto_closer& operator=(const auto_closer&) = delete; + ~auto_closer() { + if (fp != nullptr) (void)std::fclose(fp); + } +}; + +TEST_CASE("os::fwrite_bytes", "[os]") { + using spdlog::details::os::fwrite_bytes; + using spdlog::details::os::create_dir; + const char* filename = "log_tests/test_fwrite_bytes.txt"; + const char *msg = "hello"; + prepare_logdir(); + REQUIRE(create_dir(SPDLOG_FILENAME_T("log_tests")) == true); + { + auto_closer closer(std::fopen(filename, "wb")); + REQUIRE(closer.fp != nullptr); + REQUIRE(fwrite_bytes(msg, std::strlen(msg), closer.fp) == true); + REQUIRE(fwrite_bytes(msg, 0, closer.fp) == true); + std::fflush(closer.fp); + REQUIRE(spdlog::details::os::filesize(closer.fp) == 5); + } + // fwrite_bytes should return false on write failure + auto_closer closer(std::fopen(filename, "r")); + REQUIRE(closer.fp != nullptr); + REQUIRE_FALSE(fwrite_bytes("Hello", 5, closer.fp)); +}