From 582ec2b0788443fd4aebda0b6d26228d2a4070b3 Mon Sep 17 00:00:00 2001 From: George Burgess IV Date: Tue, 2 May 2017 22:06:39 -0700 Subject: [PATCH] fix static-analyzer logging/CHECK macros Two small changes in one: - `foo || for (;;abort()) bar();` isn't valid C or C++, since for is a statement. We need an expression instead. - we'll now treat everything after LOG(foo) as unreachable in the static analyzer, as long as we can prove at compile-time that foo == FATAL. The impact of this, running across internal master, is that we see ~50 fewer medium/high-severity false positives from clang-tidy. We see 15 new complaints about unreachable code (at the "tidy" severity), but all of them are harmless AFAICT (e.g. switch (foo) { // ... default: LOG(FATAL) << "Unhandled case!"; // or CHECK(false); break; // clang-tidy: unreachable break. }) (Some of the macros were forcibly formatted by the clang-format hook) Bug: None Test: Ran `DEFAULT_GLOBAL_TIDY_CHECKS=clang-analyzer*,-clang-analyzer-alpha* m`; stared at warn.py output. Change-Id: Ie984eda0481afad4274b9def7c61ba777cfa289a --- base/include/android-base/logging.h | 69 +++++++++++++++++------------ 1 file changed, 40 insertions(+), 29 deletions(-) diff --git a/base/include/android-base/logging.h b/base/include/android-base/logging.h index e78edbb45..fa0d922bd 100644 --- a/base/include/android-base/logging.h +++ b/base/include/android-base/logging.h @@ -164,6 +164,24 @@ class ErrnoRestorer { using ::android::base::FATAL; \ return (severity); }()) +#ifdef __clang_analyzer__ +// Clang's static analyzer does not see the conditional statement inside +// LogMessage's destructor that will abort on FATAL severity. +#define ABORT_AFTER_LOG_FATAL for (;; abort()) + +struct LogAbortAfterFullExpr { + ~LogAbortAfterFullExpr() __attribute__((noreturn)) { abort(); } + explicit operator bool() const { return false; } +}; +// Provides an expression that evaluates to the truthiness of `x`, automatically +// aborting if `c` is true. +#define ABORT_AFTER_LOG_EXPR_IF(c, x) (((c) && ::android::base::LogAbortAfterFullExpr()) || (x)) +#else +#define ABORT_AFTER_LOG_FATAL +#define ABORT_AFTER_LOG_EXPR_IF(c, x) (x) +#endif +#define ABORT_AFTER_LOG_FATAL_EXPR(x) ABORT_AFTER_LOG_EXPR_IF(true, x) + // Defines whether the given severity will be logged or silently swallowed. #define WOULD_LOG(severity) \ UNLIKELY((SEVERITY_LAMBDA(severity)) >= ::android::base::GetMinimumLogSeverity()) @@ -190,54 +208,47 @@ class ErrnoRestorer { // LOG(FATAL) << "We didn't expect to reach here"; #define LOG(severity) LOG_TO(DEFAULT, severity) +// Checks if we want to log something, and sets up appropriate RAII objects if +// so. +// Note: DO NOT USE DIRECTLY. This is an implementation detail. +#define LOGGING_PREAMBLE(severity) \ + (WOULD_LOG(severity) && \ + ABORT_AFTER_LOG_EXPR_IF((SEVERITY_LAMBDA(severity)) == ::android::base::FATAL, true) && \ + ::android::base::ErrnoRestorer()) + // Logs a message to logcat with the specified log ID on Android otherwise to // stderr. If the severity is FATAL it also causes an abort. -// Use an if-else statement instead of just an if statement here. So if there is a -// else statement after LOG() macro, it won't bind to the if statement in the macro. -// do-while(0) statement doesn't work here. Because we need to support << operator -// following the macro, like "LOG(DEBUG) << xxx;". - -#define LOG_TO(dest, severity) \ - WOULD_LOG(severity) && \ - ::android::base::ErrnoRestorer() && \ - LOG_STREAM_TO(dest, severity) +// Use an expression here so we can support the << operator following the macro, +// like "LOG(DEBUG) << xxx;". +#define LOG_TO(dest, severity) LOGGING_PREAMBLE(severity) && LOG_STREAM_TO(dest, severity) // A variant of LOG that also logs the current errno value. To be used when // library calls fail. #define PLOG(severity) PLOG_TO(DEFAULT, severity) // Behaves like PLOG, but logs to the specified log ID. -#define PLOG_TO(dest, severity) \ - WOULD_LOG(SEVERITY_LAMBDA(severity)) && \ - ::android::base::ErrnoRestorer() && \ - ::android::base::LogMessage(__FILE__, __LINE__, \ - ::android::base::dest, \ - SEVERITY_LAMBDA(severity), errno).stream() +#define PLOG_TO(dest, severity) \ + LOGGING_PREAMBLE(severity) && \ + ::android::base::LogMessage(__FILE__, __LINE__, ::android::base::dest, \ + SEVERITY_LAMBDA(severity), errno) \ + .stream() // Marker that code is yet to be implemented. #define UNIMPLEMENTED(level) \ LOG(level) << __PRETTY_FUNCTION__ << " unimplemented " -#ifdef __clang_analyzer__ -// ClangL static analyzer does not see the conditional statement inside -// LogMessage's destructor that will abort on FATAL severity. -#define ABORT_AFTER_LOG_FATAL for (;;abort()) -#else -#define ABORT_AFTER_LOG_FATAL -#endif - // Check whether condition x holds and LOG(FATAL) if not. The value of the // expression x is only evaluated once. Extra logging can be appended using << // after. For example: // // CHECK(false == true) results in a log message of // "Check failed: false == true". -#define CHECK(x) \ - LIKELY((x)) || \ - ABORT_AFTER_LOG_FATAL \ - ::android::base::LogMessage(__FILE__, __LINE__, ::android::base::DEFAULT, \ - ::android::base::FATAL, -1).stream() \ - << "Check failed: " #x << " " +#define CHECK(x) \ + LIKELY((x)) || ABORT_AFTER_LOG_FATAL_EXPR(false) || \ + ::android::base::LogMessage( \ + __FILE__, __LINE__, ::android::base::DEFAULT, ::android::base::FATAL, \ + -1).stream() \ + << "Check failed: " #x << " " // Helper for CHECK_xx(x,y) macros. #define CHECK_OP(LHS, RHS, OP) \