Heap: Improve debug caller address reporting #8720
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,45 +33,55 @@ | |
| #include "gdb_hooks.h" | ||
| #include "StackThunk.h" | ||
| #include "coredecls.h" | ||
| #include "umm_malloc/umm_malloc.h" | ||
|
|
||
| extern "C" { | ||
|
|
||
| // using numbers different from "REASON_" in user_interface.h (=0..6) | ||
| enum rst_reason_sw | ||
| { | ||
| REASON_USER_STACK_OVERFLOW = 252, | ||
| REASON_USER_STACK_SMASH = 253, | ||
| REASON_USER_SWEXCEPTION_RST = 254 | ||
| }; | ||
|
|
||
| // Confirmed on 12/17/22: s_pm is in the .bss section and is in the | ||
| // _bss_start/end range to be zeroed by the SDK this happens after the SDK first | ||
| // calls to Cache_Read_Enable_New. | ||
| static struct PostmortemInfo { | ||
| int user_reset_reason = REASON_DEFAULT_RST; | ||
|
|
||
| // These will be pointers to PROGMEM const strings | ||
| const char* panic_file = 0; | ||
| int panic_line = 0; | ||
| const char* panic_func = 0; | ||
| const char* panic_what = 0; | ||
|
|
||
| // Our wiring for abort() and C++ exceptions | ||
| bool abort_called = false; | ||
| const char* unhandled_exception = NULL; | ||
|
|
||
| // Common way to notify about where the stack smashing happened | ||
| // (but, **only** if caller uses our handler function) | ||
| uint32_t stack_chk_addr = 0; | ||
| } s_pm; | ||
|
|
||
| // From UMM, the last caller of a malloc/realloc/calloc which failed: | ||
| extern struct umm_last_fail_alloc { | ||
| const void *addr; | ||
| size_t size; | ||
| #if defined(DEBUG_ESP_OOM) | ||
| const char *file; | ||
| int line; | ||
| #endif | ||
| } _umm_last_fail_alloc; | ||
|
|
||
|
|
||
| void abort() __attribute__((noreturn)); | ||
| static void uart_write_char_d(char c); | ||
| static void uart0_write_char_d(char c); | ||
| static void uart1_write_char_d(char c); | ||
| static void print_stack(uint32_t start, uint32_t end); | ||
|
|
||
| static void raise_exception() __attribute__((noreturn)); | ||
|
|
||
|
|
@@ -139,7 +149,7 @@ asm( | |
| static void postmortem_report(uint32_t sp_dump) { | ||
| struct rst_info rst_info; | ||
| memset(&rst_info, 0, sizeof(rst_info)); | ||
| if (s_pm.user_reset_reason == REASON_DEFAULT_RST) | ||
| { | ||
| system_rtc_mem_read(0, &rst_info, sizeof(rst_info)); | ||
| if (rst_info.reason != REASON_SOFT_WDT_RST && | ||
|
|
@@ -150,26 +160,26 @@ static void postmortem_report(uint32_t sp_dump) { | |
| } | ||
| } | ||
| else | ||
| rst_info.reason = s_pm.user_reset_reason; | ||
|
|
||
| ets_install_putc1(&uart_write_char_d); | ||
|
|
||
| cut_here(); | ||
|
|
||
| if (s_pm.panic_line) { | ||
| ets_printf_P(PSTR("\nPanic %S:%d %S"), s_pm.panic_file, s_pm.panic_line, s_pm.panic_func); | ||
| if (s_pm.panic_what) { | ||
| ets_printf_P(PSTR(": Assertion '%S' failed."), s_pm.panic_what); | ||
| } | ||
| ets_putc('\n'); | ||
| } | ||
| else if (s_pm.panic_file) { | ||
| ets_printf_P(PSTR("\nPanic %S\n"), s_pm.panic_file); | ||
| } | ||
| else if (s_pm.unhandled_exception) { | ||
| ets_printf_P(PSTR("\nUnhandled C++ exception: %S\n"), s_pm.unhandled_exception); | ||
| } | ||
| else if (s_pm.abort_called) { | ||
| ets_printf_P(PSTR("\nAbort called\n")); | ||
| } | ||
| else if (rst_info.reason == REASON_EXCEPTION_RST) { | ||
|
|
@@ -199,7 +209,7 @@ static void postmortem_report(uint32_t sp_dump) { | |
| rst_info.exccause, /* Address executing at time of Soft WDT level-1 interrupt */ rst_info.epc1, 0, 0, 0, 0); | ||
| } | ||
| else if (rst_info.reason == REASON_USER_STACK_SMASH) { | ||
| ets_printf_P(PSTR("\nStack smashing detected at 0x%08x\n"), s_pm.stack_chk_addr); | ||
| } | ||
| else if (rst_info.reason == REASON_USER_STACK_OVERFLOW) { | ||
| ets_printf_P(PSTR("\nStack overflow detected\n")); | ||
|
|
@@ -210,7 +220,7 @@ static void postmortem_report(uint32_t sp_dump) { | |
|
|
||
| uint32_t cont_stack_start; | ||
| if (rst_info.reason == REASON_USER_STACK_SMASH) { | ||
| cont_stack_start = s_pm.stack_chk_addr; | ||
| } else { | ||
| cont_stack_start = (uint32_t) (&g_pcont->stack[0]); | ||
| } | ||
|
|
@@ -229,7 +239,7 @@ static void postmortem_report(uint32_t sp_dump) { | |
| // 16 ?unnamed? - index into a table, pull out pointer, and call if non-zero | ||
| // appears near near wDev_ProcessFiq | ||
| // 32 pp_soft_wdt_feed_local - gather the specifics and call __wrap_system_restart_local | ||
| offset = 32 + 16 + 48 + 256; | ||
| } | ||
| else if (rst_info.reason == REASON_EXCEPTION_RST) { | ||
| // Stack Tally | ||
|
|
@@ -280,22 +290,29 @@ static void postmortem_report(uint32_t sp_dump) { | |
| ets_printf_P(PSTR("<<<stack<<<\n")); | ||
|
|
||
| // Use cap-X formatting to ensure the standard EspExceptionDecoder doesn't match the address | ||
| if (_umm_last_fail_alloc.addr) { | ||
| #if defined(DEBUG_ESP_OOM) | ||
| ets_printf_P(PSTR("\nlast failed alloc call: 0x%08X(%d), File: %S:%d\n"), | ||
|
There was a problem hiding this comment.
There was a problem hiding this comment. No special case in newlib here? Implementation thinks of both 's' and 'S' as exactly same printf(3) says it is '%ls', which is wide-char ('wchar_t') and non-applicable here
There was a problem hiding this comment. I have always been confused about this duality. Some print functions give an error (or is it a warning?) about %S and PROGMEM strings while other functions don't, like Serial.printf_P.
The comment for the code you pointed out reads to me that Arduino intends to have %S for PROGMEM strings. case 'S': // TODO: Verify cap-S under Arduino is "PROGMEM char*", not wchar_tThere was a problem hiding this comment. 6280e98 / #5376
as-is both '%s' and even '%.*s' work seamlessly, no need for extra care |
||
| (uint32_t)_umm_last_fail_alloc.addr, | ||
| _umm_last_fail_alloc.size, | ||
| (_umm_last_fail_alloc.file) ? _umm_last_fail_alloc.file : "??", | ||
| _umm_last_fail_alloc.line); | ||
| #else | ||
| ets_printf_P(PSTR("\nlast failed alloc call: %08X(%d)\n"), (uint32_t)_umm_last_fail_alloc.addr, _umm_last_fail_alloc.size); | ||
| #endif | ||
| } | ||
|
|
||
| cut_here(); | ||
|
|
||
| if (s_pm.unhandled_exception && _umm_last_fail_alloc.addr) { | ||
| // now outside from the "cut-here" zone, print correctly the `new` caller address, | ||
| // idf-monitor.py will be able to decode this one and show exact location in sources | ||
| ets_printf_P(PSTR("\nlast failed alloc caller: 0x%08x\n"), (uint32_t)_umm_last_fail_alloc.addr); | ||
| } | ||
|
|
||
| size_t oom_count = umm_get_oom_count(); | ||
| if (oom_count) { | ||
| ets_printf_P(PSTR("\nOOM Count: %u\n"), oom_count); | ||
| } | ||
|
|
||
| custom_crash_callback( &rst_info, sp_dump + offset, stack_end ); | ||
|
|
@@ -353,7 +370,7 @@ static void raise_exception() { | |
| if (gdb_present()) | ||
| __asm__ __volatile__ ("syscall"); // triggers GDB when enabled | ||
|
|
||
| s_pm.user_reset_reason = REASON_USER_SWEXCEPTION_RST; | ||
| ets_printf_P(PSTR("\nUser exception (panic/abort/assert)")); | ||
| uint32_t sp; | ||
| __asm__ __volatile__ ("mov %0, a1\n\t" : "=r"(sp) :: "memory"); | ||
|
|
@@ -362,37 +379,37 @@ static void raise_exception() { | |
| } | ||
|
|
||
| void abort() { | ||
| s_pm.abort_called = true; | ||
| raise_exception(); | ||
| } | ||
|
|
||
| void __unhandled_exception(const char *str) { | ||
| s_pm.unhandled_exception = str; | ||
| raise_exception(); | ||
| } | ||
|
|
||
| void __assert_func(const char *file, int line, const char *func, const char *what) { | ||
| s_pm.panic_file = file; | ||
| s_pm.panic_line = line; | ||
| s_pm.panic_func = func; | ||
| s_pm.panic_what = what; | ||
| gdb_do_break(); /* if GDB is not present, this is a no-op */ | ||
| raise_exception(); | ||
| } | ||
|
|
||
| void __panic_func(const char* file, int line, const char* func) { | ||
| s_pm.panic_file = file; | ||
| s_pm.panic_line = line; | ||
| s_pm.panic_func = func; | ||
| s_pm.panic_what = 0; | ||
| gdb_do_break(); /* if GDB is not present, this is a no-op */ | ||
| raise_exception(); | ||
| } | ||
|
|
||
| uintptr_t __stack_chk_guard = 0x08675309 ^ RANDOM_REG32; | ||
| void __stack_chk_fail(void) { | ||
| s_pm.user_reset_reason = REASON_USER_STACK_SMASH; | ||
| s_pm.stack_chk_addr = (uint32_t)__builtin_return_address(0); | ||
|
|
||
| if (gdb_present()) | ||
| __asm__ __volatile__ ("syscall"); // triggers GDB when enabled | ||
|
|
@@ -405,8 +422,8 @@ void __stack_chk_fail(void) { | |
| } | ||
|
|
||
| void __stack_overflow(cont_t* cont, uint32_t* sp) { | ||
| s_pm.user_reset_reason = REASON_USER_STACK_OVERFLOW; | ||
| s_pm.stack_chk_addr = (uint32_t)&cont->stack[0]; | ||
|
|
||
| if (gdb_present()) | ||
| __asm__ __volatile__ ("syscall"); // triggers GDB when enabled | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't 'new_handler' be independent of debugging?
since the idea is to allow plain new to sometimes avoid throwing (regardless of whether it builds w/ -fexceptions or without). exception-less builds obviously just call abort for would-be exception
ref new_op.cc and new_opnt.cc, nothrow variant just wraps the other in try catch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It has been a while since I did this, I think the increased build size for non-debug builds was a consideration.
It would add 136 bytes of IROM to the non-debug build so that it always builds with our revised
new_handler.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will have to recheck too, then. Debug=enabled without exceptions are not handled though?
Plus there are more missing overloads from -std=c++17
https://en.cppreference.com/w/cpp/memory/new/operator_new
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current master does not support "new" align operations. Compiler reports:
/workdir/repo/gcc-gnu/libstdc++-v3/libsupc++/new_opa.cc:86: undefined reference tomemalign'`No hits on a quick issue check for
memalign.The library umm_malloc needs an enhancement to support an allocate-aligned option. Also, things will get complicated when incorporating the poison check feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've worked through adding an alignment option to the
umm_malloclibrary. In the process, I have addressed an unreported alignment issue.umm_mallocallocated data addresses always landed on an 8-byte aligned - 4-byte.Now I need to finish working through all the build options in
abi.cppandheap.cpp.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(edit: comment thread from above)
Can this be a config error? GCC / newlib / hal / etc.
Reading cppreference and some of the internal GCC stuff from '-faligned-new=X', 'Fundamental alignment' and max_align_t seem to come from 'long double' and 'long long' which are afaik 4-byte ones.
Currently, it does indeed show 8-byte
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...which is sort-of expected, as run-of-the-mill xtensa isa pdf does specify alignment req for long double and long long as 8. compiler here works according to spec, stack and heap alignment would stay consistent vs. current approach
no mention / explicit alignment things mentioned in the machine config, besides the
BIGGEST_ALIGNMENTof 128 (bits)https://github.com/gcc-mirror/gcc/tree/master/gcc/config/xtensa
/ALIGN/esp32 variants adhere to a similar standard. however, esp8266 rtos does use 4-byte heap alignment
(which I would assume works just fine for the most part)