Merge "MediaMetrics: Add thread-safety checking" into rvc-dev

media/libmediametrics/Android.bp

@@ -22,9 +22,11 @@ cc_library_shared {
     export_include_dirs: ["include"],
 
     cflags: [
-        "-Werror",
-        "-Wno-error=deprecated-declarations",
         "-Wall",
+        "-Werror",
+        "-Wextra",
+        "-Wthread-safety",
+        "-Wunreachable-code",
     ],
 
     sanitize: {

services/mediametrics/AnalyticsActions.h

@@ -16,6 +16,7 @@
 
 #pragma once
 
+#include <android-base/thread_annotations.h>
 #include <media/MediaMetricsItem.h>
 #include <mutex>
 
@@ -144,7 +145,7 @@ private:
     }
 
     mutable std::mutex mLock;
-    std::map<Trigger, Action> mFilters; // GUARDED_BY mLock
+    std::map<Trigger, Action> mFilters GUARDED_BY(mLock);
 };
 
 } // namespace android::mediametrics

services/mediametrics/Android.bp

@@ -27,6 +27,7 @@ cc_binary {
         "-Wall",
         "-Werror",
         "-Wextra",
+        "-Wthread-safety",
     ],
 }
 

services/mediametrics/MediaMetricsService.cpp

@@ -282,8 +282,8 @@ status_t MediaMetricsService::dump(int fd, const Vector<String16>& args)
         } else {
             result.appendFormat("Dump of the %s process:\n", kServiceName);
             const char *prefixptr = prefix.size() > 0 ? prefix.c_str() : nullptr;
-            dumpHeaders_l(result, sinceNs, prefixptr);
+            dumpHeaders(result, sinceNs, prefixptr);
-            dumpQueue_l(result, sinceNs, prefixptr);
+            dumpQueue(result, sinceNs, prefixptr);
 
             // TODO: maybe consider a better way of dumping audio analytics info.
             const int32_t linesToDump = all ? INT32_MAX : 1000;
@@ -312,7 +312,7 @@ status_t MediaMetricsService::dump(int fd, const Vector<String16>& args)
 }
 
 // dump headers
-void MediaMetricsService::dumpHeaders_l(String8 &result, int64_t sinceNs, const char* prefix)
+void MediaMetricsService::dumpHeaders(String8 &result, int64_t sinceNs, const char* prefix)
 {
     if (mediametrics::Item::isEnabled()) {
         result.append("Metrics gathering: enabled\n");
@@ -337,7 +337,7 @@ void MediaMetricsService::dumpHeaders_l(String8 &result, int64_t sinceNs, const
 }
 
 // TODO: should prefix be a set<string>?
-void MediaMetricsService::dumpQueue_l(String8 &result, int64_t sinceNs, const char* prefix)
+void MediaMetricsService::dumpQueue(String8 &result, int64_t sinceNs, const char* prefix)
 {
     if (mItems.empty()) {
         result.append("empty\n");
@@ -364,7 +364,7 @@ void MediaMetricsService::dumpQueue_l(String8 &result, int64_t sinceNs, const ch
 
 // if item != NULL, it's the item we just inserted
 // true == more items eligible to be recovered
-bool MediaMetricsService::expirations_l(const std::shared_ptr<const mediametrics::Item>& item)
+bool MediaMetricsService::expirations(const std::shared_ptr<const mediametrics::Item>& item)
 {
     bool more = false;
 
@@ -419,7 +419,7 @@ void MediaMetricsService::processExpirations()
     do {
         sleep(1);
         std::lock_guard _l(mLock);
-        more = expirations_l(nullptr);
+        more = expirations(nullptr);
     } while (more);
 }
 
@@ -429,7 +429,7 @@ void MediaMetricsService::saveItem(const std::shared_ptr<const mediametrics::Ite
     // we assume the items are roughly in time order.
     mItems.emplace_back(item);
     ++mItemsFinalized;
-    if (expirations_l(item)
+    if (expirations(item)
             && (!mExpireFuture.valid()
                || mExpireFuture.wait_for(std::chrono::seconds(0)) == std::future_status::ready)) {
         mExpireFuture = std::async(std::launch::async, [this] { processExpirations(); });

services/mediametrics/MediaMetricsService.h

@@ -23,6 +23,7 @@
 #include <unordered_map>
 
 // IMediaMetricsService must include Vector, String16, Errors
+#include <android-base/thread_annotations.h>
 #include <media/IMediaMetricsService.h>
 #include <mediautils/ServiceUtilities.h>
 #include <utils/String8.h>
@@ -81,12 +82,11 @@ private:
     bool isRateLimited(mediametrics::Item *) const;
     void saveItem(const std::shared_ptr<const mediametrics::Item>& item);
 
-    // The following methods are GUARDED_BY(mLock)
+    bool expirations(const std::shared_ptr<const mediametrics::Item>& item) REQUIRES(mLock);
-    bool expirations_l(const std::shared_ptr<const mediametrics::Item>& item);
 
     // support for generating output
-    void dumpQueue_l(String8 &result, int64_t sinceNs, const char* prefix);
+    void dumpQueue(String8 &result, int64_t sinceNs, const char* prefix) REQUIRES(mLock);
-    void dumpHeaders_l(String8 &result, int64_t sinceNs, const char* prefix);
+    void dumpHeaders(String8 &result, int64_t sinceNs, const char* prefix) REQUIRES(mLock);
 
     // The following variables accessed without mLock
 
@@ -102,22 +102,22 @@ private:
 
     mediautils::UidInfo mUidInfo;  // mUidInfo can be accessed without lock (locked internally)
 
-    mediametrics::AudioAnalytics mAudioAnalytics;
+    mediametrics::AudioAnalytics mAudioAnalytics; // mAudioAnalytics is locked internally.
 
     std::mutex mLock;
     // statistics about our analytics
-    int64_t mItemsFinalized = 0;        // GUARDED_BY(mLock)
+    int64_t mItemsFinalized GUARDED_BY(mLock) = 0;
-    int64_t mItemsDiscarded = 0;        // GUARDED_BY(mLock)
+    int64_t mItemsDiscarded GUARDED_BY(mLock) = 0;
-    int64_t mItemsDiscardedExpire = 0;  // GUARDED_BY(mLock)
+    int64_t mItemsDiscardedExpire GUARDED_BY(mLock) = 0;
-    int64_t mItemsDiscardedCount = 0;   // GUARDED_BY(mLock)
+    int64_t mItemsDiscardedCount GUARDED_BY(mLock) = 0;
 
     // If we have a worker thread to garbage collect
-    std::future<void> mExpireFuture;    // GUARDED_BY(mLock)
+    std::future<void> mExpireFuture GUARDED_BY(mLock);
 
     // Our item queue, generally (oldest at front)
     // TODO: Make separate class, use segmented queue, write lock only end.
     // Note: Another analytics module might have ownership of an item longer than the log.
-    std::deque<std::shared_ptr<const mediametrics::Item>> mItems; // GUARDED_BY(mLock)
+    std::deque<std::shared_ptr<const mediametrics::Item>> mItems GUARDED_BY(mLock);
 };
 
 } // namespace android

services/mediametrics/TimeMachine.h

@@ -23,6 +23,7 @@
 #include <variant>
 #include <vector>
 
+#include <android-base/thread_annotations.h>
 #include <media/MediaMetricsItem.h>
 #include <utils/Timers.h>
 
@@ -92,7 +93,8 @@ private:
         }
 
         template <typename T>
-        status_t getValue(const std::string &property, T* value, int64_t time = 0) const {
+        status_t getValue(const std::string &property, T* value, int64_t time = 0) const
+                REQUIRES(mPseudoKeyHistoryLock) {
             if (time == 0) time = systemTime(SYSTEM_TIME_REALTIME);
             const auto tsptr = mPropertyMap.find(property);
             if (tsptr == mPropertyMap.end()) return BAD_VALUE;
@@ -108,20 +110,22 @@ private:
         }
 
         template <typename T>
-        status_t getValue(const std::string &property, T defaultValue, int64_t time = 0) const {
+        status_t getValue(const std::string &property, T defaultValue, int64_t time = 0) const
+                REQUIRES(mPseudoKeyHistoryLock){
             T value;
             return getValue(property, &value, time) != NO_ERROR ? defaultValue : value;
         }
 
         void putProp(
-                const std::string &name, const mediametrics::Item::Prop &prop, int64_t time = 0) {
+                const std::string &name, const mediametrics::Item::Prop &prop, int64_t time = 0)
+                REQUIRES(mPseudoKeyHistoryLock) {
             //alternatively: prop.visit([&](auto value) { putValue(name, value, time); });
             putValue(name, prop.get(), time);
         }
 
         template <typename T>
-        void putValue(const std::string &property,
+        void putValue(const std::string &property, T&& e, int64_t time = 0)
-                T&& e, int64_t time = 0) {
+                REQUIRES(mPseudoKeyHistoryLock) {
             if (time == 0) time = systemTime(SYSTEM_TIME_REALTIME);
             mLastModificationTime = time;
             if (mPropertyMap.size() >= kKeyMaxProperties &&
@@ -144,7 +148,8 @@ private:
             }
         }
 
-        std::pair<std::string, int32_t> dump(int32_t lines, int64_t time) const {
+        std::pair<std::string, int32_t> dump(int32_t lines, int64_t time) const
+                REQUIRES(mPseudoKeyHistoryLock) {
             std::stringstream ss;
             int32_t ll = lines;
             for (auto& tsPair : mPropertyMap) {
@@ -158,7 +163,9 @@ private:
             return { ss.str(), lines - ll };
         }
 
-        int64_t getLastModificationTime() const { return mLastModificationTime; }
+        int64_t getLastModificationTime() const REQUIRES(mPseudoKeyHistoryLock) {
+            return mLastModificationTime;
+        }
 
     private:
         static std::string dump(
@@ -267,7 +274,7 @@ public:
             if (it == mHistory.end()) {
                 if (!isTrusted) return PERMISSION_DENIED;
 
-                (void)gc_l(garbage);
+                (void)gc(garbage);
 
                 // no keylock needed here as we are sole owner
                 // until placed on mHistory.
@@ -435,7 +442,8 @@ public:
 private:
 
     // Obtains the lock for a KeyHistory.
-    std::mutex &getLockForKey(const std::string &key) const {
+    std::mutex &getLockForKey(const std::string &key) const
+            RETURN_CAPABILITY(mPseudoKeyHistoryLock) {
         return mKeyLocks[std::hash<std::string>{}(key) % std::size(mKeyLocks)];
     }
 
@@ -459,7 +467,6 @@ private:
         return it->second;
     }
 
-    // GUARDED_BY mLock
     /**
      * Garbage collects if the TimeMachine size exceeds the high water mark.
      *
@@ -471,7 +478,7 @@ private:
      *
      * \return true if garbage collection was done.
      */
-    bool gc_l(std::vector<std::any>& garbage) {
+    bool gc(std::vector<std::any>& garbage) REQUIRES(mLock) {
         // TODO: something better than this for garbage collection.
         if (mHistory.size() < mKeyHighWaterMark) return false;
 
@@ -540,12 +547,17 @@ private:
      */
 
     mutable std::mutex mLock;           // Lock for mHistory
-    History mHistory;                   // GUARDED_BY mLock
+    History mHistory GUARDED_BY(mLock);
 
     // KEY_LOCKS is the number of mutexes for keys.
     // It need not be a power of 2, but faster that way.
     static inline constexpr size_t KEY_LOCKS = 256;
     mutable std::mutex mKeyLocks[KEY_LOCKS];  // Hash-striped lock for KeyHistory based on key.
+
+    // Used for thread-safety analysis, we create a fake mutex object to represent
+    // the hash stripe lock mechanism, which is then tracked by the compiler.
+    class CAPABILITY("mutex") PseudoLock {};
+    static inline PseudoLock mPseudoKeyHistoryLock;
 };
 
 } // namespace android::mediametrics

services/mediametrics/TransactionLog.h

@@ -21,6 +21,7 @@
 #include <sstream>
 #include <string>
 
+#include <android-base/thread_annotations.h>
 #include <media/MediaMetricsItem.h>
 
 namespace android::mediametrics {
@@ -92,7 +93,7 @@ public:
         std::vector<std::any> garbage;  // objects destroyed after lock.
         std::lock_guard lock(mLock);
 
-        (void)gc_l(garbage);
+        (void)gc(garbage);
         mLog.emplace(time, item);
         mItemMap[key].emplace(time, item);
         return NO_ERROR;  // no errors for now.
@@ -104,7 +105,7 @@ public:
     std::vector<std::shared_ptr<const mediametrics::Item>> get(
             int64_t startTime = 0, int64_t endTime = INT64_MAX) const {
         std::lock_guard lock(mLock);
-        return getItemsInRange_l(mLog, startTime, endTime);
+        return getItemsInRange(mLog, startTime, endTime);
     }
 
     /**
@@ -116,7 +117,7 @@ public:
         std::lock_guard lock(mLock);
         auto mapIt = mItemMap.find(key);
         if (mapIt == mItemMap.end()) return {};
-        return getItemsInRange_l(mapIt->second, startTime, endTime);
+        return getItemsInRange(mapIt->second, startTime, endTime);
     }
 
     /**
@@ -204,7 +205,6 @@ private:
         return { ss.str(), lines - ll };
     }
 
-    // GUARDED_BY mLock
     /**
      * Garbage collects if the TimeMachine size exceeds the high water mark.
      *
@@ -213,7 +213,7 @@ private:
      *
      * \return true if garbage collection was done.
      */
-    bool gc_l(std::vector<std::any>& garbage) {
+    bool gc(std::vector<std::any>& garbage) REQUIRES(mLock) {
         if (mLog.size() < mHighWaterMark) return false;
 
         ALOGD("%s: garbage collection", __func__);
@@ -268,7 +268,7 @@ private:
         return true;
     }
 
-    static std::vector<std::shared_ptr<const mediametrics::Item>> getItemsInRange_l(
+    static std::vector<std::shared_ptr<const mediametrics::Item>> getItemsInRange(
             const MapTimeItem& map,
             int64_t startTime = 0, int64_t endTime = INT64_MAX) {
         auto it = map.lower_bound(startTime);
@@ -289,11 +289,8 @@ private:
 
     mutable std::mutex mLock;
 
-    // GUARDED_BY mLock
+    MapTimeItem mLog GUARDED_BY(mLock);
-    MapTimeItem mLog;
+    std::map<std::string /* item_key */, MapTimeItem> mItemMap GUARDED_BY(mLock);
-
-    // GUARDED_BY mLock
-    std::map<std::string /* item_key */, MapTimeItem> mItemMap;
 };
 
 } // namespace android::mediametrics