refactor: moved debugging state into engine base class, added UCI option for sanitize positions

Author: benthevining

libbenbot/include/libbenbot/engine/Engine.hpp

@@ -80,11 +80,6 @@ class [[nodiscard]] Engine final : public uci::EngineBase {
 
     auto is_searching() const noexcept -> bool override { return searcher.context.in_progress(); }
 
-    void set_debug(const bool shouldDebug) override
-    {
-        debugMode.store(shouldDebug, std::memory_order::relaxed);
-    }
-
     [[nodiscard]] auto get_options() -> std::span<uci::Option*> override { return options; }
 
     [[nodiscard]] auto get_custom_uci_commands() const noexcept -> CommandList override
@@ -117,8 +112,6 @@ class [[nodiscard]] Engine final : public uci::EngineBase {
 
     [[nodiscard]] static auto create_move_format_option() -> uci::ComboOption;
 
-    std::atomic_bool debugMode { false };
-
     search::Thread searcher;
 
     /* ----- UCI options ----- */
@@ -179,8 +172,15 @@ class [[nodiscard]] Engine final : public uci::EngineBase {
 
     uci::ComboOption moveFormat { create_move_format_option() };
 
-    std::array<uci::Option*, 8uz> options {
-        &ttSize, &clearTT, &ponder, &threads, &moveOverhead, &logFile, &prettyPrintMode, &moveFormat
+    uci::BoolOption sanitizePositions {
+        "Sanitize Positions",
+        false,
+        "When on, the engine checks if the position is legal before setting it.",
+        [this](const bool sanitize) { set_sanitize_positions(sanitize); }
+    };
+
+    std::array<uci::Option*, 9uz> options {
+        &ttSize, &clearTT, &ponder, &threads, &moveOverhead, &logFile, &prettyPrintMode, &moveFormat, &sanitizePositions
     };
 
     std::array<EngineCommand, 10uz> customCommands {

libbenbot/src/engine/Bench.cpp

@@ -211,7 +211,7 @@ void Engine::run_bench(const string_view arguments) const
 
         do_bench(
             resources::get_bench_epd_text(),
-            defaultDepth, debugMode.load());
+            defaultDepth, is_debug_mode());
 
         return;
     }
@@ -223,7 +223,7 @@ void Engine::run_bench(const string_view arguments) const
               .transform([this, defaultDepth, absPathStr = epdPath.string()](const string_view fileContent) {
                   info_string(std::format("Running bench for {}...", absPathStr));
 
-                  do_bench(fileContent, defaultDepth, debugMode.load());
+                  do_bench(fileContent, defaultDepth, is_debug_mode());
               })
               .transform_error(info_string);
 }

libbenbot/src/engine/Engine.cpp

@@ -89,9 +89,7 @@ void Engine::set_pretty_printing(const bool shouldPrettyPrint)
             [this](const Move move) { return pretty_print_move(move); }));
     } else {
         searcher.context.set_callbacks(search::Callbacks::make_uci_printer(
-            [this]() noexcept {
-                return debugMode.load(memory_order_relaxed);
-            }));
+            [this]() noexcept { return is_debug_mode(); }));
     }
 }
 
@@ -163,7 +161,7 @@ void Engine::write_config_file(const string_view arg) const
     json data;
 
     data[TAG_OPTIONS] = optionsData;
-    data[TAG_DEBUG]   = debugMode.load(memory_order_relaxed);
+    data[TAG_DEBUG]   = is_debug_mode();
 
     const auto filePath = absolute(path { arg });
 
@@ -196,9 +194,8 @@ void Engine::read_config_file(const path& file)
               .transform([this, &filePath](const string_view fileContent) {
                   const auto data = json::parse(fileContent);
 
-                  debugMode.store(
-                      data.at(TAG_DEBUG).get<bool>(),
-                      memory_order_relaxed);
+                  set_debug_mode(
+                      data.at(TAG_DEBUG).get<bool>());
 
                   const auto& optionsData = data.at(TAG_OPTIONS);
 

libbenbot/src/engine/Printing.cpp

@@ -132,7 +132,7 @@ void Engine::print_options() const
     print_colored_table(table);
 
     println("");
-    println("Debug mode: {}", debugMode.load());
+    println("Debug mode: {}", is_debug_mode());
 
     std::cout.flush();
 }

libchess/include/libchess/uci/EngineBase.hpp

@@ -20,6 +20,7 @@
 #pragma once
 
 #include <array>
+#include <atomic>
 #include <functional>
 #include <libchess/uci/CommandParsing.hpp>
 #include <span>
@@ -74,6 +75,8 @@ struct EngineCommand final {
     }
 };
 
+using std::memory_order_relaxed;
+
 /** A base class for UCI @cite Meyer-Kahlen_2006 chess engines.
 
     This class provides handling of UCI command parsing, so that
@@ -92,10 +95,10 @@ struct EngineBase {
 
     virtual ~EngineBase();
 
-    EngineBase(const EngineBase&)            = default;
-    EngineBase(EngineBase&&)                 = default;
-    EngineBase& operator=(const EngineBase&) = default;
-    EngineBase& operator=(EngineBase&&)      = default;
+    EngineBase(const EngineBase&)            = delete;
+    EngineBase(EngineBase&&)                 = delete;
+    EngineBase& operator=(const EngineBase&) = delete;
+    EngineBase& operator=(EngineBase&&)      = delete;
 
     /** This must return the name of the engine.
         The returned string may optionally contain the engine's current version,
@@ -148,9 +151,6 @@ struct EngineBase {
      */
     virtual void go([[maybe_unused]] const GoCommandOptions& opts) = 0;
 
-    /** Called when the "debug" command is received. */
-    virtual void set_debug([[maybe_unused]] bool shouldDebug) { }
-
     /** Handles a UCI command.
         Typically you will not call this directly, you'll just invoke ``loop()``, but this
         method can be used to manually invoke UCI commands if needed.
@@ -187,6 +187,30 @@ struct EngineBase {
         return { };
     }
 
+    /** Returns true if the engine's debugging mode is active. */
+    [[nodiscard]] auto is_debug_mode() const noexcept -> bool
+    {
+        return debugMode.load(memory_order_relaxed);
+    }
+
+    /** Sets the engine's debugging mode. */
+    void set_debug_mode(const bool shouldDebug) noexcept
+    {
+        debugMode.store(shouldDebug, memory_order_relaxed);
+    }
+
+    /** Tells the base class whether to sanitize incoming positions when processing
+        the ``position`` command.
+        When sanitizing is on, after evaluating each ``position`` command, the engine
+        performs some basic checks to determine if the position is illegal, and if so,
+        prints a diagnostic message and reverts the internal board to the previous
+        position.
+     */
+    void set_sanitize_positions(const bool shouldSanitize) noexcept
+    {
+        sanitizeIncomingPositions.store(shouldSanitize, memory_order_relaxed);
+    }
+
 private:
     void respond_to_uci();
     void respond_to_isready();
@@ -195,9 +219,14 @@ struct EngineBase {
     void handle_setpos(string_view arguments);
     void handle_setoption(string_view arguments);
 
-    bool shouldExit { false }; // used as flag for exiting the loop() function
+    static_assert(
+        std::atomic_bool::is_always_lock_free,
+        "Platform doesn't support lock-free atomic operations");
 
-    bool initialized { false };
+    std::atomic_bool shouldExit { false }; // used as flag for exiting the loop() function
+    std::atomic_bool initialized { false };
+    std::atomic_bool debugMode { false };
+    std::atomic_bool sanitizeIncomingPositions { false };
 
     Position position;
 
@@ -249,7 +278,7 @@ struct EngineBase {
             .argsHelp    = "[name <name>] [value <value>]" },
         EngineCommand {
             .name   = "debug",
-            .action = [this](const string_view args) { set_debug(args == "on"); },
+            .action = [this](const string_view args) noexcept { set_debug_mode(args == "on"); },
             .description = "Enable/disable engine debug mode",
             .argsHelp    = "[on|off]" },
         EngineCommand {

libchess/src/uci/EngineBase.cpp

@@ -13,6 +13,7 @@
  */
 
 #include <algorithm>
+#include <atomic>
 #include <expected>
 #include <format>
 #include <iostream>
@@ -31,6 +32,7 @@ namespace chess::uci {
 
 using printing::info_string;
 using std::cout;
+using std::memory_order_relaxed;
 using std::println;
 using std::string_view;
 using util::strings::split_at_first_space;
@@ -68,7 +70,6 @@ void EngineBase::handle_command(const string_view command)
     }
 
     info_string(std::format("Unknown UCI command: '{}'", firstWord));
-    info_string("Type help for a list of supported commands");
 }
 
 void EngineBase::respond_to_uci()
@@ -99,15 +100,15 @@ void EngineBase::respond_to_isready()
 
 void EngineBase::respond_to_newgame()
 {
-    const bool wasInitialized = std::exchange(initialized, true);
+    const bool wasInitialized = initialized.exchange(true, memory_order_relaxed);
 
     new_game(not wasInitialized);
 }
 
 void EngineBase::handle_quit()
 {
     abort_search();
-    shouldExit = true; // exit the event loop
+    shouldExit.store(true, std::memory_order::release);
     wait();
 }
 
@@ -121,13 +122,10 @@ void EngineBase::handle_setpos(const string_view arguments)
     // we print an error message via `info string` and keep the old position.
     // See this Stockfish PR discussion: https://github.com/official-stockfish/Stockfish/pull/4563
 
-    // NB. enabling this check seems to cost about 8 ELO
-    static constexpr bool SanitizeIncomingPositions = false;
-
     [[maybe_unused]] const auto obj
         = parse_position_options(arguments)
               .and_then([this](const Position& pos) -> std::expected<void, std::string> {
-                  if constexpr (SanitizeIncomingPositions) {
+                  if (sanitizeIncomingPositions.load(memory_order_relaxed)) {
                       if (const auto errorStr = pos.is_illegal()) {
                           [[unlikely]];
                           return std::unexpected {
@@ -200,7 +198,7 @@ void EngineBase::loop()
         std::getline(std::cin, inputBuf);
 
         handle_command(inputBuf);
-    } while (not shouldExit);
+    } while (not shouldExit.load(std::memory_order::acquire));
 }
 
 } // namespace chess::uci