Add Clang static analysis control to all assert functions in logging.h

base/logging.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef BASE_LOGGING_H_
 #define BASE_LOGGING_H_
 
 #include <stddef.h>
 
 #include <cassert>
@@ skipping 271 matching lines @@
 
 // Sets the Log Message Handler that gets passed every log message before
 // it's sent to other log destinations (if any).
 // Returns true to signal that it handled the message and the message
 // should not be sent to other log destinations.
 typedef bool (*LogMessageHandlerFunction)(int severity,
     const char* file, int line, size_t message_start, const std::string& str);
 BASE_EXPORT void SetLogMessageHandler(LogMessageHandlerFunction handler);
 BASE_EXPORT LogMessageHandlerFunction GetLogMessageHandler();
 
+// ANALYZER_ASSUME_TRUE(bool arg) generates compiler-specific annotations which
+// prevent the static analyzer from analyzing the code using hypothetical
+// values that are asserted to be impossible. It returns the truth value of
+// |arg| as a bool.
+//
+// If |arg| is true, ANALYZER_ASSUME_TRUE has no effect and static analysis
+// may proceed with analysis along the current path.
+//
+// If |arg| is false, static analysis is terminated and no further analysis
+// errors will be generated for the current path.

[brucedawson, 2017/03/16 20:15:40]

This doesn't fit my mental model for how /analyze uses these results. The
/analyze intrinsic is __analyzis_assume and, like the name says, it tells the
analyzer to assume that the argument is true. This doesn't necessarily have to
do with a particular execution path. It is often used to indicate that a
particular index, for instance, is constrained to a particular range in order to
avoid spurious out-of-range access warnings.

It's just a comment, and it's not really a *blocking* issue but I wanted to
mention it.

[Kevin M, 2017/03/16 23:08:13]

Interesting. In the Clang case, noreturn is used for culling analysis paths, not
for creating new ones.

OK, I'll reword this to be more general. "This macro does things on stuff." ;)
Maybe not THAT general.

+#if defined(__clang_analyzer__)
+
+inline void AnalyzerNoReturn() __attribute__((analyzer_noreturn)) {}
+
+inline constexpr bool AnalyzerAssumeTrue(bool arg) {
+  if (!arg) {
+    AnalyzerNoReturn();
+  }
+  return arg;
+}
+
+#define ANALYZER_ASSUME_TRUE(arg) ::logging::AnalyzerAssumeTrue(!!(arg))
+
+#elif defined(_PREFAST_) && defined(OS_WIN)
+
+#define ANALYZER_ASSUME_TRUE(arg) (__analysis_assume(!!(arg)), !!(arg))

[brucedawson, 2017/03/16 20:15:40]

The second !! is not needed.

[Kevin M, 2017/03/16 23:08:13]

It makes the expression value consistent with the Clang case, which works with
bools.

+
+#else  // _PREFAST_ & OS_WIN
+
+#define ANALYZER_ASSUME_TRUE(arg) !!(arg)

[brucedawson, 2017/03/16 20:15:40]

The !! should not be needed, should it? It is needed for __analysis_assume due
to a quirk of its implementation. It is probably needed for AnalyzerAssumeTrue
because it expects an actual bool. But elsewhere maybe not?

[Kevin M, 2017/03/16 23:08:13]

See above..

+
+#endif  // !__clang_analyzer && !_PREFAST_
+
 typedef int LogSeverity;
 const LogSeverity LOG_VERBOSE = -1;  // This is level 1 verbosity
 // Note: the log severities are used to index into the array of names,
 // see log_severity_names.
 const LogSeverity LOG_INFO = 0;
 const LogSeverity LOG_WARNING = 1;
 const LogSeverity LOG_ERROR = 2;
 const LogSeverity LOG_FATAL = 3;
 const LogSeverity LOG_NUM_SEVERITIES = 4;
 
@@ skipping 253 matching lines @@
 // compiler optimizations.
 #define CHECK(condition) \
   UNLIKELY(!(condition)) ? IMMEDIATE_CRASH() : EAT_STREAM_PARAMETERS
 
 #define PCHECK(condition) CHECK(condition)
 
 #define CHECK_OP(name, op, val1, val2) CHECK((val1) op (val2))
 
 #else  // !(OFFICIAL_BUILD && NDEBUG)
 
-#if defined(_PREFAST_) && defined(OS_WIN)
-// Use __analysis_assume to tell the VC++ static analysis engine that
-// assert conditions are true, to suppress warnings.  The LAZY_STREAM
-// parameter doesn't reference 'condition' in /analyze builds because
-// this evaluation confuses /analyze. The !! before condition is because
-// __analysis_assume gets confused on some conditions:
-// http://randomascii.wordpress.com/2011/09/13/analyze-for-visual-studio-the-ugly-part-5/
-
-#define CHECK(condition)                    \
-  __analysis_assume(!!(condition)),         \
-      LAZY_STREAM(LOG_STREAM(FATAL), false) \
-          << "Check failed: " #condition ". "
-
-#define PCHECK(condition)                    \
-  __analysis_assume(!!(condition)),          \
-      LAZY_STREAM(PLOG_STREAM(FATAL), false) \
-          << "Check failed: " #condition ". "
-
-#else  // _PREFAST_
-
 // Do as much work as possible out of line to reduce inline code size.
 #define CHECK(condition)                                                      \
   LAZY_STREAM(::logging::LogMessage(__FILE__, __LINE__, #condition).stream(), \
-              !(condition))
+              !ANALYZER_ASSUME_TRUE(condition))
 
-#define PCHECK(condition)                       \
-  LAZY_STREAM(PLOG_STREAM(FATAL), !(condition)) \
+#define PCHECK(condition)                                           \
+  LAZY_STREAM(PLOG_STREAM(FATAL), !ANALYZER_ASSUME_TRUE(condition)) \
       << "Check failed: " #condition ". "
 
-#endif  // _PREFAST_
-
 // Helper macro for binary operators.
 // Don't use this macro directly in your code, use CHECK_EQ et al below.
 // The 'switch' is used to prevent the 'else' from being ambiguous when the
 // macro is used in an 'if' clause such as:
 // if (a == 1)
 //   CHECK_EQ(2, a);
 #define CHECK_OP(name, op, val1, val2)                                         \
   switch (0) case 0: default:                                                  \
   if (::logging::CheckOpResult true_if_passed =                                \
       ::logging::Check##name##Impl((val1), (val2),                             \
@@ skipping 72 matching lines @@
 std::string* MakeCheckOpString<unsigned int, unsigned long>(
     const unsigned int&, const unsigned long&, const char* names);
 extern template BASE_EXPORT
 std::string* MakeCheckOpString<std::string, std::string>(
     const std::string&, const std::string&, const char* name);
 
 // Helper functions for CHECK_OP macro.
 // The (int, int) specialization works around the issue that the compiler
 // will not instantiate the template version of the function on values of
 // unnamed enum type - see comment below.
+//
+// The checked condition is wrapped with ANALYZER_ASSUME_TRUE, which under
+// static analysis builds, blocks analysis of the current path if the
+// condition is false.
 #define DEFINE_CHECK_OP_IMPL(name, op)                                       \
   template <class t1, class t2>                                              \
   inline std::string* Check##name##Impl(const t1& v1, const t2& v2,          \
                                         const char* names) {                 \
-    if (v1 op v2)                                                            \
+    if (ANALYZER_ASSUME_TRUE(v1 op v2))                                      \
       return NULL;                                                           \
     else                                                                     \
       return ::logging::MakeCheckOpString(v1, v2, names);                    \
   }                                                                          \
   inline std::string* Check##name##Impl(int v1, int v2, const char* names) { \
-    if (v1 op v2)                                                            \
+    if (ANALYZER_ASSUME_TRUE(v1 op v2))                                      \
       return NULL;                                                           \
     else                                                                     \
       return ::logging::MakeCheckOpString(v1, v2, names);                    \
   }
 DEFINE_CHECK_OP_IMPL(EQ, ==)
 DEFINE_CHECK_OP_IMPL(NE, !=)
 DEFINE_CHECK_OP_IMPL(LE, <=)
 DEFINE_CHECK_OP_IMPL(LT, < )
 DEFINE_CHECK_OP_IMPL(GE, >=)
 DEFINE_CHECK_OP_IMPL(GT, > )
@@ skipping 76 matching lines @@
 
 // DCHECK et al. make sure to reference |condition| regardless of
 // whether DCHECKs are enabled; this is so that we don't get unused
 // variable warnings if the only use of a variable is in a DCHECK.
 // This behavior is different from DLOG_IF et al.
 //
 // Note that the definition of the DCHECK macros depends on whether or not
 // DCHECK_IS_ON() is true. When DCHECK_IS_ON() is false, the macros use
 // EAT_STREAM_PARAMETERS to avoid expressions that would create temporaries.
 
-#if defined(_PREFAST_) && defined(OS_WIN)
-// See comments on the previous use of __analysis_assume.
-
-#define DCHECK(condition)                    \
-  __analysis_assume(!!(condition)),          \
-      LAZY_STREAM(LOG_STREAM(DCHECK), false) \
-          << "Check failed: " #condition ". "
-
-#define DPCHECK(condition)                    \
-  __analysis_assume(!!(condition)),           \
-      LAZY_STREAM(PLOG_STREAM(DCHECK), false) \
-          << "Check failed: " #condition ". "
-
-#elif defined(__clang_analyzer__)
-
-// Keeps the static analyzer from proceeding along the current codepath,
-// otherwise false positive errors may be generated  by null pointer checks.
-inline constexpr bool AnalyzerNoReturn() __attribute__((analyzer_noreturn)) {
-  return false;
-}
-
-#define DCHECK(condition)                                                     \
-  LAZY_STREAM(                                                                \
-      LOG_STREAM(DCHECK),                                                     \
-      DCHECK_IS_ON() ? (logging::AnalyzerNoReturn() || !(condition)) : false) \
-      << "Check failed: " #condition ". "
-
-#define DPCHECK(condition)                                                    \
-  LAZY_STREAM(                                                                \
-      PLOG_STREAM(DCHECK),                                                    \
-      DCHECK_IS_ON() ? (logging::AnalyzerNoReturn() || !(condition)) : false) \
-      << "Check failed: " #condition ". "
-
-#else
-
 #if DCHECK_IS_ON()
 
-#define DCHECK(condition)                       \
-  LAZY_STREAM(LOG_STREAM(DCHECK), !(condition)) \
+#define DCHECK(condition)                                           \
+  LAZY_STREAM(LOG_STREAM(DCHECK), !ANALYZER_ASSUME_TRUE(condition)) \
       << "Check failed: " #condition ". "
-#define DPCHECK(condition)                       \
-  LAZY_STREAM(PLOG_STREAM(DCHECK), !(condition)) \
+#define DPCHECK(condition)                                           \
+  LAZY_STREAM(PLOG_STREAM(DCHECK), !ANALYZER_ASSUME_TRUE(condition)) \
       << "Check failed: " #condition ". "
 
 #else  // DCHECK_IS_ON()
 
 #define DCHECK(condition) EAT_STREAM_PARAMETERS << !(condition)
 #define DPCHECK(condition) EAT_STREAM_PARAMETERS << !(condition)
 
 #endif  // DCHECK_IS_ON()
 
-#endif
-
 // Helper macro for binary operators.
 // Don't use this macro directly in your code, use DCHECK_EQ et al below.
 // The 'switch' is used to prevent the 'else' from being ambiguous when the
 // macro is used in an 'if' clause such as:
 // if (a == 1)
 //   DCHECK_EQ(2, a);
 #if DCHECK_IS_ON()
 
 #define DCHECK_OP(name, op, val1, val2)                                \
   switch (0) case 0: default:                                          \
   if (::logging::CheckOpResult true_if_passed =                        \
       DCHECK_IS_ON() ?                                                 \
       ::logging::Check##name##Impl((val1), (val2),                     \
                                    #val1 " " #op " " #val2) : nullptr) \
    ;                                                                   \
   else                                                                 \
     ::logging::LogMessage(__FILE__, __LINE__, ::logging::LOG_DCHECK,   \
                           true_if_passed.message()).stream()
 
 #else  // DCHECK_IS_ON()
 
 // When DCHECKs aren't enabled, DCHECK_OP still needs to reference operator<<
 // overloads for |val1| and |val2| to avoid potential compiler warnings about
 // unused functions. For the same reason, it also compares |val1| and |val2|
 // using |op|.
 //
 // Note that the contract of DCHECK_EQ, etc is that arguments are only evaluated

[brucedawson, 2017/03/16 20:15:40]

Is this contract still respected? I'm not sure whether it matters or not, but
the use of the comma operator in the /analyze version of ANALYZER_ASSUME_TRUE
suggests the possibility of double evaluation.

Note that the previous usage of __analysis_assume avoided this double evaluation
by passing false to LAZY_STREAM. I believe this was determined to be harmless
because the resulting binary is never run so effectively compiling out all
DCHECKs is irrelevant.

It may be that similar logic means that the current implementation is fine also,
but the alternative would be replacing:

#define ANALYZER_ASSUME_TRUE(arg) (__analysis_assume(!!(arg)), !!(arg))

with:

#define ANALYZER_ASSUME_TRUE(arg) (__analysis_assume(!!(arg)), false)

[Kevin M, 2017/03/16 23:08:13]

Actually, the potential for double evaluation of an expression w/side effects
(e.g. DCHECK(foo++)) is concerning.

I adapted the MSVC #ifdef to use an AnalysisAssumeTrue() function a la Clang,
which just calls analysis_assume & passes the computed boolean result straight
through. That way the expression is evaluated only once.

 // once. Even though |val1| and |val2| appear twice in this version of the macro
 // expansion, this is OK, since the expression is never actually evaluated.
 #define DCHECK_OP(name, op, val1, val2)                             \
   EAT_STREAM_PARAMETERS << (::logging::MakeCheckOpValueString(      \
                                 ::logging::g_swallow_stream, val1), \
                             ::logging::MakeCheckOpValueString(      \
                                 ::logging::g_swallow_stream, val2), \
-                            (val1)op(val2))
+                            ANALYZER_ASSUME_TRUE((val1)op(val2)))
 
 #endif  // DCHECK_IS_ON()
 
 // Equality/Inequality checks - compare two values, and log a
 // LOG_DCHECK message including the two values when the result is not
 // as expected.  The values must have operator<<(ostream, ...)
 // defined.
 //
 // You may append to the error message like so:
 //   DCHECK_NE(1, 2) << "The world must be ending!";
@@ skipping 253 matching lines @@
 #elif NOTIMPLEMENTED_POLICY == 5
 #define NOTIMPLEMENTED() do {\
   static bool logged_once = false;\
   LOG_IF(ERROR, !logged_once) << NOTIMPLEMENTED_MSG;\
   logged_once = true;\
 } while(0);\
 EAT_STREAM_PARAMETERS
 #endif
 
 #endif  // BASE_LOGGING_H_