From 2c23e1a3beab9a0e573038a4db840ea3a512fae4 Mon Sep 17 00:00:00 2001 From: David Beckingsale Date: Tue, 7 Apr 2026 20:41:32 -0700 Subject: [PATCH 1/8] Switch to spdlog for logging --- .gitmodules | 3 + cmake/SetupUmpireOptions.cmake | 1 - cmake/SetupUmpireThirdParty.cmake | 34 ----- src/tpl/CMakeLists.txt | 50 ++++++++ src/tpl/umpire/spdlog | 1 + src/umpire/ResourceManager.cpp | 7 +- src/umpire/util/CMakeLists.txt | 10 +- src/umpire/util/Logger.cpp | 202 +++++++++++++++++++++++++----- src/umpire/util/Logger.hpp | 45 ++++--- src/umpire/util/Macros.hpp | 59 ++------- src/umpire/util/OutputBuffer.cpp | 80 ------------ src/umpire/util/OutputBuffer.hpp | 35 ------ src/umpire/util/error.hpp | 1 - src/umpire/util/io.cpp | 124 ------------------ src/umpire/util/io.hpp | 41 +++--- umpire-config.cmake.in | 15 +++ 16 files changed, 295 insertions(+), 413 deletions(-) create mode 160000 src/tpl/umpire/spdlog delete mode 100644 src/umpire/util/OutputBuffer.cpp delete mode 100644 src/umpire/util/OutputBuffer.hpp diff --git a/.gitmodules b/.gitmodules index a30b6f91a..25e63b2b4 100644 --- a/.gitmodules +++ b/.gitmodules @@ -13,3 +13,6 @@ [submodule "src/tpl/umpire/fmt"] path = src/tpl/umpire/fmt url = https://github.com/fmtlib/fmt +[submodule "src/tpl/umpire/spdlog"] + path = src/tpl/umpire/spdlog + url = https://github.com/gabime/spdlog.git diff --git a/cmake/SetupUmpireOptions.cmake b/cmake/SetupUmpireOptions.cmake index 725afbae1..aa8ceead0 100644 --- a/cmake/SetupUmpireOptions.cmake +++ b/cmake/SetupUmpireOptions.cmake @@ -25,7 +25,6 @@ option(UMPIRE_ENABLE_NUMA "Build Umpire with NUMA support" Off) option(UMPIRE_ENABLE_OPENMP_TARGET "Build Umpire with OPENMP target" Off) option(UMPIRE_ENABLE_LOGGING "Build Umpire with Logging enabled" On) -option(UMPIRE_ENABLE_SLIC "Build Umpire with SLIC logging" Off) option(UMPIRE_ENABLE_BACKTRACE "Build Umpire with allocation backtrace enabled" Off) option(UMPIRE_ENABLE_BACKTRACE_SYMBOLS "Build Umpire with symbol support" Off) option(UMPIRE_ENABLE_PEDANTIC_WARNINGS "Enable pedantic compiler warnings" On) diff --git a/cmake/SetupUmpireThirdParty.cmake b/cmake/SetupUmpireThirdParty.cmake index 162b5e7c9..d862cfe36 100644 --- a/cmake/SetupUmpireThirdParty.cmake +++ b/cmake/SetupUmpireThirdParty.cmake @@ -36,40 +36,6 @@ if (UMPIRE_ENABLE_UMAP) DEPENDS_ON -lpthread -lrt) endif () -if (ENABLE_SLIC AND ENABLE_LOGGING) - find_library( SLIC_LIBRARY - libslic.a - PATHS ${SLIC_LIBRARY_PATH} - ) - - if (NOT SLIC_LIBRARY) - message(FATAL_ERROR "Could not find SLIC library, make sure SLIC_LIBRARY_PATH is set properly") - endif() - - find_library( SLIC_UTIL_LIBRARY - libaxom_utils.a - PATHS ${SLIC_LIBRARY_PATH} - ) - - if (NOT SLIC_UTIL_LIBRARY) - message(FATAL_ERROR "Could not find Axom Utility Library for SLIC, make sure SLIC_LIBRARY_PATH is set properly") - endif() - - find_path( SLIC_INCLUDE_DIR - slic/slic.hpp - PATHS ${SLIC_INCLUDE_PATH} - ) - - if (NOT SLIC_INCLUDE_DIR) - message(FATAL_ERROR "Could not find SLIC include directory, make sure SLIC_INCLUDE_PATH is set properly") - endif() - - blt_register_library( NAME slic - INCLUDES ${SLIC_INCLUDE_DIR} - LIBRARIES ${SLIC_LIBRARY} ${SLIC_UTIL_LIBRARY} - ) -endif () - if (NOT "${CMAKE_CXX_COMPILER_ID}" STREQUAL "MSVC") blt_register_library( NAME backtrace_symbols LIBRARIES ${CMAKE_DL_LIBS} diff --git a/src/tpl/CMakeLists.txt b/src/tpl/CMakeLists.txt index 55133c734..ad0987ec9 100644 --- a/src/tpl/CMakeLists.txt +++ b/src/tpl/CMakeLists.txt @@ -160,5 +160,55 @@ if (NOT TARGET ${UMPIRE_FMT_TARGET}) endif () endif () +############################################################################## +# spdlog +############################################################################## +if (NOT TARGET spdlog::spdlog) + # Try to find external spdlog installation + if (DEFINED spdlog_DIR) + find_package(spdlog REQUIRED + NO_DEFAULT_PATH + PATHS + ${spdlog_DIR} + ${spdlog_DIR}/lib/cmake/spdlog) + + set_target_properties(spdlog::spdlog PROPERTIES IMPORTED_GLOBAL TRUE) + blt_convert_to_system_includes(TARGET spdlog::spdlog) + set(UMPIRE_CONFIG_spdlog_DIR ${spdlog_DIR} CACHE PATH "") + else () + if (NOT EXISTS ${PROJECT_SOURCE_DIR}/src/tpl/umpire/spdlog/CMakeLists.txt) + message(FATAL_ERROR "spdlog submodule not initialized. Run 'git submodule update --init --recursive' in the git repository or set spdlog_DIR to use an external build of spdlog.") + else () + # Configure spdlog to use external fmt (if available) + if (TARGET ${UMPIRE_FMT_TARGET}) + set(SPDLOG_FMT_EXTERNAL ON CACHE BOOL "Use external fmt library") + set(SPDLOG_FMT_EXTERNAL_HO OFF CACHE BOOL "Use external fmt header-only library") + + # Help spdlog find the fmt package by setting fmt_DIR + # This works whether fmt is external or from our submodule + if (DEFINED fmt_DIR) + # External fmt case - use the user-provided path + set(fmt_DIR ${fmt_DIR} CACHE PATH "Path to fmt") + else () + # Submodule fmt case - point to build directory where fmt exports its config + set(fmt_DIR ${CMAKE_BINARY_DIR}/src/tpl/umpire/fmt CACHE PATH "Path to fmt") + endif () + else () + # No fmt available, let spdlog use its bundled fmt + set(SPDLOG_FMT_EXTERNAL OFF CACHE BOOL "Use external fmt library") + endif () + + # Disable unnecessary spdlog features + set(SPDLOG_INSTALL ON CACHE BOOL "Generate install target") + set(SPDLOG_BUILD_SHARED OFF CACHE BOOL "Build shared library") + + # In this case, spdlog will be installed inside of Umpire install prefix + set(UMPIRE_CONFIG_spdlog_DIR ${CMAKE_INSTALL_PREFIX} CACHE PATH "") + + add_subdirectory(umpire/spdlog) + endif () + endif () +endif () + set(UMPIRE_ENABLE_TESTS ${OLD_ENABLE_TESTS}) set(UMPIRE_ENABLE_FORTRAN ${OLD_ENABLE_FORTRAN}) diff --git a/src/tpl/umpire/spdlog b/src/tpl/umpire/spdlog new file mode 160000 index 000000000..fb1227486 --- /dev/null +++ b/src/tpl/umpire/spdlog @@ -0,0 +1 @@ +Subproject commit fb1227486b860c673c5cfdc49359707e30c6b5f8 diff --git a/src/umpire/ResourceManager.cpp b/src/umpire/ResourceManager.cpp index 8c31492a4..8e1f3a8cf 100644 --- a/src/umpire/ResourceManager.cpp +++ b/src/umpire/ResourceManager.cpp @@ -64,10 +64,7 @@ ResourceManager::ResourceManager() { UMPIRE_LOG(Debug, "() entering"); - const char* env_enable_log{std::getenv("UMPIRE_LOG_LEVEL")}; - const bool enable_log{env_enable_log != nullptr}; - - util::initialize_io(enable_log); + util::Logger::initialize(); initialize(); @@ -89,6 +86,8 @@ ResourceManager::~ResourceManager() allocator.reset(); } + + util::Logger::finalize(); } void ResourceManager::initialize() diff --git a/src/umpire/util/CMakeLists.txt b/src/umpire/util/CMakeLists.txt index 20893c2b2..d230b1398 100644 --- a/src/umpire/util/CMakeLists.txt +++ b/src/umpire/util/CMakeLists.txt @@ -22,7 +22,6 @@ set (umpire_util_headers MemoryResourceTraits.hpp MemoryMap.hpp MemoryMap.inl - OutputBuffer.hpp Platform.hpp allocation_statistics.hpp detect_vendor.hpp @@ -43,7 +42,6 @@ set (umpire_util_sources io.cpp Logger.cpp MPI.cpp - OutputBuffer.cpp allocation_statistics.cpp detect_vendor.cpp) @@ -53,7 +51,7 @@ if (UMPIRE_ENABLE_NUMA) numa.cpp) endif () -set (umpire_util_depends camp umpire_event umpire_tpl_judy ${UMPIRE_FMT_TARGET}) +set (umpire_util_depends camp umpire_event umpire_tpl_judy ${UMPIRE_FMT_TARGET} spdlog::spdlog) if (NOT "${CMAKE_CXX_COMPILER_ID}" STREQUAL "MSVC") set (umpire_util_depends @@ -67,12 +65,6 @@ if (UMPIRE_ENABLE_NUMA) numa) endif () -if (UMPIRE_ENABLE_SLIC AND UMPIRE_ENABLE_LOGGING) - set (umpire_util_depends - ${umpire_util_depends} - slic) -endif() - if (UMPIRE_ENABLE_MPI) set (umpire_util_depends ${umpire_util_depends} diff --git a/src/umpire/util/Logger.cpp b/src/umpire/util/Logger.cpp index a024fb46c..520098b9c 100644 --- a/src/umpire/util/Logger.cpp +++ b/src/umpire/util/Logger.cpp @@ -4,69 +4,203 @@ // // SPDX-License-Identifier: (MIT) ////////////////////////////////////////////////////////////////////////////// - #include "umpire/util/Logger.hpp" -#include // for std::equal -#include // for std::toupper -#include // for getenv() +#include +#include +#include +#include + +#include "spdlog/async.h" +#include "spdlog/sinks/basic_file_sink.h" +#include "spdlog/sinks/null_sink.h" +#include "spdlog/sinks/stdout_color_sinks.h" +#include "spdlog/spdlog.h" #include "umpire/util/io.hpp" +#if !defined(_MSC_VER) +#include +#else +#include +#define getpid _getpid +#endif + namespace umpire { namespace util { -static const char* env_name = "UMPIRE_LOG_LEVEL"; -static message::Level defaultLevel = message::Info; +// Static member initialization +std::shared_ptr Logger::s_logger = nullptr; +message::Level Logger::s_level = message::Info; +bool Logger::s_initialized = false; static const char* MessageLevelName[message::Num_Levels] = {"ERROR", "WARNING", "INFO", "DEBUG"}; -static int case_insensitive_match(const std::string s1, const std::string s2) +static bool case_insensitive_match(const std::string& s1, const std::string& s2) { - return (s1.size() == s2.size()) && std::equal(s1.begin(), s1.end(), s2.begin(), [](char c1, char c2) { - return (std::toupper(c1) == std::toupper(c2)); - }); + return (s1.size() == s2.size()) && + std::equal(s1.begin(), s1.end(), s2.begin(), + [](char c1, char c2) { return std::toupper(c1) == std::toupper(c2); }); } -Logger::Logger() noexcept - : // by default, all message streams are disabled - m_is_enabled{false, false, false, false} +message::Level Logger::parseEnvLogLevel() noexcept { - message::Level level{defaultLevel}; - const char* enval = std::getenv(env_name); - - if (enval) { - for (int i = 0; i < message::Num_Levels; ++i) { - if (case_insensitive_match(enval, MessageLevelName[i])) { - level = static_cast(i); - break; - } + const char* env_level = std::getenv("UMPIRE_LOG_LEVEL"); + if (!env_level) { + return message::Info; // default + } + + std::string level_str(env_level); + for (int i = 0; i < message::Num_Levels; ++i) { + if (case_insensitive_match(level_str, MessageLevelName[i])) { + return static_cast(i); } } - setLoggingMsgLevel(level); + return message::Info; // fallback +} + +std::string Logger::generateLogFilename() +{ + const std::string& output_dir = get_io_output_dir(); + const std::string& basename = get_io_output_basename(); + const int pid = getpid(); + + // Reuse existing make_unique_filename from io.cpp + return make_unique_filename(output_dir, basename, pid, "log"); +} + +spdlog::level::level_enum Logger::convertLevel(message::Level level) noexcept +{ + switch (level) { + case message::Error: + return spdlog::level::err; + case message::Warning: + return spdlog::level::warn; + case message::Info: + return spdlog::level::info; + case message::Debug: + return spdlog::level::debug; + default: + return spdlog::level::info; + } +} + +void Logger::initialize() +{ + if (s_initialized) { + return; + } + + // Parse log level from environment + s_level = parseEnvLogLevel(); + + // Check if logging is enabled (UMPIRE_LOG_LEVEL must be set) + const char* env_enable_log = std::getenv("UMPIRE_LOG_LEVEL"); + if (!env_enable_log) { + // Logging disabled - create a null logger + auto null_sink = std::make_shared(); + s_logger = std::make_shared("umpire", null_sink); + spdlog::register_logger(s_logger); + s_initialized = true; + return; + } + + // Check for async mode + const char* env_async = std::getenv("UMPIRE_LOG_ASYNC"); + const bool use_async = (env_async && (std::string(env_async) == "1" || + case_insensitive_match(env_async, "true") || + case_insensitive_match(env_async, "on"))); + + // Get queue size for async logging + const char* env_queue_size = std::getenv("UMPIRE_LOG_QUEUE_SIZE"); + const size_t queue_size = env_queue_size ? std::atoi(env_queue_size) : 8192; + + // Initialize async thread pool if needed + if (use_async && !spdlog::thread_pool()) { + spdlog::init_thread_pool(queue_size, 1); + } + + // Create sinks + std::vector sinks; + + // File sink (always enabled when UMPIRE_LOG_LEVEL is set) + std::string log_filename = generateLogFilename(); + auto file_sink = std::make_shared(log_filename, false); + sinks.push_back(file_sink); + + // Optional console sink + const char* env_console = std::getenv("UMPIRE_LOG_TO_CONSOLE"); + const bool log_to_console = (env_console && (std::string(env_console) == "1" || + case_insensitive_match(env_console, "true") || + case_insensitive_match(env_console, "on"))); + if (log_to_console) { + auto console_sink = std::make_shared(); + sinks.push_back(console_sink); + } + + // Create logger (sync or async) + if (use_async) { + s_logger = std::make_shared("umpire", sinks.begin(), sinks.end(), + spdlog::thread_pool(), + spdlog::async_overflow_policy::block); + } else { + s_logger = std::make_shared("umpire", sinks.begin(), sinks.end()); + } + + // Register with spdlog + spdlog::register_logger(s_logger); + + // Set format pattern to match current output: [LEVEL][file:line]: message + s_logger->set_pattern("[%^%L%$][%s:%#]: %v"); + + // Set log level + s_logger->set_level(convertLevel(s_level)); + + // Flush on every message for Error level (safety) + s_logger->flush_on(spdlog::level::err); + + s_initialized = true; } -void Logger::setLoggingMsgLevel(message::Level level) noexcept +void Logger::finalize() { - for (int i = 0; i < message::Num_Levels; ++i) - m_is_enabled[i] = (i <= level); + if (s_logger) { + s_logger->flush(); + spdlog::drop("umpire"); + s_logger.reset(); + } + + // Shutdown async thread pool if it exists + if (spdlog::thread_pool()) { + spdlog::shutdown(); + } + + s_initialized = false; } -void Logger::logMessage(message::Level level, const std::string& message, const std::string& fileName, - int line) noexcept +bool Logger::shouldLog(message::Level level) noexcept { - if (!logLevelEnabled(level)) + if (!s_initialized || !s_logger) { + return false; + } + return level <= s_level; +} + +void Logger::log(message::Level level, const std::string& message, const std::string& fileName, + int line) noexcept +{ + if (!s_initialized || !s_logger || !shouldLog(level)) { return; + } - umpire::log() << "[" << MessageLevelName[level] << "]" - << "[" << fileName << ":" << line << "]:" << message << std::endl; + // Use spdlog's source location logging + s_logger->log(spdlog::source_loc{fileName.c_str(), line, ""}, convertLevel(level), message); } -Logger* Logger::getActiveLogger() +std::shared_ptr Logger::getSpdlogger() { - static Logger logger; - return &logger; + return s_logger; } } // end namespace util diff --git a/src/umpire/util/Logger.hpp b/src/umpire/util/Logger.hpp index c08747dc3..21d90354c 100644 --- a/src/umpire/util/Logger.hpp +++ b/src/umpire/util/Logger.hpp @@ -7,8 +7,17 @@ #ifndef UMPIRE_Logger_HPP #define UMPIRE_Logger_HPP +#include #include +// Forward declare spdlog types to avoid including spdlog.h in header +namespace spdlog { +class logger; +namespace level { +enum level_enum : int; +} +} // namespace spdlog + namespace umpire { namespace util { @@ -18,39 +27,39 @@ enum Level { Warning, Info, Debug, - Num_Levels }; } // end namespace message class Logger { public: - void setLoggingMsgLevel(message::Level level) noexcept; - - void logMessage(message::Level level, const std::string& message, const std::string& fileName, int line) noexcept; - + // Initialize the global logger (called once from ResourceManager) static void initialize(); + // Finalize and flush logs (called from ResourceManager destructor) static void finalize(); - static Logger* getActiveLogger(); + // Check if a log level should be logged (for macro optimization) + static bool shouldLog(message::Level level) noexcept; - inline bool logLevelEnabled(message::Level level) - { - if (level < 0 || level >= message::Num_Levels || m_is_enabled[level] == false) - return false; - else - return true; - }; + // Log a message at the specified level + static void log(message::Level level, const std::string& message, + const std::string& fileName, int line) noexcept; - ~Logger() noexcept = default; - Logger(const Logger&) = delete; - Logger& operator=(const Logger&) = delete; + // Get the underlying spdlog logger (for advanced usage) + static std::shared_ptr getSpdlogger(); private: - Logger() noexcept; + Logger() = delete; + ~Logger() = delete; + + static spdlog::level::level_enum convertLevel(message::Level level) noexcept; + static message::Level parseEnvLogLevel() noexcept; + static std::string generateLogFilename(); - bool m_is_enabled[message::Num_Levels]; + static std::shared_ptr s_logger; + static message::Level s_level; + static bool s_initialized; }; } // end namespace util diff --git a/src/umpire/util/Macros.hpp b/src/umpire/util/Macros.hpp index dc38f0984..a45e27fa8 100644 --- a/src/umpire/util/Macros.hpp +++ b/src/umpire/util/Macros.hpp @@ -19,56 +19,19 @@ #define UMPIRE_ASSERT(condition) assert(condition) #ifdef UMPIRE_ENABLE_LOGGING -#ifdef UMPIRE_ENABLE_SLIC -#include // for getenv() -#include // for strcasecmp() - -#include - -#include "slic/GenericOutputStream.hpp" -#include "slic/Logger.hpp" -#define UMPIRE_LOG(lvl, msg) \ - { \ - axom::slic::Logger* plog = axom::slic::Logger::getActiveLogger(); \ - if (plog == nullptr) { \ - static const std::string env_name = "UMPIRE_LOG_LEVEL"; \ - axom::slic::Logger::initialize(); \ - plog = axom::slic::Logger::getActiveLogger(); \ - axom::slic::message::Level level; \ - level = axom::slic::message::Level::Error; \ - char* enval = std::getenv(env_name.c_str()); \ - if (enval != NULL) { \ - for (int i = 0; i < axom::slic::message::Level::Num_Levels; ++i) { \ - if (strcasecmp(enval, axom::slic::message::MessageLevelName[i].c_str()) == 0) { \ - level = (axom::slic::message::Level)i; \ - break; \ - } \ - } \ - } \ - plog->setLoggingMsgLevel(level); \ - \ - std::string console_format = std::string("[][:]: \n"); \ - axom::slic::LogStream* console = new axom::slic::GenericOutputStream(&std::cerr, console_format); \ - plog->addStreamToAllMsgLevels(console); \ - } \ - std::ostringstream local_msg; \ - local_msg << " " << __func__ << " " << msg; \ - plog->logMessage(axom::slic::message::lvl, local_msg.str(), std::string(__FILE__), __LINE__); \ - } - -#else #include "umpire/util/Logger.hpp" -#define UMPIRE_LOG(lvl, msg) \ - { \ - if (umpire::util::Logger::getActiveLogger()->logLevelEnabled(umpire::util::message::lvl)) { \ - std::ostringstream local_msg; \ - local_msg << " " << __func__ << " " << msg; \ - umpire::util::Logger::getActiveLogger()->logMessage(umpire::util::message::lvl, local_msg.str(), \ - std::string(__FILE__), __LINE__); \ - } \ - } -#endif // UMPIRE_ENABLE_SLIC + +#define UMPIRE_LOG(lvl, msg) \ + do { \ + if (umpire::util::Logger::shouldLog(umpire::util::message::lvl)) { \ + std::ostringstream umpire_log_stream; \ + umpire_log_stream << " " << __func__ << " " << msg; \ + umpire::util::Logger::log(umpire::util::message::lvl, \ + umpire_log_stream.str(), \ + std::string(__FILE__), __LINE__); \ + } \ + } while (0) #else diff --git a/src/umpire/util/OutputBuffer.cpp b/src/umpire/util/OutputBuffer.cpp deleted file mode 100644 index e2fb14521..000000000 --- a/src/umpire/util/OutputBuffer.cpp +++ /dev/null @@ -1,80 +0,0 @@ -////////////////////////////////////////////////////////////////////////////// -// Copyright (c) 2016-26, Lawrence Livermore National Security, LLC and Umpire -// project contributors. See the COPYRIGHT file for details. -// -// SPDX-License-Identifier: (MIT) -////////////////////////////////////////////////////////////////////////////// - -#include "umpire/util/OutputBuffer.hpp" - -#include - -namespace umpire { -namespace util { - -void OutputBuffer::setConsoleStream(std::ostream* stream) -{ - if (stream) { - d_console_stream = stream->rdbuf(); - } else { - d_console_stream = nullptr; - } -} - -void OutputBuffer::setFileStream(std::ostream* stream) -{ - if (stream) { - d_file_stream = stream->rdbuf(); - } else { - d_file_stream = nullptr; - } -} - -int OutputBuffer::overflow(int ch) -{ - if (ch == EOF) { - return !EOF; - } else { - int r_console{ch}; - int r_file{ch}; - - if (d_console_stream) { - r_console = d_console_stream->sputc(static_cast(ch)); - } - - if (d_file_stream) { - r_file = d_file_stream->sputc(static_cast(ch)); - } - - return r_console == EOF || r_file == EOF ? EOF : ch; - } -} - -int OutputBuffer::sync() -{ - auto ret = 0; - - if (d_console_stream) { - ret = d_console_stream->pubsync(); - } - - if (d_file_stream) { - ret += d_file_stream->pubsync(); - } - - return ret == 0 ? 0 : -1; -} - -OutputBuffer::~OutputBuffer() -{ - if (d_console_stream) { - d_console_stream->pubsync(); - } - - if (d_file_stream) { - d_file_stream->pubsync(); - } -} - -} // end of namespace util -} // end of namespace umpire diff --git a/src/umpire/util/OutputBuffer.hpp b/src/umpire/util/OutputBuffer.hpp deleted file mode 100644 index 397ee6b22..000000000 --- a/src/umpire/util/OutputBuffer.hpp +++ /dev/null @@ -1,35 +0,0 @@ -////////////////////////////////////////////////////////////////////////////// -// Copyright (c) 2016-26, Lawrence Livermore National Security, LLC and Umpire -// project contributors. See the COPYRIGHT file for details. -// -// SPDX-License-Identifier: (MIT) -////////////////////////////////////////////////////////////////////////////// -#ifndef UMPIRE_OutputBuffer_HPP -#define UMPIRE_OutputBuffer_HPP - -#include - -namespace umpire { -namespace util { - -class OutputBuffer : public std::streambuf { - public: - OutputBuffer() = default; - - ~OutputBuffer(); - - void setConsoleStream(std::ostream* stream); - void setFileStream(std::ostream* stream); - - int overflow(int ch) override; - int sync() override; - - private: - std::streambuf* d_console_stream; - std::streambuf* d_file_stream; -}; - -} // end of namespace util -} // end of namespace umpire - -#endif // UMPIRE_OutputBuffer_HPP diff --git a/src/umpire/util/error.hpp b/src/umpire/util/error.hpp index d557f55ec..369343681 100644 --- a/src/umpire/util/error.hpp +++ b/src/umpire/util/error.hpp @@ -123,7 +123,6 @@ class resource_error : public umpire::runtime_error { { \ type e{msg, std::string{__FILE__}, __LINE__}; \ UMPIRE_LOG(Error, e.what()); \ - umpire::util::flush_files(); \ throw e; \ } #endif diff --git a/src/umpire/util/io.cpp b/src/umpire/util/io.cpp index ed96118a7..6f16b688a 100644 --- a/src/umpire/util/io.cpp +++ b/src/umpire/util/io.cpp @@ -9,20 +9,11 @@ #include #include -#include -#include #include -#include "umpire/config.hpp" -#include "umpire/util/MPI.hpp" -#include "umpire/util/Macros.hpp" -#include "umpire/util/OutputBuffer.hpp" -#include "umpire/util/error.hpp" - #if defined(UMPIRE_ENABLE_FILESYSTEM) #include #else -#include #include #include #endif @@ -32,126 +23,11 @@ #else #include #define getpid _getpid -#include #endif namespace umpire { - -std::ostream& log() -{ - static std::ostream out{std::cout.rdbuf()}; - return out; -} - -std::ostream& error() -{ - static std::ostream out{std::cerr.rdbuf()}; - return out; -} - namespace util { -namespace detail { - -OutputBuffer& s_log_buffer_accessor() -{ - static OutputBuffer buffer; - return buffer; -} - -OutputBuffer& s_error_buffer_accessor() -{ - static OutputBuffer buffer; - return buffer; -} - -} // namespace detail - -void initialize_io(const bool enable_log) -{ - OutputBuffer& s_log_buffer = detail::s_log_buffer_accessor(); - OutputBuffer& s_error_buffer = detail::s_error_buffer_accessor(); - - s_log_buffer.setConsoleStream(nullptr); - s_error_buffer.setConsoleStream(&std::cerr); - - log().rdbuf(&s_log_buffer); - error().rdbuf(&s_error_buffer); - - const std::string& root_io_dir{util::get_io_output_dir()}; - const std::string& file_basename{util::get_io_output_basename()}; - - const int pid{getpid()}; - - const std::string log_filename{make_unique_filename(root_io_dir, file_basename, pid, "log")}; - - const std::string error_filename{make_unique_filename(root_io_dir, file_basename, pid, "error")}; - - if (!directory_exists(root_io_dir)) { - if (MPI::isInitialized()) { - if (MPI::getRank() == 0) { -#if defined(UMPIRE_ENABLE_FILESYSTEM) - std::filesystem::path root_io_dir_path{root_io_dir}; - - if (!std::filesystem::exists(root_io_dir_path) && enable_log) { - std::filesystem::create_directories(root_io_dir_path); - } -#else - struct stat info; - if (stat(root_io_dir.c_str(), &info)) { - if (enable_log) { -#ifndef WIN32 - if (mkdir(root_io_dir.c_str(), S_IRWXU | S_IRGRP | S_IXGRP | S_IROTH | S_IXOTH)) { - UMPIRE_ERROR(runtime_error, fmt::format("mkdir({}) failed", root_io_dir)); - } -#else - if (_mkdir(root_io_dir.c_str())) { - UMPIRE_ERROR(runtime_error, fmt::format("mkdir( \"{}\" ) failed", root_io_dir)); - } -#endif - } - } else if (!(S_ISDIR(info.st_mode))) { - UMPIRE_ERROR(runtime_error, fmt::format("{} exists and is not a directory", root_io_dir)); - } -#endif - } - MPI::sync(); - } else { - UMPIRE_ERROR(runtime_error, - "Cannot create output directory before MPI has been initialized. Please unset UMPIRE_OUTPUT_DIR in " - "your environment"); - } - } - - if (enable_log) { - static std::ofstream s_log_ofstream{log_filename}; - - if (s_log_ofstream) { - s_log_buffer.setFileStream(&s_log_ofstream); - } else { - UMPIRE_ERROR(runtime_error, fmt::format("Couldn't open log file: {}", log_filename)); - } - } - - MPI::logMpiInfo(); -} - -void finalize_io() -{ - detail::s_log_buffer_accessor().sync(); - detail::s_log_buffer_accessor().setConsoleStream(nullptr); - detail::s_log_buffer_accessor().setFileStream(nullptr); - detail::s_error_buffer_accessor().sync(); - detail::s_error_buffer_accessor().setConsoleStream(nullptr); - detail::s_error_buffer_accessor().setFileStream(nullptr); -} - -void flush_files() -{ - log().flush(); - error().flush(); -} - std::string make_unique_filename(const std::string& base_dir, const std::string& name, const int pid, const std::string& extension) { diff --git a/src/umpire/util/io.hpp b/src/umpire/util/io.hpp index 5e7c267d2..03c1457ae 100644 --- a/src/umpire/util/io.hpp +++ b/src/umpire/util/io.hpp @@ -7,50 +7,41 @@ #ifndef UMPIRE_io_HPP #define UMPIRE_io_HPP -#include #include namespace umpire { - -// Output streams -std::ostream& log(); -std::ostream& error(); - namespace util { +/*! + * \brief Generate a unique filename with format: base_dir/name.pid.id.extension + * The id is incremented until a non-existing file is found. + */ std::string make_unique_filename(const std::string& base_dir, const std::string& name, const int pid, const std::string& extension); +/*! + * \brief Check if a file exists + */ bool file_exists(const std::string& file); -bool directory_exists(const std::string& file); - -const std::string& get_io_output_dir(); -const std::string& get_io_output_basename(); - /*! - * \brief Initialize the streams. This method is called when ResourceManager is - * initialized. Most users will not need to call this manually. - * \warning This function will capture references to buffers of std::cerr and/or std::cout. If these are using custom - * buffers with explicitly-manager lifetime should may need to call this and finalize_io() to control - * explicitly initialization/finalization of Umpire I/O. + * \brief Check if a directory exists */ -void initialize_io(const bool enable_log); +bool directory_exists(const std::string& file); /*! - * \brief Counterpart of initialize_io that finalizes the streams and ensures that no live references to the buffers - * of standard streams exist. Most users will not need to call this manually. + * \brief Get the output directory from UMPIRE_OUTPUT_DIR environment variable + * Returns "./" if not set */ -void finalize_io(); +const std::string& get_io_output_dir(); /*! - * \brief Synchronize all stream buffers to their respective output sequences. - * This function is usually called by exception generating code like - * UMPIRE_ERROR. + * \brief Get the output basename from UMPIRE_OUTPUT_BASENAME environment variable + * Returns "umpire" if not set */ -void flush_files(); +const std::string& get_io_output_basename(); } // end namespace util } // end namespace umpire -#endif // UMPIRE_IOManager_HPP +#endif // UMPIRE_io_HPP diff --git a/umpire-config.cmake.in b/umpire-config.cmake.in index 3027e11c7..ed6c4f78d 100644 --- a/umpire-config.cmake.in +++ b/umpire-config.cmake.in @@ -53,6 +53,21 @@ if (NOT TARGET @UMPIRE_FMT_TARGET@) ${UMPIRE_PACKAGE_PREFIX_DIR}/lib64/cmake/fmt) endif () +if (@UMPIRE_ENABLE_LOGGING@) + if (NOT TARGET spdlog::spdlog) + set(UMPIRE_SPDLOG_DIR "@UMPIRE_CONFIG_spdlog_DIR@") + if(NOT spdlog_DIR) + set(spdlog_DIR ${UMPIRE_SPDLOG_DIR}) + endif() + + find_dependency(spdlog CONFIG NO_DEFAULT_PATH PATHS + ${spdlog_DIR} + ${spdlog_DIR}/lib/cmake/spdlog + ${UMPIRE_PACKAGE_PREFIX_DIR} + ${UMPIRE_PACKAGE_PREFIX_DIR}/lib/cmake/spdlog) + endif () +endif () + if (@UMPIRE_ENABLE_SQLITE_EXPERIMENTAL@) find_package(SQLite3 REQUIRED) endif() From 3fd8ef0a9a9c0f799e3b45f9a5caac6a7efbea73 Mon Sep 17 00:00:00 2001 From: Kristi Belcher Date: Wed, 8 Apr 2026 10:03:18 -0700 Subject: [PATCH 2/8] remove output_buffer_tests stuff --- tests/unit/util/CMakeLists.txt | 9 ------ tests/unit/util/output_buffer_tests.cpp | 40 ------------------------- 2 files changed, 49 deletions(-) delete mode 100644 tests/unit/util/output_buffer_tests.cpp diff --git a/tests/unit/util/CMakeLists.txt b/tests/unit/util/CMakeLists.txt index c0ff67fc8..bc1eb2e53 100644 --- a/tests/unit/util/CMakeLists.txt +++ b/tests/unit/util/CMakeLists.txt @@ -37,15 +37,6 @@ blt_add_test( NAME allocation_map_tests COMMAND allocation_map_tests) -blt_add_executable( - NAME output_buffer_tests - SOURCES output_buffer_tests.cpp - DEPENDS_ON umpire gtest) - -blt_add_test( - NAME output_buffer_tests - COMMAND output_buffer_tests) - blt_add_executable( NAME memory_map_tests SOURCES memory_map_tests.cpp diff --git a/tests/unit/util/output_buffer_tests.cpp b/tests/unit/util/output_buffer_tests.cpp deleted file mode 100644 index 9c3b52ef0..000000000 --- a/tests/unit/util/output_buffer_tests.cpp +++ /dev/null @@ -1,40 +0,0 @@ -////////////////////////////////////////////////////////////////////////////// -// Copyright (c) 2016-26, Lawrence Livermore National Security, LLC and Umpire -// project contributors. See the COPYRIGHT file for details. -// -// SPDX-License-Identifier: (MIT) -////////////////////////////////////////////////////////////////////////////// - -#include - -#include "gtest/gtest.h" -#include "umpire/util/OutputBuffer.hpp" - -class OutputBufferTest : public ::testing::Test { - protected: - OutputBufferTest() - { - d_buffer.setConsoleStream(&d_mock_console_stream); - d_buffer.setFileStream(&d_mock_file_stream); - } - - std::stringstream d_mock_console_stream; - std::stringstream d_mock_file_stream; - - umpire::util::OutputBuffer d_buffer; -}; - -TEST_F(OutputBufferTest, WriteToStreams) -{ - const std::string expected{"TEST"}; - - d_buffer.overflow('T'); - d_buffer.overflow('E'); - d_buffer.overflow('S'); - d_buffer.overflow('T'); - - d_buffer.sync(); - - ASSERT_EQ(d_mock_console_stream.str(), expected); - ASSERT_EQ(d_mock_file_stream.str(), expected); -} From b39aeca025546f6df35f0b37b84e29eea51a4a2d Mon Sep 17 00:00:00 2001 From: Kristi Belcher Date: Thu, 9 Apr 2026 13:48:22 -0700 Subject: [PATCH 3/8] Update io_tests to use spdlog Logger API MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit updates the io_tests to work with the new spdlog-based logging implementation. Changes include: - Replace initialize_io() with Logger::initialize() - Use Logger::log() for test logging instead of umpire::log() - Replace umpire::error() with std::cerr - Add platform-specific environment variable setup for UMPIRE_LOG_LEVEL 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- tests/integration/io/io_tests.cpp | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/tests/integration/io/io_tests.cpp b/tests/integration/io/io_tests.cpp index 3e0b4ed08..37cc544e4 100644 --- a/tests/integration/io/io_tests.cpp +++ b/tests/integration/io/io_tests.cpp @@ -8,6 +8,7 @@ #include "umpire/config.hpp" #include "umpire/util/MPI.hpp" #include "umpire/util/io.hpp" +#include "umpire/util/Logger.hpp" #if defined(UMPIRE_ENABLE_MPI) #include "mpi.h" @@ -33,10 +34,18 @@ int main(int argc, char** argv) CLI11_PARSE(app, argc, argv); - umpire::util::initialize_io(enable_logging); + if (enable_logging) { +#if defined(_MSC_VER) + _putenv_s("UMPIRE_LOG_LEVEL", "Info"); +#else + setenv("UMPIRE_LOG_LEVEL", "Info", 1); +#endif + umpire::util::Logger::initialize(); + umpire::util::Logger::log(umpire::util::message::Info, "testing log stream", __FILE__, __LINE__); + umpire::util::Logger::finalize(); + } - umpire::log() << "testing log stream" << std::endl; - umpire::error() << "testing error stream" << std::endl; + std::cerr << "testing error stream" << std::endl; #if defined(UMPIRE_ENABLE_MPI) MPI_Finalize(); From 9f78cfe0973f84621941dbd76701feee78c83682 Mon Sep 17 00:00:00 2001 From: David Beckingsale Date: Fri, 10 Apr 2026 14:19:07 -0700 Subject: [PATCH 4/8] Fix test --- tests/integration/io/io_tests.cpp | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/tests/integration/io/io_tests.cpp b/tests/integration/io/io_tests.cpp index 37cc544e4..9762d0cb5 100644 --- a/tests/integration/io/io_tests.cpp +++ b/tests/integration/io/io_tests.cpp @@ -8,7 +8,10 @@ #include "umpire/config.hpp" #include "umpire/util/MPI.hpp" #include "umpire/util/io.hpp" + +#if defined(UMPIRE_ENABLE_LOGGING) #include "umpire/util/Logger.hpp" +#endif #if defined(UMPIRE_ENABLE_MPI) #include "mpi.h" @@ -34,6 +37,7 @@ int main(int argc, char** argv) CLI11_PARSE(app, argc, argv); +#if defined(UMPIRE_ENABLE_LOGGING) if (enable_logging) { #if defined(_MSC_VER) _putenv_s("UMPIRE_LOG_LEVEL", "Info"); @@ -42,10 +46,12 @@ int main(int argc, char** argv) #endif umpire::util::Logger::initialize(); umpire::util::Logger::log(umpire::util::message::Info, "testing log stream", __FILE__, __LINE__); + umpire::util::Logger::log(umpire::util::message::Error, "testing error stream", __FILE__, __LINE__); umpire::util::Logger::finalize(); } - - std::cerr << "testing error stream" << std::endl; +#else + (void)enable_logging; +#endif #if defined(UMPIRE_ENABLE_MPI) MPI_Finalize(); From b29671c8518f5d1b8ea57054fcd6ba3b26e38265 Mon Sep 17 00:00:00 2001 From: David Beckingsale Date: Wed, 15 Apr 2026 15:47:56 -0700 Subject: [PATCH 5/8] Fixu test runner script --- tests/integration/io/io_tests_runner.py | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/tests/integration/io/io_tests_runner.py b/tests/integration/io/io_tests_runner.py index 3854a7239..6cb14e4c1 100644 --- a/tests/integration/io/io_tests_runner.py +++ b/tests/integration/io/io_tests_runner.py @@ -64,6 +64,12 @@ def run_io_test(test_env, file_uid, expect_logging): global errors + # Create output directory if specified + if 'UMPIRE_OUTPUT_DIR' in test_env: + output_dir = test_env['UMPIRE_OUTPUT_DIR'] + if not os.path.exists(output_dir): + os.makedirs(output_dir) + cmd_args = ['./io_tests'] if expect_logging: cmd_args.append('--enable-logging') @@ -89,17 +95,19 @@ def run_io_test(test_env, file_uid, expect_logging): print(line) errors = 1 else: - check_output('stderr', error, b'testing error stream') - - output_filename = 'umpire_io_tests.{pid}.{uid}.log'.format(uid=file_uid, pid=pid) - + # Default output directory is "./" when UMPIRE_OUTPUT_DIR is not set if 'UMPIRE_OUTPUT_DIR' in test_env.keys(): output_filename = '{dir}/umpire_io_tests.{pid}.{uid}.log'.format(dir=test_env['UMPIRE_OUTPUT_DIR'], uid=file_uid, pid=pid) + else: + output_filename = './umpire_io_tests.{pid}.{uid}.log'.format(uid=file_uid, pid=pid) if expect_logging: check_file_exists(output_filename) with open(output_filename, 'rb') as output_file: + # With spdlog, both Info and Error messages go to the log file check_output(output_filename, output_file, b'testing log stream') + with open(output_filename, 'rb') as output_file: + check_output(output_filename, output_file, b'testing error stream') else: check_file_not_exists(output_filename) From a3e09b46b753387f628b49707e20c7de3f299150 Mon Sep 17 00:00:00 2001 From: David Beckingsale Date: Wed, 15 Apr 2026 16:16:08 -0700 Subject: [PATCH 6/8] Fix Windows issue --- src/umpire/util/io.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/umpire/util/io.cpp b/src/umpire/util/io.cpp index 6f16b688a..a3459533b 100644 --- a/src/umpire/util/io.cpp +++ b/src/umpire/util/io.cpp @@ -57,7 +57,11 @@ bool directory_exists(const std::string& path) if (stat(path.c_str(), &info)) { return false; } else { +#if defined(_MSC_VER) + return (info.st_mode & _S_IFDIR) != 0; +#else return S_ISDIR(info.st_mode); +#endif } #endif } From a16a34da34b62b4917fe7bb0caa8f5582e6db1e9 Mon Sep 17 00:00:00 2001 From: David Beckingsale Date: Thu, 16 Apr 2026 12:05:23 -0700 Subject: [PATCH 7/8] Fixup tests --- src/umpire/util/Logger.cpp | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/src/umpire/util/Logger.cpp b/src/umpire/util/Logger.cpp index 520098b9c..3e2e81089 100644 --- a/src/umpire/util/Logger.cpp +++ b/src/umpire/util/Logger.cpp @@ -99,9 +99,9 @@ void Logger::initialize() const char* env_enable_log = std::getenv("UMPIRE_LOG_LEVEL"); if (!env_enable_log) { // Logging disabled - create a null logger + // Don't use spdlog::register_logger to avoid static initialization order issues auto null_sink = std::make_shared(); s_logger = std::make_shared("umpire", null_sink); - spdlog::register_logger(s_logger); s_initialized = true; return; } @@ -140,6 +140,7 @@ void Logger::initialize() } // Create logger (sync or async) + // Don't use spdlog::register_logger to avoid static initialization order issues if (use_async) { s_logger = std::make_shared("umpire", sinks.begin(), sinks.end(), spdlog::thread_pool(), @@ -148,9 +149,6 @@ void Logger::initialize() s_logger = std::make_shared("umpire", sinks.begin(), sinks.end()); } - // Register with spdlog - spdlog::register_logger(s_logger); - // Set format pattern to match current output: [LEVEL][file:line]: message s_logger->set_pattern("[%^%L%$][%s:%#]: %v"); @@ -165,18 +163,20 @@ void Logger::initialize() void Logger::finalize() { - if (s_logger) { - s_logger->flush(); - spdlog::drop("umpire"); - s_logger.reset(); - } - - // Shutdown async thread pool if it exists - if (spdlog::thread_pool()) { - spdlog::shutdown(); - } - - s_initialized = false; + // Do nothing during finalize to avoid static destruction order issues. + // When ResourceManager is destroyed during static destruction (e.g., in tests + // that create a static ResourceManager reference), attempting to clean up the + // logger can cause segfaults because: + // 1. spdlog's internal state (sinks, file handles) may already be destroyed + // 2. Even resetting the shared_ptr can crash if the control block is corrupted + // + // It's safe to leak the logger here because: + // - Normal program exit will close all file handles and free all memory + // - The logger's file sink will flush on destruction if still valid + // - Process cleanup handles everything automatically + // + // This is only an issue for tests with static ResourceManager instances. + // Normal usage (where ResourceManager is created/destroyed in main) works fine. } bool Logger::shouldLog(message::Level level) noexcept From 8d02d54a93a356d87b22d740a19dfd5e1db4fff2 Mon Sep 17 00:00:00 2001 From: David Beckingsale Date: Mon, 20 Apr 2026 13:49:56 -0700 Subject: [PATCH 8/8] Try fix Windows tests --- src/tpl/CMakeLists.txt | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/tpl/CMakeLists.txt b/src/tpl/CMakeLists.txt index ad0987ec9..1d305ada1 100644 --- a/src/tpl/CMakeLists.txt +++ b/src/tpl/CMakeLists.txt @@ -155,6 +155,8 @@ if (NOT TARGET ${UMPIRE_FMT_TARGET}) set(UMPIRE_CONFIG_fmt_DIR ${CMAKE_INSTALL_PREFIX} CACHE PATH "") set(FMT_INSTALL ON) set(FMT_SYSTEM_HEADERS ON) + # Force header-only mode to avoid linking issues on Windows with static libraries + set(FMT_HEADER_ONLY ON CACHE BOOL "Use header-only fmt") add_subdirectory(umpire/fmt) endif () endif () @@ -182,15 +184,17 @@ if (NOT TARGET spdlog::spdlog) # Configure spdlog to use external fmt (if available) if (TARGET ${UMPIRE_FMT_TARGET}) set(SPDLOG_FMT_EXTERNAL ON CACHE BOOL "Use external fmt library") - set(SPDLOG_FMT_EXTERNAL_HO OFF CACHE BOOL "Use external fmt header-only library") # Help spdlog find the fmt package by setting fmt_DIR # This works whether fmt is external or from our submodule if (DEFINED fmt_DIR) # External fmt case - use the user-provided path + # User must set SPDLOG_FMT_EXTERNAL_HO themselves if their fmt is header-only set(fmt_DIR ${fmt_DIR} CACHE PATH "Path to fmt") else () # Submodule fmt case - point to build directory where fmt exports its config + # We built fmt as header-only, so tell spdlog + set(SPDLOG_FMT_EXTERNAL_HO ON CACHE BOOL "Use external fmt header-only library") set(fmt_DIR ${CMAKE_BINARY_DIR}/src/tpl/umpire/fmt CACHE PATH "Path to fmt") endif () else ()