Skip to content

Commit 5e13e97

Browse files
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.
1 parent c63d1a9 commit 5e13e97

File tree

5 files changed

+80
-21
lines changed

5 files changed

+80
-21
lines changed

src/lib/support/BUILD.gn

+37-11
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,40 @@ source_set("attributes") {
7373
]
7474
}
7575

76+
source_set("verifymacros_no_logging") {
77+
sources = [ "VerificationMacrosNoLogging.h" ]
78+
79+
public_deps = [ "${nlassert_root}:nlassert" ]
80+
}
81+
82+
source_set("safeint") {
83+
sources = [ "SafeInt.h" ]
84+
}
85+
86+
source_set("memory") {
87+
sources = [
88+
"CHIPMem.cpp",
89+
"CHIPMem.h",
90+
"CHIPPlatformMemory.cpp",
91+
"CHIPPlatformMemory.h",
92+
]
93+
94+
if (chip_config_memory_management == "simple") {
95+
sources += [ "CHIPMem-Simple.cpp" ]
96+
}
97+
if (chip_config_memory_management == "malloc") {
98+
sources += [ "CHIPMem-Malloc.cpp" ]
99+
}
100+
101+
public_deps = [ "${chip_root}/src/lib/core:error" ]
102+
103+
deps = [
104+
":safeint",
105+
":verifymacros_no_logging",
106+
"${chip_root}/src/lib/core:chip_config_header",
107+
]
108+
}
109+
76110
source_set("chip_version_header") {
77111
sources = get_target_outputs(":gen_chip_version")
78112

@@ -97,11 +131,7 @@ static_library("support") {
97131
"BytesToHex.h",
98132
"CHIPArgParser.cpp",
99133
"CHIPCounter.h",
100-
"CHIPMem.cpp",
101-
"CHIPMem.h",
102134
"CHIPMemString.h",
103-
"CHIPPlatformMemory.cpp",
104-
"CHIPPlatformMemory.h",
105135
"CodeUtils.h",
106136
"DLLUtil.h",
107137
"DefaultStorageKeyAllocator.h",
@@ -124,7 +154,6 @@ static_library("support") {
124154
"PrivateHeap.cpp",
125155
"PrivateHeap.h",
126156
"ReferenceCountedHandle.h",
127-
"SafeInt.h",
128157
"SerializableIntegerSet.cpp",
129158
"SerializableIntegerSet.h",
130159
"SetupDiscriminator.h",
@@ -172,6 +201,9 @@ static_library("support") {
172201
":attributes",
173202
":chip_version_header",
174203
":logging_constants",
204+
":memory",
205+
":safeint",
206+
":verifymacros_no_logging",
175207
"${chip_root}/src/lib/core:chip_config_header",
176208
"${chip_root}/src/lib/core:error",
177209
"${chip_root}/src/platform:platform_buildconfig",
@@ -203,12 +235,6 @@ static_library("support") {
203235
":storage_audit_config",
204236
]
205237

206-
if (chip_config_memory_management == "simple") {
207-
sources += [ "CHIPMem-Simple.cpp" ]
208-
}
209-
if (chip_config_memory_management == "malloc") {
210-
sources += [ "CHIPMem-Malloc.cpp" ]
211-
}
212238
if (chip_with_nlfaultinjection) {
213239
sources += [
214240
"CHIPFaultInjection.cpp",

src/lib/support/CHIPMem-Malloc.cpp

+13-8
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@
2525

2626
#include <lib/core/CHIPConfig.h>
2727
#include <lib/support/CHIPMem.h>
28-
#include <lib/support/CodeUtils.h>
28+
#include <lib/support/VerificationMacrosNoLogging.h>
2929

3030
#include <stdlib.h>
3131

@@ -57,28 +57,33 @@ static std::atomic_int memoryInitialized{ 0 };
5757

5858
static void VerifyInitialized(const char * func)
5959
{
60-
VerifyOrDieWithMsg(memoryInitialized, Support, "ABORT: chip::Platform::%s() called before chip::Platform::MemoryInit()\n",
61-
func);
60+
// Logging can use Memory::Alloc, so we can't use logging with our
61+
// VerifyOrDie bits here.
62+
VerifyOrDieWithoutLogging(memoryInitialized);
6263
}
6364

64-
#define VERIFY_POINTER(p) \
65-
VerifyOrDieWithMsg((p) == nullptr || MemoryDebugCheckPointer((p)), Support, \
66-
"ABORT: chip::Platform::%s() found corruption on %p\n", __func__, (p))
65+
// Logging can use Memory::Alloc, so we can't use logging with our
66+
// VerifyOrDie bits here.
67+
#define VERIFY_POINTER(p) VerifyOrDieWithoutLogging((p) == nullptr || MemoryDebugCheckPointer((p)))
6768

6869
#endif
6970

7071
CHIP_ERROR MemoryAllocatorInit(void * buf, size_t bufSize)
7172
{
73+
// Logging can use Memory::Alloc, so we can't use logging with our
74+
// VerifyOrDie bits here.
7275
#ifndef NDEBUG
73-
VerifyOrDieWithMsg(memoryInitialized++ == 0, Support, "ABORT: chip::Platform::MemoryInit() called twice.\n");
76+
VerifyOrDieWithoutLogging(memoryInitialized++ == 0);
7477
#endif
7578
return CHIP_NO_ERROR;
7679
}
7780

7881
void MemoryAllocatorShutdown()
7982
{
83+
// Logging can use Memory::Alloc, so we can't use logging with our
84+
// VerifyOrDie bits here.
8085
#ifndef NDEBUG
81-
VerifyOrDieWithMsg(--memoryInitialized == 0, Support, "ABORT: chip::Platform::MemoryShutdown() called twice.\n");
86+
VerifyOrDieWithoutLogging(--memoryInitialized == 0);
8287
#endif
8388
#if CHIP_CONFIG_MEMORY_DEBUG_DMALLOC
8489
dmalloc_shutdown();

src/lib/support/CHIPMem-Simple.cpp

+1-1
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@
2020

2121
#include <string.h>
2222

23-
#include <lib/support/CodeUtils.h>
23+
#include <lib/support/VerificationMacrosNoLogging.h>
2424
#include <system/SystemMutex.h>
2525

2626
namespace chip {

src/lib/support/CodeUtils.h

+2-1
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
#include <lib/core/CHIPConfig.h>
3030
#include <lib/core/CHIPError.h>
3131
#include <lib/core/ErrorStr.h>
32+
#include <lib/support/VerificationMacrosNoLogging.h>
3233
#include <lib/support/logging/TextOnlyLogging.h>
3334

3435
/**
@@ -547,7 +548,7 @@ inline void chipDie(void)
547548
#define VerifyOrDie(aCondition) \
548549
nlABORT_ACTION(aCondition, ChipLogDetail(Support, "VerifyOrDie failure at %s:%d: %s", __FILE__, __LINE__, #aCondition))
549550
#else // CHIP_CONFIG_VERBOSE_VERIFY_OR_DIE
550-
#define VerifyOrDie(aCondition) nlABORT(aCondition)
551+
#define VerifyOrDie(aCondition) VerifyOrDieWithoutLogging(aCondition)
551552
#endif // CHIP_CONFIG_VERBOSE_VERIFY_OR_DIE
552553

553554
/**
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
/*
2+
* Copyright (c) 2023 Project CHIP Authors
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
/**
18+
* @file
19+
* This file defines and implements macros for verifying values that do not
20+
* involve logging.
21+
*/
22+
23+
#pragma once
24+
25+
#include <nlassert.h>
26+
27+
#define VerifyOrDieWithoutLogging(aCondition) nlABORT(aCondition)

0 commit comments

Comments
 (0)