From 2f5e950d1fcdbcb8b45699a57a9e6fc40be12851 Mon Sep 17 00:00:00 2001 From: Sergiu Deitsch Date: Wed, 3 Jan 2024 17:28:14 +0100 Subject: [PATCH] fix: make log severity type safe --- CMakeLists.txt | 63 +++++++++++++++ src/glog/log_severity.h | 26 ++++-- src/glog/logging.h | 2 +- src/log_severity_unittest/CMakeLists.txt | 10 +++ .../glog_log_severity_constants.cc | 40 ++++++++++ .../glog_log_severity_conversion.cc | 42 ++++++++++ src/logging.cc | 80 +++++++++++-------- src/logging_unittest.cc | 2 +- src/signalhandler.cc | 2 +- 9 files changed, 224 insertions(+), 43 deletions(-) create mode 100644 src/log_severity_unittest/CMakeLists.txt create mode 100644 src/log_severity_unittest/glog_log_severity_constants.cc create mode 100644 src/log_severity_unittest/glog_log_severity_conversion.cc diff --git a/CMakeLists.txt b/CMakeLists.txt index e1a203d74..2124557ee 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -793,6 +793,69 @@ if (BUILD_TESTING) striplog10 PROPERTIES WILL_FAIL ON ) + + set (_glog_TEST_ENVIRONMENT + glog_ROOT=${glog_BINARY_DIR} + ) + + add_test (NAME log_severity_constants COMMAND ${CMAKE_CTEST_COMMAND} + --build-config $ + --build-and-test + "${glog_SOURCE_DIR}/src/log_severity_unittest" + "${glog_BINARY_DIR}/Tests/log_severity_constants" + --build-generator ${CMAKE_GENERATOR} + --build-makeprogram ${CMAKE_MAKE_PROGRAM} + --build-target glog_log_severity_constants + --build-options + -DCMAKE_BUILD_TYPE=$ + -DCMAKE_CXX_COMPILER=${CMAKE_CXX_COMPILER} + -DCMAKE_GENERATOR_PLATFORM=${CMAKE_GENERATOR_PLATFORM} + -DCMAKE_GENERATOR_TOOLSET=${CMAKE_GENERATOR_TOOLSET} + ) + set_tests_properties (log_severity_constants PROPERTIES + ENVIRONMENT "${_glog_TEST_ENVIRONMENT}" + PASS_REGULAR_EXPRESSION "COMPACT_GOOGLE_LOG_[1-3]" + ) + + add_test (NAME log_severity_conversion COMMAND ${CMAKE_CTEST_COMMAND} + --build-config $ + --build-and-test + "${glog_SOURCE_DIR}/src/log_severity_unittest" + "${glog_BINARY_DIR}/Tests/log_severity_conversion" + --build-generator ${CMAKE_GENERATOR} + --build-makeprogram ${CMAKE_MAKE_PROGRAM} + --build-target glog_log_severity_conversion + --build-options + -DCMAKE_BUILD_TYPE=$ + -DCMAKE_CXX_COMPILER=${CMAKE_CXX_COMPILER} + -DCMAKE_GENERATOR_PLATFORM=${CMAKE_GENERATOR_PLATFORM} + -DCMAKE_GENERATOR_TOOLSET=${CMAKE_GENERATOR_TOOLSET} + ) + set_tests_properties (log_severity_conversion PROPERTIES + ENVIRONMENT "${_glog_TEST_ENVIRONMENT}" + ) + + if (CMAKE_COMPILER_IS_GNUCXX) + set_tests_properties (log_severity_conversion PROPERTIES + PASS_REGULAR_EXPRESSION "error: invalid conversion from (‘|')int(’|')" + ) + elseif (CMAKE_CXX_COMPILER_ID MATCHES Clang) + set_tests_properties (log_severity_conversion PROPERTIES + PASS_REGULAR_EXPRESSION "no known conversion from 'int'" + ) + elseif (MSVC) + set_tests_properties (log_severity_conversion PROPERTIES + PASS_REGULAR_EXPRESSION "error C2440" + ) + else (CMAKE_COMPILER_IS_GNUCXX) + message (AUTHOR_WARNING + "Unsuuported C++ compiler ${CMAKE_CXX_COMPILER_ID}: " + "log_severity_conversion test will be disabled" + ) + set_tests_properties (log_severity_conversion DISABLED ON) + endif (CMAKE_COMPILER_IS_GNUCXX) + + unset (_glog_TEST_ENVIRONMENT) endif (BUILD_TESTING) install (TARGETS glog diff --git a/src/glog/log_severity.h b/src/glog/log_severity.h index 99f4b522b..86b02a1d7 100644 --- a/src/glog/log_severity.h +++ b/src/glog/log_severity.h @@ -1,4 +1,4 @@ -// Copyright (c) 2023, Google Inc. +// Copyright (c) 2024, Google Inc. // All rights reserved. // // Redistribution and use in source and binary forms, with or without @@ -48,17 +48,31 @@ // Variables of type LogSeverity are widely taken to lie in the range // [0, NUM_SEVERITIES-1]. Be careful to preserve this assumption if // you ever need to change their values or add a new severity. -using LogSeverity = int; -const int GLOG_INFO = 0, GLOG_WARNING = 1, GLOG_ERROR = 2, GLOG_FATAL = 3, - NUM_SEVERITIES = 4; +enum LogSeverity { + GLOG_INFO = 0, + GLOG_WARNING = 1, + GLOG_ERROR = 2, + GLOG_FATAL = 3, #ifndef GLOG_NO_ABBREVIATED_SEVERITIES # ifdef ERROR # error ERROR macro is defined. Define GLOG_NO_ABBREVIATED_SEVERITIES before including logging.h. See the document for detail. # endif -const int INFO = GLOG_INFO, WARNING = GLOG_WARNING, ERROR = GLOG_ERROR, - FATAL = GLOG_FATAL; + INFO = GLOG_INFO, + WARNING = GLOG_WARNING, + ERROR = GLOG_ERROR, + FATAL = GLOG_FATAL #endif +}; + +#if defined(__cpp_inline_variables) +# if (__cpp_inline_variables >= 201606L) +inline +# endif // (__cpp_inline_variables >= 201606L) +#endif // defined(__cpp_inline_variables) + // clang-format off +constexpr int NUM_SEVERITIES = 4; +// clang-format on // DFATAL is FATAL in debug mode, ERROR in normal mode #ifdef NDEBUG diff --git a/src/glog/logging.h b/src/glog/logging.h index 2d3b61667..0c821b2eb 100644 --- a/src/glog/logging.h +++ b/src/glog/logging.h @@ -1526,7 +1526,7 @@ class GLOG_EXPORT LogMessageFatal : public LogMessage { // A non-macro interface to the log facility; (useful // when the logging level is not a compile-time constant). -inline void LogAtLevel(int const severity, std::string const& msg) { +inline void LogAtLevel(LogSeverity severity, std::string const& msg) { LogMessage(__FILE__, __LINE__, severity).stream() << msg; } diff --git a/src/log_severity_unittest/CMakeLists.txt b/src/log_severity_unittest/CMakeLists.txt new file mode 100644 index 000000000..4d8412cd3 --- /dev/null +++ b/src/log_severity_unittest/CMakeLists.txt @@ -0,0 +1,10 @@ +cmake_minimum_required (VERSION 3.16) +project (glog_log_severity LANGUAGES CXX) + +find_package (glog REQUIRED NO_MODULE) + +add_executable (glog_log_severity_constants glog_log_severity_constants.cc) +target_link_libraries (glog_log_severity_constants PRIVATE glog::glog) + +add_executable (glog_log_severity_conversion glog_log_severity_conversion.cc) +target_link_libraries (glog_log_severity_conversion PRIVATE glog::glog) diff --git a/src/log_severity_unittest/glog_log_severity_constants.cc b/src/log_severity_unittest/glog_log_severity_constants.cc new file mode 100644 index 000000000..3775e083a --- /dev/null +++ b/src/log_severity_unittest/glog_log_severity_constants.cc @@ -0,0 +1,40 @@ +// Copyright (c) 2024, Google Inc. +// All rights reserved. +// +// Redistribution and use in source and binary forms, with or without +// modification, are permitted provided that the following conditions are +// met: +// +// * Redistributions of source code must retain the above copyright +// notice, this list of conditions and the following disclaimer. +// * Redistributions in binary form must reproduce the above +// copyright notice, this list of conditions and the following disclaimer +// in the documentation and/or other materials provided with the +// distribution. +// * Neither the name of Google Inc. nor the names of its +// contributors may be used to endorse or promote products derived from +// this software without specific prior written permission. +// +// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. +// +// Author: Sergiu Deitsch + +#include + +int main() { + // Must not compile + LOG(0) << "type unsafe info"; + LOG(1) << "type unsafe info"; + LOG(2) << "type unsafe info"; + LOG(3) << "type unsafe info"; +} diff --git a/src/log_severity_unittest/glog_log_severity_conversion.cc b/src/log_severity_unittest/glog_log_severity_conversion.cc new file mode 100644 index 000000000..53f6d7fbb --- /dev/null +++ b/src/log_severity_unittest/glog_log_severity_conversion.cc @@ -0,0 +1,42 @@ +// Copyright (c) 2024, Google Inc. +// All rights reserved. +// +// Redistribution and use in source and binary forms, with or without +// modification, are permitted provided that the following conditions are +// met: +// +// * Redistributions of source code must retain the above copyright +// notice, this list of conditions and the following disclaimer. +// * Redistributions in binary form must reproduce the above +// copyright notice, this list of conditions and the following disclaimer +// in the documentation and/or other materials provided with the +// distribution. +// * Neither the name of Google Inc. nor the names of its +// contributors may be used to endorse or promote products derived from +// this software without specific prior written permission. +// +// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. +// +// Author: Sergiu Deitsch + +#include + +int main() { + // Must not compile + google::LogMessage{__FILE__, __LINE__, -1}; + // Cast to int to avoid implicit conversoin to nullptr + google::LogMessage{__FILE__, __LINE__, static_cast(0)}; + google::LogMessage{__FILE__, __LINE__, 1}; + google::LogMessage{__FILE__, __LINE__, 2}; + google::LogMessage{__FILE__, __LINE__, 3}; +} diff --git a/src/logging.cc b/src/logging.cc index 6587a8dea..08bc1ac1d 100644 --- a/src/logging.cc +++ b/src/logging.cc @@ -27,14 +27,13 @@ // (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. -#include -#include #define _GNU_SOURCE 1 // needed for O_NOFOLLOW and pread()/pwrite() #include "glog/logging.h" #include #include +#include #include #include #include @@ -42,6 +41,8 @@ #include #include #include +#include +#include #include "base/commandlineflags.h" // to get the program name #include "base/googleinit.h" @@ -304,29 +305,43 @@ static bool TerminalSupportsColor() { return term_supports_color; } +#if defined(__cpp_lib_unreachable) && (__cpp_lib_unreachable >= 202202L) +# define GLOG_UNREACHABLE std::unreachable() +#elif !defined(NDEBUG) +# define GLOG_UNREACHABLE assert(false) +#else +# if defined(_MSC_VER) +# define GLOG_UNREACHABLE __assume(false) +# elif defined(__has_builtin) +# if __has_builtin(unreachable) +# define GLOG_UNREACHABLE __builtin_unreachable() +# endif +# endif +# if !defined(GLOG_UNREACHABLE) && defined(__GNUG__) +# define GLOG_UNREACHABLE __builtin_unreachable() +# endif +# if !defined(GLOG_UNREACHABLE) +# define GLOG_UNREACHABLE +# endif +#endif + namespace google { enum GLogColor { COLOR_DEFAULT, COLOR_RED, COLOR_GREEN, COLOR_YELLOW }; static GLogColor SeverityToColor(LogSeverity severity) { - assert(severity >= 0 && severity < NUM_SEVERITIES); - GLogColor color = COLOR_DEFAULT; switch (severity) { case GLOG_INFO: - color = COLOR_DEFAULT; - break; + return COLOR_DEFAULT; case GLOG_WARNING: - color = COLOR_YELLOW; - break; + return COLOR_YELLOW; case GLOG_ERROR: case GLOG_FATAL: - color = COLOR_RED; - break; - default: - // should never get here. - assert(false); + return COLOR_RED; } - return color; + + // should never get here. + GLOG_UNREACHABLE; } #ifdef GLOG_OS_WINDOWS @@ -340,9 +355,10 @@ static WORD GetColorAttribute(GLogColor color) { return FOREGROUND_GREEN; case COLOR_YELLOW: return FOREGROUND_RED | FOREGROUND_GREEN; - default: - return 0; + case COLOR_DEFAULT: + break; } + return 0; } #else @@ -382,8 +398,8 @@ struct LogMessage::LogMessageData { // Buffer space; contains complete message text. char message_text_[LogMessage::kMaxLogMessageLen + 1]; LogStream stream_; - char severity_; // What level is this LogMessage logged at? - int line_; // line number where logging call is. + LogSeverity severity_; // What level is this LogMessage logged at? + int line_; // line number where logging call is. void (LogMessage::*send_method_)(); // Call this in destructor to send union { // At most one of these is used: union to keep the size low. LogSink* sink_; // nullptr or sink to send message to @@ -621,7 +637,7 @@ class LogDestination { base::Logger* logger_; // Either &fileobject_, or wrapper around it static std::unique_ptr log_destinations_[NUM_SEVERITIES]; - static LogSeverity email_logging_severity_; + static std::underlying_type_t email_logging_severity_; static string addresses_; static string hostname_; static bool terminal_supports_color_; @@ -639,7 +655,8 @@ class LogDestination { }; // Errors do not get logged to email by default. -LogSeverity LogDestination::email_logging_severity_ = 99999; +std::underlying_type_t LogDestination::email_logging_severity_ = + 99999; string LogDestination::addresses_; string LogDestination::hostname_; @@ -696,7 +713,7 @@ inline void LogDestination::FlushLogFiles(int min_severity) { // all this stuff. std::lock_guard l{log_mutex}; for (int i = min_severity; i < NUM_SEVERITIES; i++) { - LogDestination* log = log_destination(i); + LogDestination* log = log_destination(static_cast(i)); if (log != nullptr) { log->logger_->Flush(); } @@ -705,7 +722,6 @@ inline void LogDestination::FlushLogFiles(int min_severity) { inline void LogDestination::SetLogDestination(LogSeverity severity, const char* base_filename) { - assert(severity >= 0 && severity < NUM_SEVERITIES); // Prevent any subtle race conditions by wrapping a mutex lock around // all this stuff. std::lock_guard l{log_mutex}; @@ -744,12 +760,12 @@ inline void LogDestination::SetLogFilenameExtension(const char* ext) { // all this stuff. std::lock_guard l{log_mutex}; for (int severity = 0; severity < NUM_SEVERITIES; ++severity) { - log_destination(severity)->fileobject_.SetExtension(ext); + log_destination(static_cast(severity)) + ->fileobject_.SetExtension(ext); } } inline void LogDestination::SetStderrLogging(LogSeverity min_severity) { - assert(min_severity >= 0 && min_severity < NUM_SEVERITIES); // Prevent any subtle race conditions by wrapping a mutex lock around // all this stuff. std::lock_guard l{log_mutex}; @@ -759,15 +775,15 @@ inline void LogDestination::SetStderrLogging(LogSeverity min_severity) { inline void LogDestination::LogToStderr() { // *Don't* put this stuff in a mutex lock, since SetStderrLogging & // SetLogDestination already do the locking! - SetStderrLogging(0); // thus everything is "also" logged to stderr + SetStderrLogging(GLOG_INFO); // thus everything is "also" logged to stderr for (int i = 0; i < NUM_SEVERITIES; ++i) { - SetLogDestination(i, ""); // "" turns off logging to a logfile + SetLogDestination(static_cast(i), + ""); // "" turns off logging to a logfile } } inline void LogDestination::SetEmailLogging(LogSeverity min_severity, const char* addresses) { - assert(min_severity >= 0 && min_severity < NUM_SEVERITIES); // Prevent any subtle race conditions by wrapping a mutex lock around // all this stuff. std::lock_guard l{log_mutex}; @@ -907,7 +923,8 @@ inline void LogDestination::LogToAllLogfiles(LogSeverity severity, ColoredWriteToStderr(severity, message, len); } else { for (int i = severity; i >= 0; --i) { - LogDestination::MaybeLogToLogfile(i, timestamp, message, len); + LogDestination::MaybeLogToLogfile(static_cast(i), timestamp, + message, len); } } } @@ -946,7 +963,6 @@ std::unique_ptr LogDestination::log_destinations_[NUM_SEVERITIES]; inline LogDestination* LogDestination::log_destination(LogSeverity severity) { - assert(severity >= 0 && severity < NUM_SEVERITIES); if (log_destinations_[severity] == nullptr) { log_destinations_[severity] = std::make_unique(severity, nullptr); @@ -999,10 +1015,7 @@ LogFileObject::LogFileObject(LogSeverity severity, const char* base_filename) filename_extension_(), severity_(severity), rollover_attempt_(kRolloverAttemptFrequency - 1), - start_time_(std::chrono::system_clock::now()) { - assert(severity >= 0); - assert(severity < NUM_SEVERITIES); -} + start_time_(std::chrono::system_clock::now()) {} LogFileObject::~LogFileObject() { std::lock_guard l{mutex_}; @@ -1819,7 +1832,6 @@ void ReprintFatalMessage() { void LogMessage::SendToLog() EXCLUSIVE_LOCKS_REQUIRED(log_mutex) { static bool already_warned_before_initgoogle = false; - RAW_DCHECK(data_->num_chars_to_log_ > 0 && data_->message_text_[data_->num_chars_to_log_ - 1] == '\n', ""); diff --git a/src/logging_unittest.cc b/src/logging_unittest.cc index edb1a1ba1..c1d117225 100644 --- a/src/logging_unittest.cc +++ b/src/logging_unittest.cc @@ -1419,7 +1419,7 @@ TEST(LogAtLevel, Basic) { EXPECT_CALL(log, Log(GLOG_WARNING, StrNe(__FILE__), "function version")); EXPECT_CALL(log, Log(GLOG_INFO, __FILE__, "macro version")); - int severity = GLOG_WARNING; + LogSeverity severity = GLOG_WARNING; LogAtLevel(severity, "function version"); severity = GLOG_INFO; diff --git a/src/signalhandler.cc b/src/signalhandler.cc index 841427914..2aa1dad68 100644 --- a/src/signalhandler.cc +++ b/src/signalhandler.cc @@ -353,7 +353,7 @@ void FailureSignalHandler(int signal_number, siginfo_t* signal_info, // Flush the logs before we do anything in case 'anything' // causes problems. - FlushLogFilesUnsafe(0); + FlushLogFilesUnsafe(GLOG_INFO); // Kill ourself by the default signal handler. InvokeDefaultSignalHandler(signal_number);