From 4524506fc34886e794e1405757af606622206b05 Mon Sep 17 00:00:00 2001 From: Jo Voordeckers Date: Tue, 22 Oct 2024 13:23:43 -0700 Subject: [PATCH 1/3] Address thread-safery issue by removing backtrace logger --- include/flexi_cfg/logger.h | 22 ------------- tests/config_parse_test.cpp | 61 +++++++++++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+), 22 deletions(-) diff --git a/include/flexi_cfg/logger.h b/include/flexi_cfg/logger.h index 771f06b..e220f2b 100644 --- a/include/flexi_cfg/logger.h +++ b/include/flexi_cfg/logger.h @@ -26,33 +26,15 @@ class Logger { void setLevel(Severity level) { log_level_ = level; } [[nodiscard]] auto logLevel() const -> Severity { return log_level_; } - void setMaxHistory(std::size_t max_history) { max_history_ = max_history; } - void clearHistory() { log_history_.clear(); } - template void log(Severity level, std::string_view msg_f, Args&&... args) { const auto msg = fmt::vformat(msg_f, fmt::make_format_args(args...)); - log_history_.emplace_back(level, msg); - if (log_history_.size() > max_history_) { - log_history_.pop_front(); - } if (level >= log_level_) { // NOTE: The clear format sequence shouldn't be necessary, but appears to be. fmt::print(fg_color_.at(level), "[{}] {}\x1b[0m\n", level, msg); } } - /// \brief Prints the history of messages (ignoring log level) - /// \param[in] clear - Flag to indicate if history should be cleared after printing - void backtrace(bool clear = true) { - for (const auto& msg : log_history_) { - fmt::print(fg_color_.at(msg.first), "[{}] {}\x1b[0m\n", msg.first, msg.second); - } - if (clear) { - clearHistory(); - } - } - private: Severity log_level_{Severity::INFO}; @@ -65,15 +47,11 @@ class Logger { {Severity::CRITICAL, fmt::emphasis::bold | fmt::fg(fmt::color::orange_red)}}; // This container holds a history of messages to provide a type of "backtrace" functionality - std::size_t max_history_{15}; // NOLINT - std::deque> log_history_; }; static void setLevel(Severity lvl) { Logger::instance().setLevel(lvl); } static auto logLevel() -> Severity { return Logger::instance().logLevel(); } -static void backtrace(bool clear = true) { Logger::instance().backtrace(clear); } - template static void log(Severity level, std::string_view msg_f, Args&&... args) { Logger::instance().log(level, msg_f, std::forward(args)...); diff --git a/tests/config_parse_test.cpp b/tests/config_parse_test.cpp index 867c32d..ff47c48 100644 --- a/tests/config_parse_test.cpp +++ b/tests/config_parse_test.cpp @@ -4,6 +4,7 @@ #include #include #include +#include #include "flexi_cfg/config/actions.h" #include "flexi_cfg/config/grammar.h" @@ -100,6 +101,66 @@ TEST_P(InputString, Reader) { } } +// Ensure we don't get optimized out and force "consuming" the value +template +__attribute__((optimize("O0"))) auto consume(const T& value) -> T { + return value; +} + +// Multi-threaded map access shouldn't segfault +TEST_P(InputString, MultiThreadedConfigAccess) { + setLevel(flexi_cfg::logger::Severity::INFO); + flexi_cfg::logger::info("MultiThreaded Test Start"); + flexi_cfg::Reader cfg({}, ""); // Nominally, we wouldn't do this, but we need a mechanism to + EXPECT_NO_THROW(cfg = flexi_cfg::Parser::parseFromString(GetParam(), "From String")); + auto random_read = [&cfg](const std::stop_token& stop) { + while (!stop.stop_requested()) { + const auto& vec = cfg.getValue>("test2.inner.list"); + for (const auto& v : vec) { + consume(v); + } + for (const auto& k : cfg.keys()) { + ASSERT_TRUE(cfg.exists(k)); + switch (cfg.getType(k)) { + case flexi_cfg::config::types::Type::kString: + consume(cfg.getValue(k)); + break; + case flexi_cfg::config::types::Type::kNumber: + consume(cfg.getValue(k)); + break; + case flexi_cfg::config::types::Type::kBoolean: + consume(cfg.getValue(k)); + break; + case flexi_cfg::config::types::Type::kList: + consume(cfg.getValue>(k)); + break; + case flexi_cfg::config::types::Type::kExpression: + case flexi_cfg::config::types::Type::kValueLookup: + case flexi_cfg::config::types::Type::kVar: + case flexi_cfg::config::types::Type::kStruct: + case flexi_cfg::config::types::Type::kStructInProto: + case flexi_cfg::config::types::Type::kProto: + case flexi_cfg::config::types::Type::kReference: + case flexi_cfg::config::types::Type::kUnknown: + case flexi_cfg::config::types::Type::kValue: + // These aren't values + break; + } + } + } + }; + + std::jthread t1{random_read}; + std::jthread t2{random_read}; + std::jthread t3{random_read}; + std::jthread t4{random_read}; + std::jthread t5{random_read}; + std::jthread t6{random_read}; + + std::this_thread::sleep_for(std::chrono::seconds(10)); // NOLINT + flexi_cfg::logger::info("MultiThreaded Test Done"); +} + INSTANTIATE_TEST_SUITE_P(ConfigParse, InputString, testing::Values(std::string(R"( struct test1 { From aa96344d632d499f5e727f79d9c9bbc55cfa498e Mon Sep 17 00:00:00 2001 From: Jo Voordeckers Date: Tue, 22 Oct 2024 15:37:32 -0700 Subject: [PATCH 2/3] compatibility with all compilers --- tests/config_parse_test.cpp | 42 +++++++++++++++++++++++-------------- 1 file changed, 26 insertions(+), 16 deletions(-) diff --git a/tests/config_parse_test.cpp b/tests/config_parse_test.cpp index ff47c48..b7b1cfd 100644 --- a/tests/config_parse_test.cpp +++ b/tests/config_parse_test.cpp @@ -1,6 +1,8 @@ #include +#include #include +#include #include #include #include @@ -102,23 +104,26 @@ TEST_P(InputString, Reader) { } // Ensure we don't get optimized out and force "consuming" the value +#pragma GCC push_options +#pragma GCC optimize("-O0") +#pragma clang optnone push template -__attribute__((optimize("O0"))) auto consume(const T& value) -> T { +auto consume(const T& value) -> T { return value; } +#pragma clang optnone pop +#pragma GCC pop_options // Multi-threaded map access shouldn't segfault TEST_P(InputString, MultiThreadedConfigAccess) { setLevel(flexi_cfg::logger::Severity::INFO); + std::atomic_bool done{false}; + constexpr auto thread_count = 6; flexi_cfg::logger::info("MultiThreaded Test Start"); flexi_cfg::Reader cfg({}, ""); // Nominally, we wouldn't do this, but we need a mechanism to EXPECT_NO_THROW(cfg = flexi_cfg::Parser::parseFromString(GetParam(), "From String")); - auto random_read = [&cfg](const std::stop_token& stop) { - while (!stop.stop_requested()) { - const auto& vec = cfg.getValue>("test2.inner.list"); - for (const auto& v : vec) { - consume(v); - } + auto random_read = [&cfg, &done] { + while (!done) { for (const auto& k : cfg.keys()) { ASSERT_TRUE(cfg.exists(k)); switch (cfg.getType(k)) { @@ -131,9 +136,12 @@ TEST_P(InputString, MultiThreadedConfigAccess) { case flexi_cfg::config::types::Type::kBoolean: consume(cfg.getValue(k)); break; - case flexi_cfg::config::types::Type::kList: - consume(cfg.getValue>(k)); + case flexi_cfg::config::types::Type::kList: { + for (const auto& vec = cfg.getValue>(k); const auto& v : vec) { + consume(v); + } break; + } case flexi_cfg::config::types::Type::kExpression: case flexi_cfg::config::types::Type::kValueLookup: case flexi_cfg::config::types::Type::kVar: @@ -150,14 +158,16 @@ TEST_P(InputString, MultiThreadedConfigAccess) { } }; - std::jthread t1{random_read}; - std::jthread t2{random_read}; - std::jthread t3{random_read}; - std::jthread t4{random_read}; - std::jthread t5{random_read}; - std::jthread t6{random_read}; - + std::vector threads; + threads.reserve(thread_count); + for (auto i = 0; i < thread_count; i++) { + threads.emplace_back(random_read); + } std::this_thread::sleep_for(std::chrono::seconds(10)); // NOLINT + done = true; + for (auto& t : threads) { + t.join(); + } flexi_cfg::logger::info("MultiThreaded Test Done"); } From 2872071892cfbdca7c318441d49010fc71353ea7 Mon Sep 17 00:00:00 2001 From: Jo Voordeckers Date: Tue, 22 Oct 2024 16:37:27 -0700 Subject: [PATCH 3/3] Moving to ubuntu22 for clang-13 --- .github/workflows/cmake.yml | 6 +- Dockerfile | 122 ------------------------------------ compose.yml | 10 +-- 3 files changed, 4 insertions(+), 134 deletions(-) diff --git a/.github/workflows/cmake.yml b/.github/workflows/cmake.yml index cbff088..26c1c11 100644 --- a/.github/workflows/cmake.yml +++ b/.github/workflows/cmake.yml @@ -25,7 +25,7 @@ jobs: name: [ ubuntu-22.04-gcc-12, - ubuntu-20.04-clang-13, + ubuntu-22.04-clang-13, ubuntu-22.04-clang-14, ubuntu-22.04-clang-15, mac-os-latest @@ -38,8 +38,8 @@ jobs: compiler: gcc version: "12" - - name: ubuntu-20.04-clang-13 - os: ubuntu-20.04 + - name: ubuntu-22.04-clang-13 + os: ubuntu-22.04 compiler: clang version: "13" diff --git a/Dockerfile b/Dockerfile index 95a3135..0cada05 100644 --- a/Dockerfile +++ b/Dockerfile @@ -1,115 +1,3 @@ -# We only run clang-13 on 20.04 (and whatever ships on its libc), the other compilers run on 22.04 -# unfortunately we have to duplicate the postprocessing steps -FROM ubuntu:20.04 AS base-ubuntu20 -ENV DEBIAN_FRONTEND noninteractive -RUN apt-get update && apt-get install -y --no-install-recommends ca-certificates wget gpg -RUN wget -qO- https://apt.llvm.org/llvm-snapshot.gpg.key | tee /etc/apt/trusted.gpg.d/apt.llvm.org.asc -RUN echo 'deb http://apt.llvm.org/focal/ llvm-toolchain-focal-13 main' | tee -a /etc/apt/sources.list.d/llvm.list -RUN wget -O - https://apt.kitware.com/keys/kitware-archive-latest.asc 2> /dev/null \ - | gpg --dearmor - | tee /usr/share/keyrings/kitware-archive-keyring.gpg >/dev/null -RUN echo 'deb [signed-by=/usr/share/keyrings/kitware-archive-keyring.gpg] https://apt.kitware.com/ubuntu/ focal main' \ - | tee /etc/apt/sources.list.d/kitware.list -RUN apt-get update && apt-get install -y --no-install-recommends \ - sudo \ - build-essential \ - git \ - gdb \ - python3 \ - python3-dev \ - python3-pip \ - python3-pybind11 \ - python3-pybindgen \ - pybind11-dev \ - libc++-13-dev \ - libc++abi-13-dev \ - clang-13 \ - clang-format-13 \ - clang-tidy-13 \ - lld-13 \ - ninja-build \ - cmake - -# project root will be mounted at /usr/src/flexi_cfg -COPY < /etc/sudoers.d/$USERNAME \ - && chmod 0440 /etc/sudoers.d/$USERNAME - -# compose mounts volumes as root -COPY < # runs tests" -echo -e " tests/utils_test # run any gtest directly" -echo -e " gdb tests/utils_test # run with gdb" -echo -e "=======================================================================================" -EOF - -USER ${USERNAME} -WORKDIR /usr/src/flexi_cfg_build - -ENV PATH=${PATH}:/home/${USERNAME}/.local/bin -ENV TEST_DIR=env - -ENTRYPOINT ["/usr/bin/entrypoint"] - -# ---------------------------------------------------------------------------------------------------------------- - FROM ubuntu:22.04 AS base ENV DEBIAN_FRONTEND noninteractive RUN apt-get update && apt-get install -y --no-install-recommends ca-certificates wget gpg @@ -226,16 +114,6 @@ ENTRYPOINT ["/usr/bin/entrypoint"] # ---------------------------------------------------------------------------------------------------------------- -# 20.04 for clang-13 -FROM base-ubuntu20 AS clang-13-ubuntu20 -ENV CC=/usr/bin/clang-13 -ENV CXX=/usr/bin/clang++-13 -# clang-13 libc++ for C++20 (e.g. ) -ENV CXXFLAGS="-stdlib=libc++ -fno-omit-frame-pointer -mno-omit-leaf-frame-pointer" -ENV LDFLAGS="-stdlib=libc++ -g -fuse-ld=/usr/bin/ld.lld-13" -ENV CLANG_TOOLS_PATH=/usr/lib/llvm-13 -CMD ["/usr/bin/build-and-test"] - FROM base AS gcc-12 ENV CC=/usr/bin/gcc-12 ENV CXX=/usr/bin/g++-12 diff --git a/compose.yml b/compose.yml index d939511..737307d 100644 --- a/compose.yml +++ b/compose.yml @@ -1,7 +1,7 @@ # # This helps running all the tests and checks from various compilers we support # -# usage: docker compose up --build -d # checks all compilers +# usage: docker compose up --build --force-recreate -d # checks all compilers # docker compose run --rm clang-14 bash -c "build-and-test;bash" # interactive # docker compose logs -f gcc-12 # gcc-12 build logs # docker compose run gcc-12 bash # shell into gcc-12 to run interactively @@ -47,14 +47,6 @@ services: target: clang-13 volumes: [ "${HOME}/.cache/flexi_cfg_build_deps/clang-13:/usr/src/flexi_cfg_build/_deps" ] - clang-13-ubuntu20: - extends: - service: base - container_name: clang-13-ubuntu20 - build: - target: clang-13-ubuntu20 - volumes: [ "${HOME}/.cache/flexi_cfg_build_deps/clang-13:/usr/src/flexi_cfg_build/_deps" ] - clang-14: extends: service: base