Ensure filtered fatal logs abort

Author: SineStriker

include/stdcorelib/support/logging.h

@@ -128,6 +128,8 @@ namespace stdc {
     /// Each category registers itself on construction and picks up whatever filter rules are
     /// already in effect.
     ///
+    /// Disabling the fatal level suppresses its record but does not suppress process termination.
+    ///
     /// \sa setFilterRules()
     class STDC_EXPORT LogCategory {
     public:
@@ -187,11 +189,10 @@ namespace stdc {
         template <int Level, class... Args>
         void log(const char *fileName, int lineNumber, const char *functionName,
                  const std::string_view &format, Args &&...args) const {
-            if (!isLevelEnabled(Level)) {
-                return;
+            if (isLevelEnabled(Level)) {
+                Logger(fileName, lineNumber, functionName, _name)
+                    .log(Level, format, std::forward<Args>(args)...);
             }
-            Logger(fileName, lineNumber, functionName, _name)
-                .log(Level, format, std::forward<Args>(args)...);
             if constexpr (Level == stdc::Logger::Fatal) {
                 Logger::abort();
             }
@@ -200,11 +201,10 @@ namespace stdc {
         template <int Level, class... Args>
         void logf(const char *fileName, int lineNumber, const char *functionName, const char *fmt,
                   Args &&...args) const {
-            if (!isLevelEnabled(Level)) {
-                return;
+            if (isLevelEnabled(Level)) {
+                Logger(fileName, lineNumber, functionName, _name)
+                    .printf(Level, fmt, std::forward<Args>(args)...);
             }
-            Logger(fileName, lineNumber, functionName, _name)
-                .printf(Level, fmt, std::forward<Args>(args)...);
             if constexpr (Level == stdc::Logger::Fatal) {
                 Logger::abort();
             }

tests/auto/CMakeLists.txt

@@ -141,8 +141,18 @@ set_target_properties(test_popen_child PROPERTIES
     RUNTIME_OUTPUT_DIRECTORY ${CMAKE_RUNTIME_OUTPUT_DIRECTORY}
 )
 
+# Fatal logging has to run in a child because the behavior under test terminates its process.
+add_executable(test_logging_fatal helpers/test_logging_fatal.cpp)
+target_link_libraries(test_logging_fatal PRIVATE stdcorelib)
+set_target_properties(test_logging_fatal PROPERTIES
+    CXX_EXTENSIONS OFF
+    CXX_STANDARD 17
+    CXX_STANDARD_REQUIRED ON
+    RUNTIME_OUTPUT_DIRECTORY ${CMAKE_RUNTIME_OUTPUT_DIRECTORY}
+)
+
 add_dependencies(${PROJECT_NAME}
-    test_any_plugin test_dynamicregistry_plugin test_sharedlibrary_unloadable test_sharedlibrary_pinned test_popen_child
+    test_any_plugin test_dynamicregistry_plugin test_sharedlibrary_unloadable test_sharedlibrary_pinned test_popen_child test_logging_fatal
     test_sharedlibrary_dependency test_sharedlibrary_needs_dependency)
 target_compile_definitions(${PROJECT_NAME} PRIVATE
     TEST_ANY_PLUGIN_PATH="$<TARGET_FILE:test_any_plugin>"
@@ -151,6 +161,7 @@ target_compile_definitions(${PROJECT_NAME} PRIVATE
     TEST_SHAREDLIBRARY_PINNED_PATH="$<TARGET_FILE:test_sharedlibrary_pinned>"
     TEST_SHAREDLIBRARY_NEEDS_DEPENDENCY_PATH="$<TARGET_FILE:test_sharedlibrary_needs_dependency>"
     TEST_POPEN_CHILD_PATH="$<TARGET_FILE:test_popen_child>"
+    TEST_LOGGING_FATAL_PATH="$<TARGET_FILE:test_logging_fatal>"
 )
 
 # One entry rather than one per suite. The whole binary is a second, and twenty five process

tests/auto/helpers/test_logging_fatal.cpp

@@ -0,0 +1,24 @@
+// SPDX-License-Identifier: MIT
+
+#include <cstring>
+
+#include <stdcorelib/support/logging.h>
+
+using namespace stdc;
+
+int main(int argc, char *argv[]) {
+    if (argc != 2) {
+        return 2;
+    }
+
+    LogCategory category("stdc.fatal.child");
+    category.setFilterRules("stdc.fatal.child.fatal=false");
+
+    if (std::strcmp(argv[1], "log") == 0) {
+        category.log<Logger::Fatal>(__FILE__, __LINE__, __FUNCTION__, "filtered fatal");
+    }
+    if (std::strcmp(argv[1], "logf") == 0) {
+        category.logf<Logger::Fatal>(__FILE__, __LINE__, __FUNCTION__, "filtered %s", "fatal");
+    }
+    return 0;
+}

tests/auto/support/test_logging.cpp

@@ -6,6 +6,7 @@
 #include <vector>
 
 #include <stdcorelib/support/logging.h>
+#include <stdcorelib/support/popen.h>
 
 #include <boost/test/unit_test.hpp>
 
@@ -79,6 +80,15 @@ namespace {
         g_lastContext = context;
     }
 
+    bool filteredFatalTerminates(const char *mode) {
+        Popen process;
+        process.args({TEST_LOGGING_FATAL_PATH, mode})
+            .standardOutput(Popen::DeviceNull)
+            .standardError(Popen::DeviceNull);
+        return process.start() && process.wait(5000) && process.returnCode() &&
+               *process.returnCode() != 0;
+    }
+
     // Redirects stdout and stderr into a scratch file for as long as it lives, and hands back
     // what was written. The default sink writes to them directly, so this is the only way to see
     // what it produced.
@@ -350,6 +360,11 @@ BOOST_AUTO_TEST_CASE(test_disabled_level_never_reaches_the_callback) {
     BOOST_CHECK_EQUAL(lastLevel, int(Logger::Warning));
 }
 
+BOOST_AUTO_TEST_CASE(test_filtered_fatal_still_terminates) {
+    BOOST_CHECK(filteredFatalTerminates("log"));
+    BOOST_CHECK(filteredFatalTerminates("logf"));
+}
+
 // The macros resolve an in-scope category through stdcGetLogCategory(), and fall back to the
 // default one when there is none.
 BOOST_AUTO_TEST_CASE(test_macros) {