kudu-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From mpe...@apache.org
Subject kudu git commit: stopwatch: avoid clang use-after-move warnings in LOG_TIMING macros
Date Thu, 09 Nov 2017 08:51:33 GMT
Repository: kudu
Updated Branches:
  refs/heads/master c5fa3ed72 -> 04c23bc84


stopwatch: avoid clang use-after-move warnings in LOG_TIMING macros

As seen in [1] the implementation of LOG_TIMING was confusing
clang-tidy's use-after-move detection, since it relied on a
single-iteration for loop. Although the loop always ran for exactly one
iteration, clang-tidy wasn't smart enough to identify that, and thus any
std::move inside the loop would be analyzed as if it could run twice
(which would be an error).

This detects the case that the code is being analyzed and removes the
timing macro definitions.

A new test case produces the following clang-tidy warning without the
fix:

logging-test.cc:242:18: warning: 's1' used after it was moved [misc-use-after-move]
    LOG(INFO) << s1;
                 ^
logging-test.cc:243:8: note: move occurred here
    s2 = std::move(s1);
       ^
logging-test.cc:242:18: note: the use happens in a later loop iteration than the move
    LOG(INFO) << s1;
                 ^

With the fix, clang-tidy is clean.

[1] https://gerrit.cloudera.org/c/8352/8/src/kudu/fs/fs_manager.cc#372

Change-Id: I4ee030291d54cde0d846ceec6b8610ef051ffafa
Reviewed-on: http://gerrit.cloudera.org:8080/8502
Reviewed-by: Mike Percy <mpercy@apache.org>
Tested-by: Mike Percy <mpercy@apache.org>


Project: http://git-wip-us.apache.org/repos/asf/kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/04c23bc8
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/04c23bc8
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/04c23bc8

Branch: refs/heads/master
Commit: 04c23bc84efc48a75d44a2a8dce03d317ac4bb66
Parents: c5fa3ed
Author: Todd Lipcon <todd@apache.org>
Authored: Wed Nov 8 12:19:39 2017 -0800
Committer: Mike Percy <mpercy@apache.org>
Committed: Thu Nov 9 08:51:00 2017 +0000

----------------------------------------------------------------------
 build-support/clang_tidy_gerrit.py |  3 ++-
 src/kudu/util/logging-test.cc      | 23 +++++++++++++++++++++++
 src/kudu/util/stopwatch.h          | 17 +++++++++++++++++
 3 files changed, 42 insertions(+), 1 deletion(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/kudu/blob/04c23bc8/build-support/clang_tidy_gerrit.py
----------------------------------------------------------------------
diff --git a/build-support/clang_tidy_gerrit.py b/build-support/clang_tidy_gerrit.py
index edaf39e..b139d82 100755
--- a/build-support/clang_tidy_gerrit.py
+++ b/build-support/clang_tidy_gerrit.py
@@ -53,7 +53,8 @@ def run_tidy(sha="HEAD", is_rev_range=False):
         [CLANG_TIDY_DIFF,
          "-clang-tidy-binary", CLANG_TIDY,
          "-p0",
-         "--"] + compile_flags.get_flags(),
+         "--",
+         "-DCLANG_TIDY"] + compile_flags.get_flags(),
         stdin=file(patch_file.name))
 
 

http://git-wip-us.apache.org/repos/asf/kudu/blob/04c23bc8/src/kudu/util/logging-test.cc
----------------------------------------------------------------------
diff --git a/src/kudu/util/logging-test.cc b/src/kudu/util/logging-test.cc
index 9514d55..cceece8 100644
--- a/src/kudu/util/logging-test.cc
+++ b/src/kudu/util/logging-test.cc
@@ -21,6 +21,7 @@
 #include <ostream>
 #include <string>
 #include <thread>
+#include <utility>
 #include <vector>
 
 #include <glog/logging.h>
@@ -33,6 +34,7 @@
 #include "kudu/util/logging.h"
 #include "kudu/util/logging_test_util.h"
 #include "kudu/util/monotime.h"
+#include "kudu/util/stopwatch.h"
 #include "kudu/util/test_macros.h"  // IWYU pragma: keep
 #include "kudu/util/test_util.h"
 
@@ -223,4 +225,25 @@ TEST(LoggingTest, TestRedactionIllustrateUsage) {
   });
 }
 
+
+TEST(LoggingTest, TestLogTiming) {
+  LOG_TIMING(INFO, "foo") {
+  }
+  {
+    SCOPED_LOG_TIMING(INFO, "bar");
+  }
+  LOG_SLOW_EXECUTION(INFO, 1, "baz") {
+  }
+
+  // Previous implementations of the above macro confused clang-tidy's use-after-move
+  // check and generated false positives.
+  string s1 = "hello";
+  string s2;
+  LOG_SLOW_EXECUTION(INFO, 1, "baz") {
+    LOG(INFO) << s1;
+    s2 = std::move(s1);
+  }
+
+  ASSERT_EQ("hello", s2);
+}
 } // namespace kudu

http://git-wip-us.apache.org/repos/asf/kudu/blob/04c23bc8/src/kudu/util/stopwatch.h
----------------------------------------------------------------------
diff --git a/src/kudu/util/stopwatch.h b/src/kudu/util/stopwatch.h
index 617ee8b..cde2483 100644
--- a/src/kudu/util/stopwatch.h
+++ b/src/kudu/util/stopwatch.h
@@ -101,6 +101,23 @@ namespace kudu {
   kudu::sw_internal::LogTiming VARNAME_LINENUM(_log_timing)(__FILE__, __LINE__, \
       google::INFO, "", description, -1, VLOG_IS_ON(vlog_level));
 
+
+// Workaround for the clang analyzer being confused by the above loop-based macros.
+// The analyzer thinks the macros might loop more than once, and thus generates
+// false positives. So, for its purposes, just make them empty.
+#if defined(CLANG_TIDY) || defined(__clang_analyzer__)
+
+#undef LOG_TIMING_PREFIX_IF
+#define LOG_TIMING_PREFIX_IF(severity, condition, prefix, description)
+
+#undef VLOG_TIMING
+#define VLOG_TIMING(vlog_level, description)
+
+#undef LOG_SLOW_EXECUTION
+#define LOG_SLOW_EXECUTION(severity, max_expected_millis, description)
+#endif
+
+
 #define NANOS_PER_SECOND 1000000000.0
 #define NANOS_PER_MILLISECOND 1000000.0
 


Mime
View raw message