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

base/logging.h

Index: base/logging.h
diff --git a/base/logging.h b/base/logging.h
index c67b937e5b08607736e860dd349f1de6204c6a5b..f76cfdf8ed0223125f01d8f56469de57c1fd900b 100644
--- a/base/logging.h
+++ b/base/logging.h
@@ -289,6 +289,39 @@ typedef bool (*LogMessageHandlerFunction)(int severity,
 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,
@@ -562,37 +595,15 @@ class CheckOpResult {
 
 #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
@@ -685,17 +696,21 @@ std::string* MakeCheckOpString<std::string, std::string>(
 // 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);                    \
@@ -792,48 +807,13 @@ const LogSeverity LOG_DCHECK = LOG_INFO;
 // 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()
@@ -843,8 +823,6 @@ inline constexpr bool AnalyzerNoReturn() __attribute__((analyzer_noreturn)) {
 
 #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
@@ -879,7 +857,7 @@ inline constexpr bool AnalyzerNoReturn() __attribute__((analyzer_noreturn)) {
                                 ::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()