From 5e13e9751f83e08abe775d7efa9076689f47a202 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Wed, 11 Oct 2023 16:02:17 -0400 Subject: [PATCH] Break dependency cycle between CHIPMem and logging. (#29685) CHIPMem used things from CodeUtils.h which used logging, but logging might need to allocate memory via Memory::Alloc to actually work. This breaks the cycle (both conceptual and real: we could try to Memory::Alloc to log the fact that Memory::Init had not been called!), by making CHIPMem not use any macros that involve logging, at the cost of not having nice error messages when CHIPMem invariants are violated. --- src/lib/support/BUILD.gn | 48 ++++++++++++++----- src/lib/support/CHIPMem-Malloc.cpp | 21 ++++---- src/lib/support/CHIPMem-Simple.cpp | 2 +- src/lib/support/CodeUtils.h | 3 +- src/lib/support/VerificationMacrosNoLogging.h | 27 +++++++++++ 5 files changed, 80 insertions(+), 21 deletions(-) create mode 100644 src/lib/support/VerificationMacrosNoLogging.h diff --git a/src/lib/support/BUILD.gn b/src/lib/support/BUILD.gn index ae745b52b8ef28..e516b6e412280a 100644 --- a/src/lib/support/BUILD.gn +++ b/src/lib/support/BUILD.gn @@ -73,6 +73,40 @@ source_set("attributes") { ] } +source_set("verifymacros_no_logging") { + sources = [ "VerificationMacrosNoLogging.h" ] + + public_deps = [ "${nlassert_root}:nlassert" ] +} + +source_set("safeint") { + sources = [ "SafeInt.h" ] +} + +source_set("memory") { + sources = [ + "CHIPMem.cpp", + "CHIPMem.h", + "CHIPPlatformMemory.cpp", + "CHIPPlatformMemory.h", + ] + + if (chip_config_memory_management == "simple") { + sources += [ "CHIPMem-Simple.cpp" ] + } + if (chip_config_memory_management == "malloc") { + sources += [ "CHIPMem-Malloc.cpp" ] + } + + public_deps = [ "${chip_root}/src/lib/core:error" ] + + deps = [ + ":safeint", + ":verifymacros_no_logging", + "${chip_root}/src/lib/core:chip_config_header", + ] +} + source_set("chip_version_header") { sources = get_target_outputs(":gen_chip_version") @@ -97,11 +131,7 @@ static_library("support") { "BytesToHex.h", "CHIPArgParser.cpp", "CHIPCounter.h", - "CHIPMem.cpp", - "CHIPMem.h", "CHIPMemString.h", - "CHIPPlatformMemory.cpp", - "CHIPPlatformMemory.h", "CodeUtils.h", "DLLUtil.h", "DefaultStorageKeyAllocator.h", @@ -124,7 +154,6 @@ static_library("support") { "PrivateHeap.cpp", "PrivateHeap.h", "ReferenceCountedHandle.h", - "SafeInt.h", "SerializableIntegerSet.cpp", "SerializableIntegerSet.h", "SetupDiscriminator.h", @@ -172,6 +201,9 @@ static_library("support") { ":attributes", ":chip_version_header", ":logging_constants", + ":memory", + ":safeint", + ":verifymacros_no_logging", "${chip_root}/src/lib/core:chip_config_header", "${chip_root}/src/lib/core:error", "${chip_root}/src/platform:platform_buildconfig", @@ -203,12 +235,6 @@ static_library("support") { ":storage_audit_config", ] - if (chip_config_memory_management == "simple") { - sources += [ "CHIPMem-Simple.cpp" ] - } - if (chip_config_memory_management == "malloc") { - sources += [ "CHIPMem-Malloc.cpp" ] - } if (chip_with_nlfaultinjection) { sources += [ "CHIPFaultInjection.cpp", diff --git a/src/lib/support/CHIPMem-Malloc.cpp b/src/lib/support/CHIPMem-Malloc.cpp index ec247a5a2322f7..7ef04256d3d47a 100644 --- a/src/lib/support/CHIPMem-Malloc.cpp +++ b/src/lib/support/CHIPMem-Malloc.cpp @@ -25,7 +25,7 @@ #include #include -#include +#include #include @@ -57,28 +57,33 @@ static std::atomic_int memoryInitialized{ 0 }; static void VerifyInitialized(const char * func) { - VerifyOrDieWithMsg(memoryInitialized, Support, "ABORT: chip::Platform::%s() called before chip::Platform::MemoryInit()\n", - func); + // Logging can use Memory::Alloc, so we can't use logging with our + // VerifyOrDie bits here. + VerifyOrDieWithoutLogging(memoryInitialized); } -#define VERIFY_POINTER(p) \ - VerifyOrDieWithMsg((p) == nullptr || MemoryDebugCheckPointer((p)), Support, \ - "ABORT: chip::Platform::%s() found corruption on %p\n", __func__, (p)) +// Logging can use Memory::Alloc, so we can't use logging with our +// VerifyOrDie bits here. +#define VERIFY_POINTER(p) VerifyOrDieWithoutLogging((p) == nullptr || MemoryDebugCheckPointer((p))) #endif CHIP_ERROR MemoryAllocatorInit(void * buf, size_t bufSize) { + // Logging can use Memory::Alloc, so we can't use logging with our + // VerifyOrDie bits here. #ifndef NDEBUG - VerifyOrDieWithMsg(memoryInitialized++ == 0, Support, "ABORT: chip::Platform::MemoryInit() called twice.\n"); + VerifyOrDieWithoutLogging(memoryInitialized++ == 0); #endif return CHIP_NO_ERROR; } void MemoryAllocatorShutdown() { + // Logging can use Memory::Alloc, so we can't use logging with our + // VerifyOrDie bits here. #ifndef NDEBUG - VerifyOrDieWithMsg(--memoryInitialized == 0, Support, "ABORT: chip::Platform::MemoryShutdown() called twice.\n"); + VerifyOrDieWithoutLogging(--memoryInitialized == 0); #endif #if CHIP_CONFIG_MEMORY_DEBUG_DMALLOC dmalloc_shutdown(); diff --git a/src/lib/support/CHIPMem-Simple.cpp b/src/lib/support/CHIPMem-Simple.cpp index 3a1e0aa14cdd38..8816afeaed21c5 100644 --- a/src/lib/support/CHIPMem-Simple.cpp +++ b/src/lib/support/CHIPMem-Simple.cpp @@ -20,7 +20,7 @@ #include -#include +#include #include namespace chip { diff --git a/src/lib/support/CodeUtils.h b/src/lib/support/CodeUtils.h index 60780a689ef82a..bbb18544c3ead6 100644 --- a/src/lib/support/CodeUtils.h +++ b/src/lib/support/CodeUtils.h @@ -29,6 +29,7 @@ #include #include #include +#include #include /** @@ -547,7 +548,7 @@ inline void chipDie(void) #define VerifyOrDie(aCondition) \ nlABORT_ACTION(aCondition, ChipLogDetail(Support, "VerifyOrDie failure at %s:%d: %s", __FILE__, __LINE__, #aCondition)) #else // CHIP_CONFIG_VERBOSE_VERIFY_OR_DIE -#define VerifyOrDie(aCondition) nlABORT(aCondition) +#define VerifyOrDie(aCondition) VerifyOrDieWithoutLogging(aCondition) #endif // CHIP_CONFIG_VERBOSE_VERIFY_OR_DIE /** diff --git a/src/lib/support/VerificationMacrosNoLogging.h b/src/lib/support/VerificationMacrosNoLogging.h new file mode 100644 index 00000000000000..8846390557c611 --- /dev/null +++ b/src/lib/support/VerificationMacrosNoLogging.h @@ -0,0 +1,27 @@ +/* + * Copyright (c) 2023 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. + */ + +/** + * @file + * This file defines and implements macros for verifying values that do not + * involve logging. + */ + +#pragma once + +#include + +#define VerifyOrDieWithoutLogging(aCondition) nlABORT(aCondition)