Skip to content

platform: fix mem_trace trace level guard #5614

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Dec 29, 2017
Merged

platform: fix mem_trace trace level guard #5614

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 32 additions & 19 deletions platform/mbed_alloc_wrappers.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,6 @@ typedef struct {
uint32_t pad;
} alloc_info_t;

#ifdef MBED_MEM_TRACING_ENABLED
static SingletonPtr<PlatformMutex> mem_trace_mutex;
#endif
#ifdef MBED_HEAP_STATS_ENABLED
static SingletonPtr<PlatformMutex> malloc_stats_mutex;
static mbed_stats_heap_t heap_stats = {0, 0, 0, 0, 0};
Expand Down Expand Up @@ -91,6 +88,9 @@ extern "C" {

extern "C" void * __wrap__malloc_r(struct _reent * r, size_t size) {
void *ptr = NULL;
#ifdef MBED_MEM_TRACING_ENABLED
mbed_mem_trace_lock();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The memory trace mutex should be held here, otherwise another thread my have acquired this lock but not the mutex, causing logging on other threads to be dropped. Could mbed_mem_trace_lock() include mem_trace_mutex->lock() to simply this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mistakenly assumed that whole __wrap__malloc_r is protected by __malloc_lock but actually __real__malloc_r is protected

Copy link
Contributor Author

@maciejbocianski maciejbocianski Dec 12, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure we could encapsulate mem_trace_mutex inside mbed_mem_trace.c module but since it's C file we have to use raw OS mutex.
Other solution could be changing this module to cpp and use SingletonPtr<PlatformMutex>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure which solution will be acceptable?

#endif
#ifdef MBED_HEAP_STATS_ENABLED
malloc_stats_mutex->lock();
alloc_info_t *alloc_info = (alloc_info_t*)__real__malloc_r(r, size + sizeof(alloc_info_t));
Expand All @@ -111,15 +111,17 @@ extern "C" void * __wrap__malloc_r(struct _reent * r, size_t size) {
ptr = __real__malloc_r(r, size);
#endif // #ifdef MBED_HEAP_STATS_ENABLED
#ifdef MBED_MEM_TRACING_ENABLED
mem_trace_mutex->lock();
mbed_mem_trace_malloc(ptr, size, MBED_CALLER_ADDR());
mem_trace_mutex->unlock();
mbed_mem_trace_unlock();
#endif // #ifdef MBED_MEM_TRACING_ENABLED
return ptr;
}

extern "C" void * __wrap__realloc_r(struct _reent * r, void * ptr, size_t size) {
void *new_ptr = NULL;
#ifdef MBED_MEM_TRACING_ENABLED
mbed_mem_trace_lock();
#endif
#ifdef MBED_HEAP_STATS_ENABLED
// Implement realloc_r with malloc and free.
// The function realloc_r can't be used here directly since
Expand Down Expand Up @@ -151,14 +153,16 @@ extern "C" void * __wrap__realloc_r(struct _reent * r, void * ptr, size_t size)
new_ptr = __real__realloc_r(r, ptr, size);
#endif // #ifdef MBED_HEAP_STATS_ENABLED
#ifdef MBED_MEM_TRACING_ENABLED
mem_trace_mutex->lock();
mbed_mem_trace_realloc(new_ptr, ptr, size, MBED_CALLER_ADDR());
mem_trace_mutex->unlock();
mbed_mem_trace_unlock();
#endif // #ifdef MBED_MEM_TRACING_ENABLED
return new_ptr;
}

extern "C" void __wrap__free_r(struct _reent * r, void * ptr) {
#ifdef MBED_MEM_TRACING_ENABLED
mbed_mem_trace_lock();
#endif
#ifdef MBED_HEAP_STATS_ENABLED
malloc_stats_mutex->lock();
alloc_info_t *alloc_info = NULL;
Expand All @@ -173,14 +177,16 @@ extern "C" void __wrap__free_r(struct _reent * r, void * ptr) {
__real__free_r(r, ptr);
#endif // #ifdef MBED_HEAP_STATS_ENABLED
#ifdef MBED_MEM_TRACING_ENABLED
mem_trace_mutex->lock();
mbed_mem_trace_free(ptr, MBED_CALLER_ADDR());
mem_trace_mutex->unlock();
mbed_mem_trace_unlock();
#endif // #ifdef MBED_MEM_TRACING_ENABLED
}

extern "C" void * __wrap__calloc_r(struct _reent * r, size_t nmemb, size_t size) {
void *ptr = NULL;
#ifdef MBED_MEM_TRACING_ENABLED
mbed_mem_trace_lock();
#endif
#ifdef MBED_HEAP_STATS_ENABLED
// Note - no lock needed since malloc is thread safe

Expand All @@ -192,9 +198,8 @@ extern "C" void * __wrap__calloc_r(struct _reent * r, size_t nmemb, size_t size)
ptr = __real__calloc_r(r, nmemb, size);
#endif // #ifdef MBED_HEAP_STATS_ENABLED
#ifdef MBED_MEM_TRACING_ENABLED
mem_trace_mutex->lock();
mbed_mem_trace_calloc(ptr, nmemb, size, MBED_CALLER_ADDR());
mem_trace_mutex->unlock();
mbed_mem_trace_unlock();
#endif // #ifdef MBED_MEM_TRACING_ENABLED
return ptr;
}
Expand Down Expand Up @@ -244,6 +249,9 @@ extern "C" {

extern "C" void* SUB_MALLOC(size_t size) {
void *ptr = NULL;
#ifdef MBED_MEM_TRACING_ENABLED
mbed_mem_trace_lock();
#endif
#ifdef MBED_HEAP_STATS_ENABLED
malloc_stats_mutex->lock();
alloc_info_t *alloc_info = (alloc_info_t*)SUPER_MALLOC(size + sizeof(alloc_info_t));
Expand All @@ -264,15 +272,17 @@ extern "C" void* SUB_MALLOC(size_t size) {
ptr = SUPER_MALLOC(size);
#endif // #ifdef MBED_HEAP_STATS_ENABLED
#ifdef MBED_MEM_TRACING_ENABLED
mem_trace_mutex->lock();
mbed_mem_trace_malloc(ptr, size, MBED_CALLER_ADDR());
mem_trace_mutex->unlock();
mbed_mem_trace_unlock();
#endif // #ifdef MBED_MEM_TRACING_ENABLED
return ptr;
}

extern "C" void* SUB_REALLOC(void *ptr, size_t size) {
void *new_ptr = NULL;
#ifdef MBED_MEM_TRACING_ENABLED
mbed_mem_trace_lock();
#endif
#ifdef MBED_HEAP_STATS_ENABLED
// Note - no lock needed since malloc and free are thread safe

Expand All @@ -299,15 +309,17 @@ extern "C" void* SUB_REALLOC(void *ptr, size_t size) {
new_ptr = SUPER_REALLOC(ptr, size);
#endif // #ifdef MBED_HEAP_STATS_ENABLED
#ifdef MBED_MEM_TRACING_ENABLED
mem_trace_mutex->lock();
mbed_mem_trace_realloc(new_ptr, ptr, size, MBED_CALLER_ADDR());
mem_trace_mutex->unlock();
mbed_mem_trace_unlock();
#endif // #ifdef MBED_MEM_TRACING_ENABLED
return new_ptr;
}

extern "C" void *SUB_CALLOC(size_t nmemb, size_t size) {
void *ptr = NULL;
#ifdef MBED_MEM_TRACING_ENABLED
mbed_mem_trace_lock();
#endif
#ifdef MBED_HEAP_STATS_ENABLED
// Note - no lock needed since malloc is thread safe
ptr = malloc(nmemb * size);
Expand All @@ -318,14 +330,16 @@ extern "C" void *SUB_CALLOC(size_t nmemb, size_t size) {
ptr = SUPER_CALLOC(nmemb, size);
#endif // #ifdef MBED_HEAP_STATS_ENABLED
#ifdef MBED_MEM_TRACING_ENABLED
mem_trace_mutex->lock();
mbed_mem_trace_calloc(ptr, nmemb, size, MBED_CALLER_ADDR());
mem_trace_mutex->unlock();
mbed_mem_trace_unlock();
#endif // #ifdef MBED_MEM_TRACING_ENABLED
return ptr;
}

extern "C" void SUB_FREE(void *ptr) {
#ifdef MBED_MEM_TRACING_ENABLED
mbed_mem_trace_lock();
#endif
#ifdef MBED_HEAP_STATS_ENABLED
malloc_stats_mutex->lock();
alloc_info_t *alloc_info = NULL;
Expand All @@ -340,9 +354,8 @@ extern "C" void SUB_FREE(void *ptr) {
SUPER_FREE(ptr);
#endif // #ifdef MBED_HEAP_STATS_ENABLED
#ifdef MBED_MEM_TRACING_ENABLED
mem_trace_mutex->lock();
mbed_mem_trace_free(ptr, MBED_CALLER_ADDR());
mem_trace_mutex->unlock();
mbed_mem_trace_unlock();
#endif // #ifdef MBED_MEM_TRACING_ENABLED
}

Expand Down
34 changes: 24 additions & 10 deletions platform/mbed_mem_trace.c → platform/mbed_mem_trace.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,17 +19,23 @@
#include <stdio.h>
#include "platform/mbed_mem_trace.h"
#include "platform/mbed_critical.h"
#include "platform/SingletonPtr.h"
#include "platform/PlatformMutex.h"

/******************************************************************************
* Internal variables, functions and helpers
*****************************************************************************/

/* The callback function that will be called after a traced memory operations finishes. */
static mbed_mem_trace_cb_t mem_trace_cb;
/* 'trave_level' guards "trace inside trace" situations (for example, the implementation
/* 'trace_lock_count' guards "trace inside trace" situations (for example, the implementation
* of realloc() might call malloc() internally, and since malloc() is also traced, this could
* result in two calls to the callback function instead of one. */
static uint8_t trace_level;
static uint8_t trace_lock_count;
static SingletonPtr<PlatformMutex> mem_trace_mutex;

#define TRACE_FIRST_LOCK() (trace_lock_count < 2)


/******************************************************************************
* Public interface
Expand All @@ -39,42 +45,50 @@ void mbed_mem_trace_set_callback(mbed_mem_trace_cb_t cb) {
mem_trace_cb = cb;
}

void mbed_mem_trace_lock()
{
mem_trace_mutex->lock();
trace_lock_count++;
}

void mbed_mem_trace_unlock()
{
trace_lock_count--;
mem_trace_mutex->unlock();
}

void *mbed_mem_trace_malloc(void *res, size_t size, void *caller) {
if (mem_trace_cb) {
if (core_util_atomic_incr_u8(&trace_level, 1) == 1) {
if (TRACE_FIRST_LOCK()) {
mem_trace_cb(MBED_MEM_TRACE_MALLOC, res, caller, size);
}
core_util_atomic_decr_u8(&trace_level, 1);
}
return res;
}

void *mbed_mem_trace_realloc(void *res, void *ptr, size_t size, void *caller) {
if (mem_trace_cb) {
if (core_util_atomic_incr_u8(&trace_level, 1) == 1) {
if (TRACE_FIRST_LOCK()) {
mem_trace_cb(MBED_MEM_TRACE_REALLOC, res, caller, ptr, size);
}
core_util_atomic_decr_u8(&trace_level, 1);
}
return res;
}

void *mbed_mem_trace_calloc(void *res, size_t num, size_t size, void *caller) {
if (mem_trace_cb) {
if (core_util_atomic_incr_u8(&trace_level, 1) == 1) {
if (TRACE_FIRST_LOCK()) {
mem_trace_cb(MBED_MEM_TRACE_CALLOC, res, caller, num, size);
}
core_util_atomic_decr_u8(&trace_level, 1);
}
return res;
}

void mbed_mem_trace_free(void *ptr, void *caller) {
if (mem_trace_cb) {
if (core_util_atomic_incr_u8(&trace_level, 1) == 1) {
if (TRACE_FIRST_LOCK()) {
mem_trace_cb(MBED_MEM_TRACE_FREE, NULL, caller, ptr);
}
core_util_atomic_decr_u8(&trace_level, 1);
}
}

Expand Down
11 changes: 11 additions & 0 deletions platform/mbed_mem_trace.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,17 @@ typedef void (*mbed_mem_trace_cb_t)(uint8_t op, void *res, void* caller, ...);
*/
void mbed_mem_trace_set_callback(mbed_mem_trace_cb_t cb);

/**
* Trace lock.
* @note Locking prevent recursive tracing of malloc/free inside relloc/calloc
*/
void mbed_mem_trace_lock();

/**
* Trace unlock.
*/
void mbed_mem_trace_unlock();

/**
* Trace a call to 'malloc'.
* @param res the result of running 'malloc'.
Expand Down