From: Simon Glass <simon.glass@canonical.com> Replace the dynamic allocation in os_backtrace_symbols() with a static buffer embedded in struct backtrace_ctx. This avoids malloc recursion when backtrace is called from within dlmalloc (e.g., for the upcoming mcheck caller-tracking). The API gets a complete rework as part of this: - Combine addrs[] and syms[] arrays into struct backtrace_frame with addr and sym fields - Store the strings in a unified buffer, with pointers from an array - Change os_backtrace_symbols() to take ctx pointer and fill sym_buf - Remove os_backtrace_symbols_free() as nothing needs freeing - Rename BACKTRACE_MAX to BACKTRACE_MAX_FRAMES Co-developed-by: Claude <noreply@anthropic.com> Signed-off-by: Simon Glass <simon.glass@canonical.com> --- arch/sandbox/cpu/backtrace.c | 53 +++++++++++++++++------------------- arch/sandbox/lib/backtrace.c | 47 ++++++-------------------------- include/backtrace.h | 41 ++++++++++++++++++++++------ include/os.h | 22 ++++----------- lib/backtrace.c | 13 +++++---- test/lib/backtrace.c | 13 +++++---- 6 files changed, 86 insertions(+), 103 deletions(-) diff --git a/arch/sandbox/cpu/backtrace.c b/arch/sandbox/cpu/backtrace.c index 1f5a14ed541..2d0a0113cc0 100644 --- a/arch/sandbox/cpu/backtrace.c +++ b/arch/sandbox/cpu/backtrace.c @@ -16,6 +16,8 @@ #include <string.h> #include <os.h> +/* For BACKTRACE_MAX_FRAMES - include U-Boot's header after system headers */ +#include "../../../include/backtrace.h" /* libbacktrace state - created once and cached */ static struct backtrace_state *bt_state; @@ -77,46 +79,41 @@ static int bt_full_callback(void *data, uintptr_t pc, const char *fname, return 0; /* continue to get innermost frame for inlined functions */ } -char **os_backtrace_symbols(void *const *buffer, uint count) +void os_backtrace_symbols(struct backtrace_ctx *ctx) { + char *end = ctx->sym_buf + BACKTRACE_SYM_BUFSZ; struct backtrace_state *state; - char *str_storage; - char **strings; - uint i; + char *p = ctx->sym_buf; + int remaining, i; state = get_bt_state(); - /* Allocate array of string pointers plus space for strings */ - strings = malloc(count * sizeof(char *) + count * 256); - if (!strings) - return NULL; + for (i = 0; i < ctx->count; i++) { + struct backtrace_frame *frame = &ctx->frame[i]; + struct bt_sym_ctx sym_ctx; - /* String storage starts after the pointer array */ - str_storage = (char *)(strings + count); - - for (i = 0; i < count; i++) { - struct bt_sym_ctx ctx; + remaining = end - p; + if (remaining <= 1) { + /* No more space, leave remaining syms as NULL */ + frame->sym = NULL; + continue; + } - strings[i] = str_storage + i * 256; - ctx.buf = strings[i]; - ctx.size = 256; - ctx.found = 0; + frame->sym = p; + sym_ctx.buf = p; + sym_ctx.size = remaining; + sym_ctx.found = 0; if (state) { - backtrace_pcinfo(state, (uintptr_t)buffer[i], + backtrace_pcinfo(state, (uintptr_t)frame->addr, bt_full_callback, bt_error_callback, - &ctx); + &sym_ctx); } /* Fall back to address if no symbol found */ - if (!ctx.found) - snprintf(strings[i], 256, "%p", buffer[i]); - } + if (!sym_ctx.found) + snprintf(p, remaining, "%p", frame->addr); - return strings; -} - -void os_backtrace_symbols_free(char **strings) -{ - free(strings); + p += strlen(p) + 1; + } } diff --git a/arch/sandbox/lib/backtrace.c b/arch/sandbox/lib/backtrace.c index 073eb945622..62a1c9ee028 100644 --- a/arch/sandbox/lib/backtrace.c +++ b/arch/sandbox/lib/backtrace.c @@ -13,54 +13,23 @@ int backtrace_init(struct backtrace_ctx *ctx, uint skip) { + void *addrs[BACKTRACE_MAX_FRAMES]; uint i; - for (i = 0; i < BACKTRACE_MAX; i++) - ctx->syms[i] = NULL; /* +1 to skip this function */ - ctx->count = os_backtrace(ctx->addrs, BACKTRACE_MAX, skip + 1); + ctx->count = os_backtrace(addrs, BACKTRACE_MAX_FRAMES, skip + 1); + + for (i = 0; i < ctx->count; i++) { + ctx->frame[i].addr = addrs[i]; + ctx->frame[i].sym = NULL; + } return ctx->count; } int backtrace_get_syms(struct backtrace_ctx *ctx, char *buf, int size) { - char **raw_syms; - size_t total_len; - char *p; - uint i; - - raw_syms = os_backtrace_symbols(ctx->addrs, ctx->count); - if (!raw_syms) - return -ENOMEM; - - /* Calculate total buffer size needed */ - total_len = 0; - for (i = 0; i < ctx->count; i++) { - if (raw_syms[i]) - total_len += strlen(raw_syms[i]) + 1; - else - total_len += 1; /* empty string */ - } - - if ((size_t)size < total_len) { - os_backtrace_symbols_free(raw_syms); - return -ENOSPC; - } - - /* Copy strings into buffer */ - p = buf; - for (i = 0; i < ctx->count; i++) { - ctx->syms[i] = p; - if (raw_syms[i]) { - strcpy(p, raw_syms[i]); - p += strlen(raw_syms[i]) + 1; - } else { - *p++ = '\0'; - } - } - - os_backtrace_symbols_free(raw_syms); + os_backtrace_symbols(ctx); return 0; } diff --git a/include/backtrace.h b/include/backtrace.h index eece61e4d9a..7bb2ba68bec 100644 --- a/include/backtrace.h +++ b/include/backtrace.h @@ -9,21 +9,46 @@ #ifndef __BACKTRACE_H #define __BACKTRACE_H -#define BACKTRACE_MAX 100 -#define BACKTRACE_SYM_SIZE 128 -#define BACKTRACE_BUFSZ (BACKTRACE_MAX * BACKTRACE_SYM_SIZE) +/* Maximum number of stack frames that can be collected */ +#define BACKTRACE_MAX_FRAMES 100 + +/* Size of buffer for all symbol strings combined */ +#define BACKTRACE_SYM_BUFSZ (4 * 1024) + +/** + * struct backtrace_frame - a single stack frame in a backtrace + * + * @addr: return address for this frame + * @sym: pointer to symbol string in backtrace_ctx->sym_buf, or NULL if not + * yet resolved or if sym_buf ran out of space + */ +struct backtrace_frame { + void *addr; + char *sym; +}; /** * struct backtrace_ctx - context for backtrace operations * - * @addrs: array of return addresses - * @syms: array of symbol strings (NULL until backtrace_get_syms() called) - * @count: number of entries in addrs/syms arrays + * This structure holds all state for collecting and symbolising a backtrace. + * It should be declared static to avoid consuming stack space (~5KB). + * + * Lifecycle: + * 1. Call backtrace_init() - fills @frame[].addr with return addresses and + * sets @count. The @frame[].sym pointers are initialised to NULL. + * 2. Call backtrace_get_syms() - resolves addresses to symbol strings, + * writing them into @sym_buf and setting @frame[].sym pointers. + * 3. Access @frame[0..count-1] to read addresses and symbol strings. + * 4. Call backtrace_uninit() to release resources (currently a no-op). + * + * @frame: array of stack frames + * @count: number of valid entries in @frame + * @sym_buf: buffer holding NUL-terminated symbol strings packed consecutively */ struct backtrace_ctx { - void *addrs[BACKTRACE_MAX]; - char *syms[BACKTRACE_MAX]; + struct backtrace_frame frame[BACKTRACE_MAX_FRAMES]; unsigned int count; + char sym_buf[BACKTRACE_SYM_BUFSZ]; }; /** diff --git a/include/os.h b/include/os.h index ab4710fc265..3ea88230af3 100644 --- a/include/os.h +++ b/include/os.h @@ -588,27 +588,17 @@ void os_signal_action(int sig, unsigned long pc); */ uint os_backtrace(void **buffer, uint size, uint skip); -/** - * os_backtrace_symbols() - convert addresses to symbol strings - * - * Convert backtrace addresses to human-readable symbol strings. The returned - * array and strings are allocated with malloc() and must be freed with - * os_backtrace_symbols_free(). - * - * @buffer: array of addresses from os_backtrace() - * @count: number of addresses in buffer - * Return: array of symbol strings, or NULL on error - */ -char **os_backtrace_symbols(void *const *buffer, uint count); +struct backtrace_ctx; /** - * os_backtrace_symbols_free() - free symbol strings + * os_backtrace_symbols() - convert addresses to symbol strings * - * Free the array returned by os_backtrace_symbols(). + * Convert backtrace addresses to human-readable symbol strings. The symbol + * strings are written into ctx->sym_buf and ctx->syms pointers are set up. * - * @strings: array to free (may be NULL) + * @ctx: backtrace context with addrs and count already filled in */ -void os_backtrace_symbols_free(char **strings); +void os_backtrace_symbols(struct backtrace_ctx *ctx); /** * os_get_time_offset() - get time offset diff --git a/lib/backtrace.c b/lib/backtrace.c index 715d7d1d05e..b01a08af8ba 100644 --- a/lib/backtrace.c +++ b/lib/backtrace.c @@ -26,8 +26,7 @@ static void print_sym(const char *sym) int backtrace_show(void) { - char buf[BACKTRACE_BUFSZ]; - struct backtrace_ctx ctx; + static struct backtrace_ctx ctx; uint i; int ret; @@ -35,7 +34,7 @@ int backtrace_show(void) if (ret < 0) return ret; - ret = backtrace_get_syms(&ctx, buf, sizeof(buf)); + ret = backtrace_get_syms(&ctx, NULL, 0); if (ret) { backtrace_uninit(&ctx); return ret; @@ -43,10 +42,12 @@ int backtrace_show(void) printf("backtrace: %d addresses\n", ctx.count); for (i = 0; i < ctx.count; i++) { - if (ctx.syms[i]) - print_sym(ctx.syms[i]); + const struct backtrace_frame *frame = &ctx.frame[i]; + + if (frame->sym) + print_sym(frame->sym); else - printf(" %p\n", ctx.addrs[i]); + printf(" %p\n", frame->addr); } backtrace_uninit(&ctx); diff --git a/test/lib/backtrace.c b/test/lib/backtrace.c index d9c36bbd495..11f0d43ca7e 100644 --- a/test/lib/backtrace.c +++ b/test/lib/backtrace.c @@ -15,24 +15,25 @@ /* Test backtrace_init() and backtrace_get_syms() */ static int lib_test_backtrace(struct unit_test_state *uts) { - char buf[BACKTRACE_BUFSZ]; - struct backtrace_ctx ctx; + static struct backtrace_ctx ctx; bool found_self = false; bool found_ut_run_list = false; uint i; ut_assert(backtrace_init(&ctx, 0) > 2); - ut_assertok(backtrace_get_syms(&ctx, buf, sizeof(buf))); + ut_assertok(backtrace_get_syms(&ctx, NULL, 0)); /* * Check for known functions in the call stack. With libbacktrace * we can find static functions too, so check for this test function. */ for (i = 0; i < ctx.count; i++) { - if (ctx.syms[i]) { - if (strstr(ctx.syms[i], "lib_test_backtrace")) + const struct backtrace_frame *frame = &ctx.frame[i]; + + if (frame->sym) { + if (strstr(frame->sym, "lib_test_backtrace")) found_self = true; - if (strstr(ctx.syms[i], "ut_run_list")) + if (strstr(frame->sym, "ut_run_list")) found_ut_run_list = true; } } -- 2.43.0