Skip to content

Commit

Permalink
spdlog: do not drop logger in destructor
Browse files Browse the repository at this point in the history
For static objects, the order of destruction is in reverse of the order
of construction, but the latter is hard to control except among static
objects defined within the same translation unit. This means that spdlog
may be destructed before spdlog::drop() is invoked in the destructor,
which leads to a segmentation fault due to memory use-after-free.

Move call of spdlog::drop() from destructor to main() function, to
ensure the logger may be re-registered with the same name in tests.
Use Scope Guard pattern to implement RAII for logger registration.

Link: ros2/rclcpp#933
Link: gabime/spdlog#1738
Link: gabime/spdlog#2113
Link: https://en.wikibooks.org/wiki/More_C++_Idioms/Scope_Guard
Closes: https://hsdes.intel.com/appstore/article/#/22019839238
Signed-off-by: Peter Colberg <peter.colberg@intel.com>
  • Loading branch information
pcolberg committed Apr 8, 2024
1 parent b96e35c commit 588c4dc
Show file tree
Hide file tree
Showing 3 changed files with 57 additions and 8 deletions.
22 changes: 19 additions & 3 deletions libraries/afu-test/afu_test.h
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,24 @@ inline std::vector<std::string> spdlog_levels() {
inline std::vector<std::string> spdlog_levels() { return SPDLOG_LEVEL_NAMES; }
#endif // SPDLOG_VERSION

// Dropping a logger is optional, but not doing so will cause an exception
// when re-registering a logger with the same name, e.g., in test runs.
class scoped_register_logger {
public:
scoped_register_logger(std::shared_ptr<spdlog::logger> logger)
: logger_(std::move(logger)) {
spdlog::register_logger(logger_);
}

~scoped_register_logger() { spdlog::drop(logger_->name()); }

scoped_register_logger(scoped_register_logger const &) = delete;
void operator=(scoped_register_logger const &) = delete;

private:
std::shared_ptr<spdlog::logger> logger_;
};

class afu {
public:
typedef int (*command_fn)(afu *afu, CLI::App *app);
Expand Down Expand Up @@ -186,8 +204,6 @@ class afu {
app_.add_option("-t,--timeout", timeout_msec_, "test timeout (msec)")->default_str(std::to_string(timeout_msec_));
}
virtual ~afu() {
if (logger_)
spdlog::drop(logger_->name());
}

CLI::App & cli() { return app_; }
Expand Down Expand Up @@ -310,7 +326,7 @@ class afu {

auto console_sink = std::make_shared<spdlog::sinks::stdout_color_sink_mt>();
logger_ = std::make_shared<spdlog::logger>(test->name(), console_sink);
spdlog::register_logger(logger_);
scoped_register_logger register_logger(logger_);
logger_->set_level(spdlog::level::from_str(log_level_));

int res = open_handle(test->afu_id());
Expand Down
21 changes: 19 additions & 2 deletions samples/cxl_hello_fpga/he_cache_test.h
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,24 @@ inline std::vector<std::string> spdlog_levels() {
inline std::vector<std::string> spdlog_levels() { return SPDLOG_LEVEL_NAMES; }
#endif // SPDLOG_VERSION

// Dropping a logger is optional, but not doing so will cause an exception
// when re-registering a logger with the same name, e.g., in test runs.
class scoped_register_logger {
public:
scoped_register_logger(std::shared_ptr<spdlog::logger> logger)
: logger_(std::move(logger)) {
spdlog::register_logger(logger_);
}

~scoped_register_logger() { spdlog::drop(logger_->name()); }

scoped_register_logger(scoped_register_logger const &) = delete;
void operator=(scoped_register_logger const &) = delete;

private:
std::shared_ptr<spdlog::logger> logger_;
};

class afu {
public:
typedef int (*command_fn)(afu *afu, CLI::App *app);
Expand Down Expand Up @@ -297,7 +315,6 @@ class afu {
}
virtual ~afu() {
if (fd_ > 0) close(fd_);
if (logger_) spdlog::drop(logger_->name());
}

CLI::App &cli() { return app_; }
Expand Down Expand Up @@ -449,7 +466,7 @@ class afu {

auto console_sink = std::make_shared<spdlog::sinks::stdout_color_sink_mt>();
logger_ = std::make_shared<spdlog::logger>(test->name(), console_sink);
spdlog::register_logger(logger_);
scoped_register_logger register_logger(logger_);
logger_->set_level(spdlog::level::from_str(log_level_));
current_command_ = test;
if (find_dev_feature() != 0) {
Expand Down
22 changes: 19 additions & 3 deletions samples/cxl_host_exerciser/he_cache_test.h
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,24 @@ inline std::vector<std::string> spdlog_levels() {
inline std::vector<std::string> spdlog_levels() { return SPDLOG_LEVEL_NAMES; }
#endif // SPDLOG_VERSION

// Dropping a logger is optional, but not doing so will cause an exception
// when re-registering a logger with the same name, e.g., in test runs.
class scoped_register_logger {
public:
scoped_register_logger(std::shared_ptr<spdlog::logger> logger)
: logger_(std::move(logger)) {
spdlog::register_logger(logger_);
}

~scoped_register_logger() { spdlog::drop(logger_->name()); }

scoped_register_logger(scoped_register_logger const &) = delete;
void operator=(scoped_register_logger const &) = delete;

private:
std::shared_ptr<spdlog::logger> logger_;
};

class afu {
public:
typedef int (*command_fn)(afu *afu, CLI::App *app);
Expand Down Expand Up @@ -304,8 +322,6 @@ class afu {

if (fd_ > 0)
close(fd_);
if (logger_)
spdlog::drop(logger_->name());
}

CLI::App &cli() { return app_; }
Expand Down Expand Up @@ -459,7 +475,7 @@ class afu {

auto console_sink = std::make_shared<spdlog::sinks::stdout_color_sink_mt>();
logger_ = std::make_shared<spdlog::logger>(test->name(), console_sink);
spdlog::register_logger(logger_);
scoped_register_logger register_logger(logger_);
logger_->set_level(spdlog::level::from_str(log_level_));
current_command_ = test;
if (find_dev_feature() != 0) {
Expand Down

0 comments on commit 588c4dc

Please sign in to comment.