logging: fix issues detecting XLOG(FATAL) statements as noreturn

Summary:
Update the FB_LOG() and XLOG() macros so that FATAL log messages are correctly
detected as not returning, by both clang and gcc.

We have to ensure that both sides of the log statement check (log message
enabled or disabled) evaluate to `[[noreturn]]` expressions.  I did try
updating the log check itself so that it could be constexpr detected as always
passing, but this was not sufficient.

Reviewed By: wez

Differential Revision: D5290780

fbshipit-source-id: 773a56a8392dfd7c310d5d84fc9311e66edf99cb

folly/experimental/logging/LogCategory.cpp

@@ -155,8 +155,13 @@ void LogCategory::setLevelLocked(LogLevel level, bool inherit) {
   //
   // This makes sure that UNINITIALIZED is always less than any valid level
   // value, and that level values cannot conflict with our flag bits.
-  if (level > LogLevel::MAX_LEVEL) {
-    level = LogLevel::MAX_LEVEL;
+  //
+  // In debug builds we clamp the maximum to DFATAL rather than MAX_LEVEL
+  // (FATAL), to ensure that fatal log messages can never be disabled.
+  constexpr LogLevel maxLogLevel =
+      kIsDebug ? LogLevel::DFATAL : LogLevel::MAX_LEVEL;
+  if (level > maxLogLevel) {
+    level = maxLogLevel;
   } else if (level < LogLevel::MIN_LEVEL) {
     level = LogLevel::MIN_LEVEL;
   }

folly/experimental/logging/LogStreamProcessor.cpp

@@ -175,6 +175,19 @@ void LogStreamProcessor::logNow() noexcept {
                                      extractMessageString(stream_)});
 }
 
+std::string LogStreamProcessor::extractMessageString(
+    LogStream& stream) noexcept {
+  if (stream.empty()) {
+    return std::move(message_);
+  }
+
+  if (message_.empty()) {
+    return stream.extractString();
+  }
+  message_.append(stream.extractString());
+  return std::move(message_);
+}
+
 void LogStreamVoidify<true>::operator&(std::ostream& stream) {
   // Non-fatal log messages wait until the LogStreamProcessor destructor to log
   // the message.  However for fatal messages we log immediately in the &
@@ -189,16 +202,11 @@ void LogStreamVoidify<true>::operator&(std::ostream& stream) {
   abort();
 }
 
-std::string LogStreamProcessor::extractMessageString(
-    LogStream& stream) noexcept {
-  if (stream.empty()) {
-    return std::move(message_);
-  }
-
-  if (message_.empty()) {
-    return stream.extractString();
-  }
-  message_.append(stream.extractString());
-  return std::move(message_);
+void logDisabledHelper(std::integral_constant<bool, true>) noexcept {
+  // This function can only be reached if we had a disabled fatal log message.
+  // This should never happen: LogCategory::setLevelLocked() does not allow
+  // setting the threshold for a category lower than FATAL (in production
+  // builds) or DFATAL (in debug builds).
+  abort();
 }
 }

folly/experimental/logging/LogStreamProcessor.h

@@ -463,4 +463,34 @@ class LogStreamVoidify<true> {
    */
   [[noreturn]] void operator&(std::ostream&);
 };
+
+/**
+ * logDisabledHelper() is invoked in FB_LOG() and XLOG() statements if the log
+ * admittance check fails.
+ *
+ * This function exists solely to ensure that both sides of the log check are
+ * marked [[noreturn]] for fatal log messages.  This allows the compiler to
+ * recognize that the full statement is noreturn, preventing warnings about
+ * missing return statements after fatal log messages.
+ *
+ * Unfortunately it does not appear possible to get the compiler to recognize
+ * that the disabled side of the log statement should never be reached for
+ * fatal messages.  Even if we make the check something like
+ * `(isLogLevelFatal(level) || realCheck)`, where isLogLevelFatal() is
+ * constexpr, this is not sufficient for gcc or clang to recognize that the
+ * full expression is noreturn.
+ *
+ * Ideally this would just be a template function specialized on a boolean
+ * IsFatal parameter.  Unfortunately this triggers a bug in clang, which does
+ * not like differing noreturn behavior for different template instantiations.
+ * Therefore we overload on integral_constant instead.
+ *
+ * clang-format also doesn't do a good job understanding this code and figuring
+ * out how to format it.
+ */
+// clang-format off
+inline void logDisabledHelper(std::integral_constant<bool, false>) noexcept {}
+[[noreturn]] void logDisabledHelper(
+        std::integral_constant<bool, true>) noexcept;
+// clang-format on
 }

folly/experimental/logging/Logger.h

@@ -27,16 +27,17 @@
  *
  * This macro generally should not be used directly by end users.
  */
-#define FB_LOG_IMPL(logger, level, type, ...)                          \
-  (!(logger).getCategory()->logCheck(level))                           \
-      ? (void)0                                                        \
-      : ::folly::LogStreamVoidify<::folly::isLogLevelFatal(level)>{} & \
-          ::folly::LogStreamProcessor{(logger).getCategory(),          \
-                                      (level),                         \
-                                      __FILE__,                        \
-                                      __LINE__,                        \
-                                      (type),                          \
-                                      ##__VA_ARGS__}                   \
+#define FB_LOG_IMPL(logger, level, type, ...)                                \
+  (!(logger).getCategory()->logCheck(level))                                 \
+      ? ::folly::logDisabledHelper(                                          \
+            std::integral_constant<bool, ::folly::isLogLevelFatal(level)>{}) \
+      : ::folly::LogStreamVoidify<::folly::isLogLevelFatal(level)>{} &       \
+          ::folly::LogStreamProcessor{(logger).getCategory(),                \
+                                      (level),                               \
+                                      __FILE__,                              \
+                                      __LINE__,                              \
+                                      (type),                                \
+                                      ##__VA_ARGS__}                         \
               .stream()
 
 /**

folly/experimental/logging/test/FatalHelper.cpp

@@ -59,13 +59,8 @@ class InitChecker {
 static InitChecker initChecker;
 }
 
-/*
- * This is a simple helper program to exercise the LOG(FATAL) functionality.
- */
-int main(int argc, char* argv[]) {
-  // Call folly::init() and then initialize log levels and handlers
-  folly::init(&argc, &argv);
-
+namespace {
+int runHelper() {
   if (FLAGS_handler_style == "async") {
     initLoggingGlogStyle(FLAGS_logging, LogLevel::INFO, true);
   } else if (FLAGS_handler_style == "immediate") {
@@ -84,7 +79,31 @@ int main(int argc, char* argv[]) {
   }
 
   XLOG(FATAL) << "test program crashing!";
-  // Even though main() is defined to return an integer, the compiler
+  // Even though this function is defined to return an integer, the compiler
   // should be able to detect that XLOG(FATAL) never returns.  It shouldn't
   // complain that we don't return an integer here.
 }
+}
+
+std::string fbLogFatalCheck() {
+  folly::Logger logger("some.category");
+  FB_LOG(logger, FATAL) << "we always crash";
+  // This function mostly exists to make sure the compiler does not warn
+  // about a missing return statement here.
+}
+
+/*
+ * This is a simple helper program to exercise the LOG(FATAL) functionality.
+ */
+int main(int argc, char* argv[]) {
+  // Call folly::init() and then initialize log levels and handlers
+  folly::init(&argc, &argv);
+
+  // Do most of the work in a separate helper function.
+  //
+  // The main reason for putting this in a helper function is to ensure that
+  // the compiler does not warn about missing return statements on XLOG(FATAL)
+  // code paths.  Unfortunately it appears like some compilers always suppress
+  // this warning for main().
+  return runHelper();
+}

folly/experimental/logging/xlog.h

@@ -112,28 +112,31 @@
  *   initialized.  On all subsequent calls, disabled log statements can be
  *   skipped with just a single check of the LogLevel.
  */
-#define XLOG_IMPL(level, type, ...)                                      \
-  (!XLOG_IS_ON_IMPL(level))                                              \
-      ? static_cast<void>(0)                                             \
-      : ::folly::LogStreamVoidify<::folly::isLogLevelFatal(level)>{} &   \
-          ::folly::LogStreamProcessor(                                   \
-              [] {                                                       \
-                static ::folly::XlogCategoryInfo<XLOG_IS_IN_HEADER_FILE> \
-                    _xlogCategory_;                                      \
-                return _xlogCategory_.getInfo(                           \
-                    &xlog_detail::xlogFileScopeInfo);                    \
-              }(),                                                       \
-              (level),                                                   \
-              xlog_detail::getXlogCategoryName(__FILE__, 0),             \
-              xlog_detail::isXlogCategoryOverridden(0),                  \
-              __FILE__,                                                  \
-              __LINE__,                                                  \
-              (type),                                                    \
-              ##__VA_ARGS__)                                             \
+#define XLOG_IMPL(level, type, ...)                                          \
+  (!XLOG_IS_ON_IMPL(level))                                                  \
+      ? ::folly::logDisabledHelper(                                          \
+            std::integral_constant<bool, ::folly::isLogLevelFatal(level)>{}) \
+      : ::folly::LogStreamVoidify<::folly::isLogLevelFatal(level)>{} &       \
+          ::folly::LogStreamProcessor(                                       \
+              [] {                                                           \
+                static ::folly::XlogCategoryInfo<XLOG_IS_IN_HEADER_FILE>     \
+                    _xlogCategory_;                                          \
+                return _xlogCategory_.getInfo(                               \
+                    &xlog_detail::xlogFileScopeInfo);                        \
+              }(),                                                           \
+              (level),                                                       \
+              xlog_detail::getXlogCategoryName(__FILE__, 0),                 \
+              xlog_detail::isXlogCategoryOverridden(0),                      \
+              __FILE__,                                                      \
+              __LINE__,                                                      \
+              (type),                                                        \
+              ##__VA_ARGS__)                                                 \
               .stream()
 
 /**
  * Check if and XLOG() statement with the given log level would be enabled.
+ *
+ * The level parameter must be an unqualified LogLevel enum value.
  */
 #define XLOG_IS_ON(level) XLOG_IS_ON_IMPL(::folly::LogLevel::level)