Darwin: Avoid autorelease handling around LoggerForModule (#35884)

We don't want the LoggerForModule() implementation to call retain/autorelease on the logger, because its unnecessary, and because LoggerForModule() can be called from a C++ context without an auto-release pool in place. To avoid this, we can have LoggerForModule() return a raw pointer, and handle the bridge cast to os_log_t on the caller side. This also avoids unnecessary code on the caller side that would otherwise attempt to rescue the presumed-autoreleased logger object from the ARP.
project-chip · Oct 3, 2024 · af1b0ee · af1b0ee
1 parent 0320653
commit af1b0ee
Show file tree

Hide file tree

Showing 2 changed files with 18 additions and 10 deletions.
diff --git a/src/platform/Darwin/Logging.h b/src/platform/Darwin/Logging.h
@@ -31,7 +31,7 @@
         ChipPlatformValidateLogFormat(MSG, ##__VA_ARGS__); /* validate once and ignore warnings from os_log() / Log() */           \
         _Pragma("clang diagnostic push");                                                                                          \
         _Pragma("clang diagnostic ignored \"-Wformat\"");                                                                          \
-        os_log_with_type(chip::Logging::Platform::LoggerForModule(chip::Logging::kLogModule_##MOD, #MOD),                          \
+        os_log_with_type(ChipPlatformLogger(::chip::Logging::Platform::LoggerForModule(chip::Logging::kLogModule_##MOD, #MOD)),    \
                          static_cast<os_log_type_t>(chip::Logging::Platform::kOSLogCategory_##CAT), MSG, ##__VA_ARGS__);           \
         ChipInternalLogImpl(MOD, CHIP_LOG_CATEGORY_##CAT, MSG, ##__VA_ARGS__);                                                     \
         _Pragma("clang diagnostic pop");                                                                                           \
@@ -40,8 +40,8 @@
 #define ChipPlatformLogByteSpan(MOD, CAT, DATA)                                                                                    \
     do                                                                                                                             \
     {                                                                                                                              \
-        chip::Logging::Platform::LogByteSpan(chip::Logging::kLogModule_##MOD, #MOD,                                                \
-                                             static_cast<os_log_type_t>(chip::Logging::Platform::kOSLogCategory_##CAT), DATA);     \
+        ::chip::Logging::Platform::LogByteSpan(chip::Logging::kLogModule_##MOD, #MOD,                                              \
+                                               static_cast<os_log_type_t>(chip::Logging::Platform::kOSLogCategory_##CAT), DATA);   \
         ChipInternalLogByteSpanImpl(MOD, CHIP_LOG_CATEGORY_##CAT, DATA);                                                           \
     } while (0)
 
@@ -64,7 +64,15 @@ enum OSLogCategory
     kOSLogCategory_AUTOMATION = OS_LOG_TYPE_DEFAULT,
 };
 
-os_log_t LoggerForModule(chip::Logging::LogModule moduleId, char const * moduleName);
+// Note: A raw pointer is used here instead of os_log_t to avoid an unwanted retain/autorelease
+// in the function itself, as well as unnecessary code to rescue the object from the ARP in callers.
+struct os_log_s * LoggerForModule(chip::Logging::LogModule moduleId, char const * moduleName);
+#ifdef __OBJC__
+#define ChipPlatformLogger(log) ((__bridge os_log_t)(log))
+#else
+#define ChipPlatformLogger(log) (log)
+#endif
+
 void LogByteSpan(chip::Logging::LogModule moduleId, char const * moduleName, os_log_type_t type, const chip::ByteSpan & span);
 
 // Helper constructs for compile-time validation of format strings for C++ / ObjC++ contexts.

diff --git a/src/platform/Darwin/Logging.mm b/src/platform/Darwin/Logging.mm
@@ -30,7 +30,7 @@
 namespace Logging {
     namespace Platform {
 
-        os_log_t LoggerForModule(chip::Logging::LogModule moduleID, char const * moduleName)
+        struct os_log_s * LoggerForModule(chip::Logging::LogModule moduleID, char const * moduleName)
         {
             if (moduleID <= kLogModule_NotSpecified || kLogModule_Max <= moduleID) {
                 moduleID = kLogModule_NotSpecified;
@@ -39,27 +39,27 @@ os_log_t LoggerForModule(chip::Logging::LogModule moduleID, char const * moduleN
 
             static struct {
                 dispatch_once_t onceToken;
-                os_log_t logger;
+                struct os_log_s * logger;
             } cache[kLogModule_Max];
             auto & entry = cache[moduleID];
             dispatch_once(&entry.onceToken, ^{
-                entry.logger = os_log_create("com.csa.matter", moduleName);
+                entry.logger = (__bridge_retained struct os_log_s *) os_log_create("com.csa.matter", moduleName);
             });
             return entry.logger;
         }
 
         void LogByteSpan(
             chip::Logging::LogModule moduleId, char const * moduleName, os_log_type_t type, const chip::ByteSpan & span)
         {
-            os_log_t logger = LoggerForModule(moduleId, moduleName);
-            if (os_log_type_enabled(logger, type)) {
+            auto * logger = LoggerForModule(moduleId, moduleName);
+            if (os_log_type_enabled(ChipPlatformLogger(logger), type)) {
                 auto size = span.size();
                 auto data = span.data();
                 NSMutableString * string = [[NSMutableString alloc] initWithCapacity:(size * 6)]; // 6 characters per byte
                 for (size_t i = 0; i < size; i++) {
                     [string appendFormat:((i % 8 != 7) ? @"0x%02x, " : @"0x%02x,\n"), data[i]];
                 }
-                os_log_with_type(logger, type, "%@", string);
+                os_log_with_type(ChipPlatformLogger(logger), type, "%@", string);
             }
         }