From ff87cf2d1944f13886dac629cc55926c7ff163c5 Mon Sep 17 00:00:00 2001 From: Karsten Sperling <113487422+ksperling-apple@users.noreply.github.com> Date: Thu, 1 Aug 2024 09:28:53 +1200 Subject: [PATCH] Dump details about leaked ExchangeContexts before aborting (#34617) * Dump details about leaked ExchangeContexts before aborting This is implemented via a VerifyOrDieWithObject() variant of the existing VerifyOrDie() macro that calls a DumpToLog() method on the provided object if it exists (otherwise this is simply a no-op). If CHIP_CONFIG_VERBOSE_VERIFY_OR_DIE is not enabled, VerifyOrDieWithObject() simply behaves like a plain VerifyOrDie(). DumpToLog() implementations can use ChipLogFormatRtti to log type information about an object (usually a delegate); if RTTI is disabled this simply outputs whether the object was null or not. * Address review comments * Make gcc happy and improve documentation * Remove unused include * Fix compile error without CHIP_CONFIG_VERBOSE_VERIFY_OR_DIE * Avoid unused parameter warning --- src/lib/support/BUILD.gn | 6 ++- src/lib/support/CodeUtils.h | 16 +++++++ src/lib/support/Compiler.h | 24 ++++++++++ src/lib/support/ObjectDump.h | 57 +++++++++++++++++++++++ src/lib/support/Pool.h | 29 +++++++++++- src/lib/support/logging/TextOnlyLogging.h | 20 ++++++++ src/messaging/ExchangeContext.cpp | 8 ++-- src/messaging/ExchangeContext.h | 8 +++- src/messaging/tests/TestExchange.cpp | 12 +++++ 9 files changed, 172 insertions(+), 8 deletions(-) create mode 100644 src/lib/support/Compiler.h create mode 100644 src/lib/support/ObjectDump.h diff --git a/src/lib/support/BUILD.gn b/src/lib/support/BUILD.gn index 27f52465858b1d..0b1d353821dabf 100644 --- a/src/lib/support/BUILD.gn +++ b/src/lib/support/BUILD.gn @@ -68,6 +68,7 @@ source_set("logging_constants") { source_set("attributes") { sources = [ + "Compiler.h", "DLLUtil.h", "EnforceFormat.h", ] @@ -132,7 +133,10 @@ source_set("text_only_logging") { } source_set("verifymacros") { - sources = [ "CodeUtils.h" ] + sources = [ + "CodeUtils.h", + "ObjectDump.h", + ] public_deps = [ ":attributes", diff --git a/src/lib/support/CodeUtils.h b/src/lib/support/CodeUtils.h index 800123aa456851..fb659c47939afd 100644 --- a/src/lib/support/CodeUtils.h +++ b/src/lib/support/CodeUtils.h @@ -29,6 +29,7 @@ #include #include #include +#include #include #include @@ -547,6 +548,21 @@ inline void chipDie(void) #define VerifyOrDie(aCondition) VerifyOrDieWithoutLogging(aCondition) #endif // CHIP_CONFIG_VERBOSE_VERIFY_OR_DIE +/** + * @def VerifyOrDieWithObject(aCondition, aObject) + * + * Like VerifyOrDie(), but calls DumpObjectToLog() + * on the provided object on failure before aborting + * if CHIP_CONFIG_VERBOSE_VERIFY_OR_DIE is enabled. + */ +#if CHIP_CONFIG_VERBOSE_VERIFY_OR_DIE +#define VerifyOrDieWithObject(aCondition, aObject) \ + nlABORT_ACTION(aCondition, ::chip::DumpObjectToLog(aObject); \ + ChipLogError(Support, "VerifyOrDie failure at %s:%d: %s", __FILE__, __LINE__, #aCondition)) +#else // CHIP_CONFIG_VERBOSE_VERIFY_OR_DIE +#define VerifyOrDieWithObject(aCondition, aObject) VerifyOrDieWithoutLogging(aCondition) +#endif // CHIP_CONFIG_VERBOSE_VERIFY_OR_DIE + /** * @def VerifyOrDieWithMsg(aCondition, aModule, aMessage, ...) * diff --git a/src/lib/support/Compiler.h b/src/lib/support/Compiler.h new file mode 100644 index 00000000000000..0c26e185a23349 --- /dev/null +++ b/src/lib/support/Compiler.h @@ -0,0 +1,24 @@ +/* + * Copyright (c) 2024 Project CHIP Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +// CHIP_HAVE_RTTI: Is C++ RTTI enabled? +#if defined(__clang__) +#define CHIP_HAVE_RTTI __has_feature(cxx_rtti) +#elif defined(__GNUC__) && defined(__GXX_RTTI) +#define CHIP_HAVE_RTTI 1 +#else +#define CHIP_HAVE_RTTI 0 +#endif diff --git a/src/lib/support/ObjectDump.h b/src/lib/support/ObjectDump.h new file mode 100644 index 00000000000000..d297cd5b590141 --- /dev/null +++ b/src/lib/support/ObjectDump.h @@ -0,0 +1,57 @@ +/* + * + * Copyright (c) 2024 Project CHIP Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#pragma once + +#include +#include + +namespace chip { + +/** + * A dumpable object that can log some useful state for debugging in fatal + * error scenarios by exposing a `void DumpToLog() const` method. The method + * should log key details about the state of object using ChipLogError(). + */ +template +struct IsDumpable : std::false_type +{ +}; +template +struct IsDumpable().DumpToLog())>> : std::true_type +{ +}; + +struct DumpableTypeExample +{ + void DumpToLog() const {}; +}; +static_assert(IsDumpable::value); + +/** + * Calls DumpToLog() on the object, if supported. + */ +template +void DumpObjectToLog([[maybe_unused]] const T * object) +{ + if constexpr (IsDumpable::value) + { + object->DumpToLog(); + } +} + +} // namespace chip diff --git a/src/lib/support/Pool.h b/src/lib/support/Pool.h index e4fb31769f4193..3ab4b8db6713ba 100644 --- a/src/lib/support/Pool.h +++ b/src/lib/support/Pool.h @@ -24,6 +24,7 @@ #include #include +#include #include #include @@ -263,7 +264,7 @@ class BitMapObjectPool : public internal::StaticAllocatorBitmap { public: BitMapObjectPool() : StaticAllocatorBitmap(mData.mMemory, mUsage, N, sizeof(T)) {} - ~BitMapObjectPool() { VerifyOrDie(Allocated() == 0); } + ~BitMapObjectPool() { VerifyOrDieWithObject(Allocated() == 0, this); } BitmapActiveObjectIterator begin() { return BitmapActiveObjectIterator(this, FirstActiveIndex()); } BitmapActiveObjectIterator end() { return BitmapActiveObjectIterator(this, N); } @@ -323,6 +324,18 @@ class BitMapObjectPool : public internal::StaticAllocatorBitmap return ForEachActiveObjectInner(&proxy, &internal::LambdaProxy::ConstCall); } + void DumpToLog() const + { + ChipLogError(Support, "BitMapObjectPool: %lu allocated", static_cast(Allocated())); + if constexpr (IsDumpable::value) + { + ForEachActiveObject([](const T * object) { + object->DumpToLog(); + return Loop::Continue; + }); + } + } + private: static Loop ReleaseObject(void * context, void * object) { @@ -389,7 +402,7 @@ class HeapObjectPool : public internal::Statistics, public HeapObjectPoolExitHan if (!sIgnoringLeaksOnExit) { // Verify that no live objects remain, to prevent potential use-after-free. - VerifyOrDie(Allocated() == 0); + VerifyOrDieWithObject(Allocated() == 0, this); } #endif // __SANITIZE_ADDRESS__ } @@ -570,6 +583,18 @@ class HeapObjectPool : public internal::Statistics, public HeapObjectPoolExitHan return mObjects.ForEachNode(&proxy, &internal::LambdaProxy::ConstCall); } + void DumpToLog() const + { + ChipLogError(Support, "HeapObjectPool: %lu allocated", static_cast(Allocated())); + if constexpr (IsDumpable::value) + { + ForEachActiveObject([](const T * object) { + object->DumpToLog(); + return Loop::Continue; + }); + } + } + private: static Loop ReleaseObject(void * context, void * object) { diff --git a/src/lib/support/logging/TextOnlyLogging.h b/src/lib/support/logging/TextOnlyLogging.h index dd9e73d0f5aeed..903624183a7d6c 100644 --- a/src/lib/support/logging/TextOnlyLogging.h +++ b/src/lib/support/logging/TextOnlyLogging.h @@ -36,6 +36,7 @@ #include +#include #include #include #include @@ -44,6 +45,7 @@ #include #include #include +#include #if CHIP_SYSTEM_CONFIG_PLATFORM_LOG && defined(CHIP_SYSTEM_CONFIG_PLATFORM_LOG_INCLUDE) #include CHIP_SYSTEM_CONFIG_PLATFORM_LOG_INCLUDE @@ -292,6 +294,24 @@ using LogRedirectCallback_t = void (*)(const char * module, uint8_t category, co #define ChipLogValueExchangeIdFromReceivedHeader(payloadHeader) \ ChipLogValueExchangeId((payloadHeader).GetExchangeID(), !(payloadHeader).IsInitiator()) +/** + * Logging helpers for logging the dynamic type of an object, if possible. + * + * Primarily useful when logging the type of delegates or similar objects when + * performing logging for a fatal error in DumpToLog(). + * + * Example: + * @code + * ChipLogError(Foo, "Delegate=" ChipLogFormatRtti, ChipLogValueRtti(mDelegate)); + * @endcode + */ +#define ChipLogFormatRtti "%s" +#if CHIP_HAVE_RTTI +#define ChipLogValueRtti(ptr) ((ptr) != nullptr ? typeid(*(ptr)).name() : "null") +#else +#define ChipLogValueRtti(ptr) ((ptr) != nullptr ? "?" : "null") +#endif + /** * Logging helpers for protocol ids. A protocol id is a (vendor-id, * protocol-id) pair. diff --git a/src/messaging/ExchangeContext.cpp b/src/messaging/ExchangeContext.cpp index 554e5fbce9482d..62c5206f7b272e 100644 --- a/src/messaging/ExchangeContext.cpp +++ b/src/messaging/ExchangeContext.cpp @@ -294,7 +294,7 @@ ExchangeContext::ExchangeContext(ExchangeManager * em, uint16_t ExchangeId, cons mDispatch(GetMessageDispatch(isEphemeralExchange, delegate)), mSession(*this) { - VerifyOrDie(mExchangeMgr == nullptr); + VerifyOrDieWithObject(mExchangeMgr == nullptr, this); mExchangeMgr = em; mExchangeId = ExchangeId; @@ -334,12 +334,12 @@ ExchangeContext::ExchangeContext(ExchangeManager * em, uint16_t ExchangeId, cons ExchangeContext::~ExchangeContext() { - VerifyOrDie(mExchangeMgr != nullptr && GetReferenceCount() == 0); + VerifyOrDieWithObject(mExchangeMgr != nullptr && GetReferenceCount() == 0, this); // // Ensure that DoClose has been called by the time we get here. If not, we have a leak somewhere. // - VerifyOrDie(mFlags.Has(Flags::kFlagClosed)); + VerifyOrDieWithObject(mFlags.Has(Flags::kFlagClosed), this); #if CHIP_CONFIG_ENABLE_ICD_SERVER // TODO(#33075) : Add check for group context to not a req since it serves no purpose @@ -666,7 +666,7 @@ void ExchangeContext::AbortAllOtherCommunicationOnFabric() void ExchangeContext::ExchangeSessionHolder::GrabExpiredSession(const SessionHandle & session) { - VerifyOrDie(session->AsSecureSession()->IsPendingEviction()); + VerifyOrDieWithObject(session->AsSecureSession()->IsPendingEviction(), this); GrabUnchecked(session); } diff --git a/src/messaging/ExchangeContext.h b/src/messaging/ExchangeContext.h index 47cf0ddbef2783..e10ef84ce911be 100644 --- a/src/messaging/ExchangeContext.h +++ b/src/messaging/ExchangeContext.h @@ -158,7 +158,7 @@ class DLL_EXPORT ExchangeContext : public ReliableMessageContext, SessionHandle GetSessionHandle() const { - VerifyOrDie(mSession); + VerifyOrDieWithObject(mSession, this); auto sessionHandle = mSession.Get(); return std::move(sessionHandle.Value()); } @@ -238,6 +238,12 @@ class DLL_EXPORT ExchangeContext : public ReliableMessageContext, void ClearInjectedFailures() { mInjectedFailures.ClearAll(); } #endif + void DumpToLog() const + { + ChipLogError(ExchangeManager, "ExchangeContext: " ChipLogFormatExchangeId " delegate=" ChipLogFormatRtti, + ChipLogValueExchangeId(GetExchangeId(), IsInitiator()), ChipLogValueRtti(mDelegate)); + } + private: #if CONFIG_BUILD_FOR_HOST_UNIT_TEST BitFlags mInjectedFailures; diff --git a/src/messaging/tests/TestExchange.cpp b/src/messaging/tests/TestExchange.cpp index f221c999ac4e22..20df66f9d143d9 100644 --- a/src/messaging/tests/TestExchange.cpp +++ b/src/messaging/tests/TestExchange.cpp @@ -23,6 +23,7 @@ #include #include #include +#include #include #include #include @@ -219,4 +220,15 @@ TEST_F(TestExchange, CheckBasicExchangeMessageDispatch) }); } } + +// A crude test to exercise VerifyOrDieWithObject() in ObjectPool and +// the resulting DumpToLog() call on the ExchangeContext. +// TODO: Find a way to automate this test without killing the process. +// TEST_F(TestExchange, DumpExchangePoolToLog) +// { +// MockExchangeDelegate delegate; +// ObjectPool pool; +// pool.CreateObject(&GetExchangeManager(), static_cast(1234), GetSessionAliceToBob(), true, &delegate); +// } + } // namespace