Skip to content

Commit

Permalink
Break dependency cycle between CHIPMem and logging. (#29685)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
bzbarsky-apple authored and pull[bot] committed Oct 31, 2023
1 parent b0c7b50 commit ba157d2
Show file tree
Hide file tree
Showing 5 changed files with 80 additions and 21 deletions.
48 changes: 37 additions & 11 deletions src/lib/support/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -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")

Expand All @@ -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",
Expand All @@ -124,7 +154,6 @@ static_library("support") {
"PrivateHeap.cpp",
"PrivateHeap.h",
"ReferenceCountedHandle.h",
"SafeInt.h",
"SerializableIntegerSet.cpp",
"SerializableIntegerSet.h",
"SetupDiscriminator.h",
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand Down
21 changes: 13 additions & 8 deletions src/lib/support/CHIPMem-Malloc.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@

#include <lib/core/CHIPConfig.h>
#include <lib/support/CHIPMem.h>
#include <lib/support/CodeUtils.h>
#include <lib/support/VerificationMacrosNoLogging.h>

#include <stdlib.h>

Expand Down Expand Up @@ -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();
Expand Down
2 changes: 1 addition & 1 deletion src/lib/support/CHIPMem-Simple.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@

#include <string.h>

#include <lib/support/CodeUtils.h>
#include <lib/support/VerificationMacrosNoLogging.h>
#include <system/SystemMutex.h>

namespace chip {
Expand Down
3 changes: 2 additions & 1 deletion src/lib/support/CodeUtils.h
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
#include <lib/core/CHIPConfig.h>
#include <lib/core/CHIPError.h>
#include <lib/core/ErrorStr.h>
#include <lib/support/VerificationMacrosNoLogging.h>
#include <lib/support/logging/TextOnlyLogging.h>

/**
Expand Down Expand Up @@ -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

/**
Expand Down
27 changes: 27 additions & 0 deletions src/lib/support/VerificationMacrosNoLogging.h
Original file line number Diff line number Diff line change
@@ -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 <nlassert.h>

#define VerifyOrDieWithoutLogging(aCondition) nlABORT(aCondition)

0 comments on commit ba157d2

Please sign in to comment.