From 6141600b612aada94015182fccb16041ba75ba5f Mon Sep 17 00:00:00 2001 From: Guillaume Souchere Date: Thu, 3 Nov 2022 08:44:56 +0100 Subject: [PATCH 1/7] heap: add selective placement of function in IRAM This commit aims to place in the IRAM section only the functions that are relevent for performance instead of placing the entire content of multi_heap.c, mullti_heap_poisoning.c and tlsf.c in the IRAM. --- components/heap/heap_private.h | 2 +- components/heap/linker.lf | 46 +++++++++++++++++++++++++++++++--- components/heap/multi_heap.c | 19 +++----------- 3 files changed, 47 insertions(+), 20 deletions(-) diff --git a/components/heap/heap_private.h b/components/heap/heap_private.h index 5e172c034b..345ae032c8 100644 --- a/components/heap/heap_private.h +++ b/components/heap/heap_private.h @@ -51,7 +51,7 @@ extern SLIST_HEAD(registered_heap_ll, heap_t_) registered_heaps; bool heap_caps_match(const heap_t *heap, uint32_t caps); /* return all possible capabilities (across all priorities) for a given heap */ -inline static IRAM_ATTR uint32_t get_all_caps(const heap_t *heap) +inline static uint32_t get_all_caps(const heap_t *heap) { if (heap->heap == NULL) { return 0; diff --git a/components/heap/linker.lf b/components/heap/linker.lf index a2be01e113..1b29d7c24f 100644 --- a/components/heap/linker.lf +++ b/components/heap/linker.lf @@ -1,7 +1,47 @@ [mapping:heap] archive: libheap.a entries: - heap_tlsf (noflash) - multi_heap (noflash) + heap_tlsf:tlsf_ffs (noflash) + heap_tlsf:tlsf_fls (noflash) + heap_tlsf:tlsf_block_size (noflash) + heap_tlsf:tlsf_size (noflash) + heap_tlsf:tlsf_align_size (noflash) + heap_tlsf:tlsf_block_size_min (noflash) + heap_tlsf:tlsf_block_size_max (noflash) + heap_tlsf:tlsf_alloc_overhead (noflash) + heap_tlsf:tlsf_get_pool (noflash) + heap_tlsf:tlsf_malloc (noflash) + heap_tlsf:tlsf_memalign_offs (noflash) + heap_tlsf:tlsf_memalign (noflash) + heap_tlsf:tlsf_free (noflash) + heap_tlsf:tlsf_realloc (noflash) + + multi_heap:assert_valid_block (noflash) + multi_heap:multi_heap_get_block_address_impl (noflash) + multi_heap:multi_heap_get_allocated_size_impl (noflash) + multi_heap:multi_heap_set_lock (noflash) + multi_heap:multi_heap_get_first_block (noflash) + multi_heap:multi_heap_get_next_block (noflash) + multi_heap:multi_heap_is_free (noflash) + multi_heap:multi_heap_malloc_impl (noflash) + multi_heap:multi_heap_free_impl (noflash) + multi_heap:multi_heap_realloc_impl (noflash) + multi_heap:multi_heap_aligned_alloc_impl_offs (noflash) + multi_heap:multi_heap_aligned_alloc_impl (noflash) + if HEAP_POISONING_DISABLED = n: - multi_heap_poisoning (noflash) + multi_heap_poisoning:poison_allocated_region (noflash) + multi_heap_poisoning:verify_allocated_region (noflash) + multi_heap_poisoning:multi_heap_aligned_alloc (noflash) + multi_heap_poisoning:multi_heap_malloc (noflash) + multi_heap_poisoning:multi_heap_free (noflash) + multi_heap_poisoning:multi_heap_aligned_free (noflash) + multi_heap_poisoning:multi_heap_realloc (noflash) + multi_heap_poisoning:multi_heap_get_block_address (noflash) + multi_heap_poisoning:multi_heap_get_block_owner (noflash) + multi_heap_poisoning:multi_heap_get_allocated_size (noflash) + multi_heap_poisoning:multi_heap_internal_check_block_poisoning (noflash) + multi_heap_poisoning:multi_heap_internal_poison_fill_region (noflash) + + if HEAP_POISONING_COMPREHENSIVE = y: + multi_heap_poisoning:verify_fill_pattern (noflash) diff --git a/components/heap/multi_heap.c b/components/heap/multi_heap.c index 6b0d0a2c93..fd7183ac27 100644 --- a/components/heap/multi_heap.c +++ b/components/heap/multi_heap.c @@ -85,18 +85,6 @@ typedef struct multi_heap_info { tlsf_t heap_data; } heap_t; -/* Return true if this block is free. */ -static inline bool is_free(const block_header_t *block) -{ - return ((block->size & 0x01) != 0); -} - -/* Data size of the block (excludes this block's header) */ -static inline size_t block_data_size(const block_header_t *block) -{ - return (block->size & ~0x03); -} - /* Check a block is valid for this heap. Used to verify parameters. */ static void assert_valid_block(const heap_t *heap, const block_header_t *block) { @@ -110,8 +98,7 @@ static void assert_valid_block(const heap_t *heap, const block_header_t *block) void *multi_heap_get_block_address_impl(multi_heap_block_handle_t block) { - void *ptr = block_to_ptr(block); - return (ptr); + return block_to_ptr(block); } size_t multi_heap_get_allocated_size_impl(multi_heap_handle_t heap, void *p) @@ -175,7 +162,7 @@ multi_heap_block_handle_t multi_heap_get_next_block(multi_heap_handle_t heap, mu assert_valid_block(heap, block); block_header_t* next = block_next(block); - if(block_data_size(next) == 0) { + if(block_size(next) == 0) { //Last block: return NULL; } else { @@ -186,7 +173,7 @@ multi_heap_block_handle_t multi_heap_get_next_block(multi_heap_handle_t heap, mu bool multi_heap_is_free(multi_heap_block_handle_t block) { - return is_free(block); + return block_is_free(block); } void *multi_heap_malloc_impl(multi_heap_handle_t heap, size_t size) From 125609963e97b7c159d709942dd2aad8a75e9e1d Mon Sep 17 00:00:00 2001 From: Guillaume Souchere Date: Thu, 3 Nov 2022 12:38:36 +0100 Subject: [PATCH 2/7] heap: add documentation about the function placement in IRAM and its usage in ISR This commits adds a internal.md file in the heap directory to clarify the idea behind which functions is placed in IRAM or in flash. A section in mem_alloc.rst documentation is added to specify which functions from the heap component API can be used in interrupt handlers. --- components/heap/internals.md | 9 +++++++++ docs/en/api-reference/system/mem_alloc.rst | 23 +++++++++++++++++++++- 2 files changed, 31 insertions(+), 1 deletion(-) create mode 100644 components/heap/internals.md diff --git a/components/heap/internals.md b/components/heap/internals.md new file mode 100644 index 0000000000..d007f1d4c8 --- /dev/null +++ b/components/heap/internals.md @@ -0,0 +1,9 @@ +# Function placement in IRAM section + +The heap component is compiled and linked in a way that minimizes the utilization of the IRAM section of memory without impacting the performance of its core functionalities. For this reason, the heap component API provided through [esp_heap_caps.h](./include/esp_heap_caps.h) and [esp_heap_caps_init.h](./include/esp_heap_caps_init.h) can be sorted into two sets of functions. + +1. The performance related functions placed into the IRAM by using the `IRAM_ATTR` defined in [esp_attr.h](./../../components/esp_common/include/esp_attr.h) (e.g., `heap_caps_malloc`, `heap_caps_free`, `heap_caps_realloc`, etc.) + +2. The functions that does not require the best of performance placed in the flash (e.g., `heap_caps_print_heap_info`, `heap_caps_dump`, `heap_caps_dump_all`, etc.) + +With that in mind, all the functions defined in [multi_heap.c](./multi_heap.c), [multi_heap_poisoning.c](./multi_heap_poisoning.c) and [tlsf.c](./tlsf/tlsf.c) that are directly or indirectly called from one of the heap component API functions placed in IRAM have to also be placed in IRAM. Symmetrically, the functions directly or indirectly called from one of the heap component API functions placed in flash will also be placed in flash. \ No newline at end of file diff --git a/docs/en/api-reference/system/mem_alloc.rst b/docs/en/api-reference/system/mem_alloc.rst index aa84fd4027..f92fbbf6f6 100644 --- a/docs/en/api-reference/system/mem_alloc.rst +++ b/docs/en/api-reference/system/mem_alloc.rst @@ -126,7 +126,28 @@ Thread Safety Heap functions are thread safe, meaning they can be called from different tasks simultaneously without any limitations. -It is technically possible to call ``malloc``, ``free``, and related functions from interrupt handler (ISR) context. However this is not recommended, as heap function calls may delay other interrupts. It is strongly recommended to refactor applications so that any buffers used by an ISR are pre-allocated outside of the ISR. Support for calling heap functions from ISRs may be removed in a future update. +It is technically possible to call ``malloc``, ``free``, and related functions from interrupt handler (ISR) context (see :ref:`calling-heap-related-functions-from-isr`). However this is not recommended, as heap function calls may delay other interrupts. It is strongly recommended to refactor applications so that any buffers used by an ISR are pre-allocated outside of the ISR. Support for calling heap functions from ISRs may be removed in a future update. + +.. _calling-heap-related-functions-from-isr: + +Calling heap related functions from ISR +--------------------------------------- + +The following functions from the heap component can be called form interrupt handler (ISR): + +* :cpp:func:`heap_caps_malloc` +* :cpp:func:`heap_caps_malloc_default` +* :cpp:func:`heap_caps_realloc_default` +* :cpp:func:`heap_caps_malloc_prefer` +* :cpp:func:`heap_caps_realloc_prefer` +* :cpp:func:`heap_caps_calloc_prefer` +* :cpp:func:`heap_caps_free` +* :cpp:func:`heap_caps_realloc` +* :cpp:func:`heap_caps_calloc` +* :cpp:func:`heap_caps_aligned_alloc` +* :cpp:func:`heap_caps_aligned_free` + +Note however this practice is strongly discouraged. Heap Tracing & Debugging ------------------------ From 9ec87993c887788b601715c32365ea98b9be17e0 Mon Sep 17 00:00:00 2001 From: Guillaume Souchere Date: Fri, 4 Nov 2022 15:48:09 +0100 Subject: [PATCH 3/7] heap: add check for usage of flash content from iram this commits: - adds build-time test to check that no call to flash regions are done from IRAM functions - resolves problems related to IRAM function using content in flash memory - update heap_caps_alloc_failed to use a default function name in DRAM when necessary instead of creating a function name variable in DRAM for each call of heap_caps_alloc_failed. This allows to save some extra bytes in RAM. --- components/heap/CMakeLists.txt | 2 +- components/heap/heap_caps.c | 12 +++++++++--- components/heap/heap_private.h | 2 +- components/heap/test/CMakeLists.txt | 14 ++++++++++++++ tools/unit-test-app/sdkconfig.defaults | 1 + 5 files changed, 26 insertions(+), 5 deletions(-) diff --git a/components/heap/CMakeLists.txt b/components/heap/CMakeLists.txt index 40f75860a8..0110a19d7d 100644 --- a/components/heap/CMakeLists.txt +++ b/components/heap/CMakeLists.txt @@ -27,7 +27,7 @@ list(APPEND srcs "port/${target}/memory_layout.c") idf_component_register(SRCS "${srcs}" INCLUDE_DIRS include LDFRAGMENTS linker.lf - PRIV_REQUIRES soc) + PRIV_REQUIRES soc spi_flash) if(CONFIG_HEAP_TRACING) set(WRAP_FUNCTIONS diff --git a/components/heap/heap_caps.c b/components/heap/heap_caps.c index f3ac9460ee..bc29bb38db 100644 --- a/components/heap/heap_caps.c +++ b/components/heap/heap_caps.c @@ -22,7 +22,7 @@ #include "esp_log.h" #include "heap_private.h" #include "esp_system.h" - +#include "esp_private/cache_utils.h" /* Forward declaration for base function, put in IRAM. * These functions don't check for errors after trying to allocate memory. */ @@ -64,10 +64,16 @@ IRAM_ATTR static void *dram_alloc_to_iram_addr(void *addr, size_t len) } -static void heap_caps_alloc_failed(size_t requested_size, uint32_t caps, const char *function_name) +IRAM_ATTR static void heap_caps_alloc_failed(size_t requested_size, uint32_t caps, const char *function_name) { + static const DRAM_ATTR char *default_func_name = ""; if (alloc_failed_callback) { - alloc_failed_callback(requested_size, caps, function_name); + if (!spi_flash_cache_enabled() && !esp_ptr_internal(function_name)) { + alloc_failed_callback(requested_size, caps, default_func_name); + } + else { + alloc_failed_callback(requested_size, caps, function_name); + } } #ifdef CONFIG_HEAP_ABORT_WHEN_ALLOCATION_FAILS diff --git a/components/heap/heap_private.h b/components/heap/heap_private.h index 345ae032c8..bba9a86074 100644 --- a/components/heap/heap_private.h +++ b/components/heap/heap_private.h @@ -51,7 +51,7 @@ extern SLIST_HEAD(registered_heap_ll, heap_t_) registered_heaps; bool heap_caps_match(const heap_t *heap, uint32_t caps); /* return all possible capabilities (across all priorities) for a given heap */ -inline static uint32_t get_all_caps(const heap_t *heap) +inline static __attribute__((always_inline)) uint32_t get_all_caps(const heap_t *heap) { if (heap->heap == NULL) { return 0; diff --git a/components/heap/test/CMakeLists.txt b/components/heap/test/CMakeLists.txt index 6da69a0cbc..3496205c22 100644 --- a/components/heap/test/CMakeLists.txt +++ b/components/heap/test/CMakeLists.txt @@ -1,3 +1,17 @@ idf_component_register(SRC_DIRS "." PRIV_INCLUDE_DIRS "." PRIV_REQUIRES cmock test_utils heap) + +if(CONFIG_COMPILER_DUMP_RTL_FILES) + idf_build_get_property(elf_file_name EXECUTABLE GENERATOR_EXPRESSION) + add_custom_target(check_test_app_sections ALL + COMMAND ${PYTHON} $ENV{IDF_PATH}/tools/ci/check_callgraph.py + --rtl-dir ${CMAKE_BINARY_DIR}/esp-idf/heap/ + --elf-file ${CMAKE_BINARY_DIR}/${elf_file_name} + find-refs + --from-sections=.iram0.text + --to-sections=.flash.text,.flash.rodata + --exit-code + DEPENDS ${elf_file_name} + ) +endif() diff --git a/tools/unit-test-app/sdkconfig.defaults b/tools/unit-test-app/sdkconfig.defaults index bedb19eae8..8a2965119f 100644 --- a/tools/unit-test-app/sdkconfig.defaults +++ b/tools/unit-test-app/sdkconfig.defaults @@ -28,3 +28,4 @@ CONFIG_UNITY_ENABLE_BACKTRACE_ON_FAIL=y CONFIG_ESP_NETIF_TCPIP_ADAPTER_COMPATIBLE_LAYER=n CONFIG_FREERTOS_TIMER_TASK_STACK_DEPTH=3000 CONFIG_NVS_ASSERT_ERROR_CHECK=y +CONFIG_COMPILER_DUMP_RTL_FILES=y From e0c92b3e0499059b4bb882c6980ca12b2482bdf7 Mon Sep 17 00:00:00 2001 From: Guillaume Souchere Date: Wed, 9 Nov 2022 10:46:18 +0100 Subject: [PATCH 4/7] tools: update list of references to not include symbold used by __assert_func calls On xtensa architecture, the call to __assert_func uses a reference to __func__ that can sometimes be placed in flash. Since the __asert_func can be called from functions in IRAM the check_callgraph script can report an error when checking for invalid calls from IRAM to flash sections. However, the __asert_func prevents this scenario at runtime so the check_callgraph script reports a 'flas positive' situation. For this reasson, all references to __func__$x found prior to a call to __assert_func are droped in the parsing of the rtl files. --- components/heap/test/CMakeLists.txt | 1 + tools/ci/check_callgraph.py | 32 +++++++++++++++++++++++++---- 2 files changed, 29 insertions(+), 4 deletions(-) diff --git a/components/heap/test/CMakeLists.txt b/components/heap/test/CMakeLists.txt index 3496205c22..ec980b5b75 100644 --- a/components/heap/test/CMakeLists.txt +++ b/components/heap/test/CMakeLists.txt @@ -11,6 +11,7 @@ if(CONFIG_COMPILER_DUMP_RTL_FILES) find-refs --from-sections=.iram0.text --to-sections=.flash.text,.flash.rodata + --ignore-symbols=__func__/__assert_func,__func__/heap_caps_alloc_failed --exit-code DEPENDS ${elf_file_name} ) diff --git a/tools/ci/check_callgraph.py b/tools/ci/check_callgraph.py index 700d5e6bad..175718df7e 100755 --- a/tools/ci/check_callgraph.py +++ b/tools/ci/check_callgraph.py @@ -110,6 +110,11 @@ class Reference(object): ) +class IgnorePair(): + def __init__(self, pair: str) -> None: + self.symbol, self.function_call = pair.split('/') + + class ElfInfo(object): def __init__(self, elf_file): # type: (BinaryIO) -> None self.elf_file = elf_file @@ -174,7 +179,7 @@ class ElfInfo(object): return None -def load_rtl_file(rtl_filename, tu_filename, functions): # type: (str, str, List[RtlFunction]) -> None +def load_rtl_file(rtl_filename, tu_filename, functions, ignore_pairs): # type: (str, str, List[RtlFunction], List[IgnorePair]) -> None last_function = None # type: Optional[RtlFunction] for line in open(rtl_filename): # Find function definition @@ -190,6 +195,17 @@ def load_rtl_file(rtl_filename, tu_filename, functions): # type: (str, str, Lis match = re.match(CALL_REGEX, line) if match: target = match.group('target') + + # if target matches on of the IgnorePair function_call attributes, remove + # the last occurrence of the associated symbol from the last_function.refs list. + call_matching_pairs = [pair for pair in ignore_pairs if pair.function_call == target] + if call_matching_pairs and last_function and last_function.refs: + for pair in call_matching_pairs: + ignored_symbols = [ref for ref in last_function.refs if pair.symbol in ref] + if ignored_symbols: + last_ref = ignored_symbols.pop() + last_function.refs = [ref for ref in last_function.refs if last_ref != ref] + if target not in last_function.calls: last_function.calls.append(target) continue @@ -319,12 +335,12 @@ def match_rtl_funcs_to_symbols(rtl_functions, elfinfo): # type: (List[RtlFuncti return symbols, refs -def get_symbols_and_refs(rtl_list, elf_file): # type: (List[str], BinaryIO) -> Tuple[List[Symbol], List[Reference]] +def get_symbols_and_refs(rtl_list, elf_file, ignore_pairs): # type: (List[str], BinaryIO, List[IgnorePair]) -> Tuple[List[Symbol], List[Reference]] elfinfo = ElfInfo(elf_file) rtl_functions = [] # type: List[RtlFunction] for file_name in rtl_list: - load_rtl_file(file_name, file_name, rtl_functions) + load_rtl_file(file_name, file_name, rtl_functions, ignore_pairs) return match_rtl_funcs_to_symbols(rtl_functions, elfinfo) @@ -376,6 +392,10 @@ def main(): find_refs_parser.add_argument( '--to-sections', help='comma-separated list of target sections' ) + find_refs_parser.add_argument( + '--ignore-symbols', help='comma-separated list of symbol/function_name pairs. \ + This will force the parser to ignore the symbol preceding the call to function_name' + ) find_refs_parser.add_argument( '--exit-code', action='store_true', @@ -399,7 +419,11 @@ def main(): if not rtl_list: raise RuntimeError('No RTL files specified') - _, refs = get_symbols_and_refs(rtl_list, args.elf_file) + ignore_pairs = [] + for pair in args.ignore_symbols.split(',') if args.ignore_symbols else []: + ignore_pairs.append(IgnorePair(pair)) + + _, refs = get_symbols_and_refs(rtl_list, args.elf_file, ignore_pairs) if args.action == 'find-refs': from_sections = args.from_sections.split(',') if args.from_sections else [] From b78193700acbbd0b80bc0ce905e554f79dfdb375 Mon Sep 17 00:00:00 2001 From: Guillaume Souchere Date: Fri, 11 Nov 2022 08:44:07 +0100 Subject: [PATCH 5/7] feat: remove tlsf_fls and tlsf_ffs from linker as they are inlined. --- components/heap/linker.lf | 2 -- 1 file changed, 2 deletions(-) diff --git a/components/heap/linker.lf b/components/heap/linker.lf index 1b29d7c24f..6c1050b41a 100644 --- a/components/heap/linker.lf +++ b/components/heap/linker.lf @@ -1,8 +1,6 @@ [mapping:heap] archive: libheap.a entries: - heap_tlsf:tlsf_ffs (noflash) - heap_tlsf:tlsf_fls (noflash) heap_tlsf:tlsf_block_size (noflash) heap_tlsf:tlsf_size (noflash) heap_tlsf:tlsf_align_size (noflash) From 43ba878870b4c94231365b9575271f6b0ad4c9bb Mon Sep 17 00:00:00 2001 From: Guillaume Souchere Date: Mon, 14 Nov 2022 08:21:33 +0100 Subject: [PATCH 6/7] heap: fix linker issues and remove spi flash dependencies --- components/heap/CMakeLists.txt | 2 +- components/heap/heap_caps.c | 11 ++--------- components/heap/linker.lf | 8 +++++--- components/heap/multi_heap.c | 10 +++++----- components/heap/multi_heap_internal.h | 8 ++++++++ components/heap/multi_heap_poisoning.c | 14 ++++++++++---- 6 files changed, 31 insertions(+), 22 deletions(-) diff --git a/components/heap/CMakeLists.txt b/components/heap/CMakeLists.txt index 0110a19d7d..40f75860a8 100644 --- a/components/heap/CMakeLists.txt +++ b/components/heap/CMakeLists.txt @@ -27,7 +27,7 @@ list(APPEND srcs "port/${target}/memory_layout.c") idf_component_register(SRCS "${srcs}" INCLUDE_DIRS include LDFRAGMENTS linker.lf - PRIV_REQUIRES soc spi_flash) + PRIV_REQUIRES soc) if(CONFIG_HEAP_TRACING) set(WRAP_FUNCTIONS diff --git a/components/heap/heap_caps.c b/components/heap/heap_caps.c index bc29bb38db..7258c0af3c 100644 --- a/components/heap/heap_caps.c +++ b/components/heap/heap_caps.c @@ -22,7 +22,6 @@ #include "esp_log.h" #include "heap_private.h" #include "esp_system.h" -#include "esp_private/cache_utils.h" /* Forward declaration for base function, put in IRAM. * These functions don't check for errors after trying to allocate memory. */ @@ -64,16 +63,10 @@ IRAM_ATTR static void *dram_alloc_to_iram_addr(void *addr, size_t len) } -IRAM_ATTR static void heap_caps_alloc_failed(size_t requested_size, uint32_t caps, const char *function_name) +IRAM_ATTR NOINLINE_ATTR static void heap_caps_alloc_failed(size_t requested_size, uint32_t caps, const char *function_name) { - static const DRAM_ATTR char *default_func_name = ""; if (alloc_failed_callback) { - if (!spi_flash_cache_enabled() && !esp_ptr_internal(function_name)) { - alloc_failed_callback(requested_size, caps, default_func_name); - } - else { - alloc_failed_callback(requested_size, caps, function_name); - } + alloc_failed_callback(requested_size, caps, function_name); } #ifdef CONFIG_HEAP_ABORT_WHEN_ALLOCATION_FAILS diff --git a/components/heap/linker.lf b/components/heap/linker.lf index 6c1050b41a..33e1092d35 100644 --- a/components/heap/linker.lf +++ b/components/heap/linker.lf @@ -14,7 +14,6 @@ entries: heap_tlsf:tlsf_free (noflash) heap_tlsf:tlsf_realloc (noflash) - multi_heap:assert_valid_block (noflash) multi_heap:multi_heap_get_block_address_impl (noflash) multi_heap:multi_heap_get_allocated_size_impl (noflash) multi_heap:multi_heap_set_lock (noflash) @@ -26,6 +25,9 @@ entries: multi_heap:multi_heap_realloc_impl (noflash) multi_heap:multi_heap_aligned_alloc_impl_offs (noflash) multi_heap:multi_heap_aligned_alloc_impl (noflash) + multi_heap:multi_heap_internal_lock (noflash) + multi_heap:multi_heap_internal_unlock (noflash) + multi_heap:assert_valid_block (noflash) if HEAP_POISONING_DISABLED = n: multi_heap_poisoning:poison_allocated_region (noflash) @@ -41,5 +43,5 @@ entries: multi_heap_poisoning:multi_heap_internal_check_block_poisoning (noflash) multi_heap_poisoning:multi_heap_internal_poison_fill_region (noflash) - if HEAP_POISONING_COMPREHENSIVE = y: - multi_heap_poisoning:verify_fill_pattern (noflash) + if HEAP_POISONING_COMPREHENSIVE = y: + multi_heap_poisoning:verify_fill_pattern (noflash) diff --git a/components/heap/multi_heap.c b/components/heap/multi_heap.c index fd7183ac27..e5c8647f17 100644 --- a/components/heap/multi_heap.c +++ b/components/heap/multi_heap.c @@ -86,7 +86,7 @@ typedef struct multi_heap_info { } heap_t; /* Check a block is valid for this heap. Used to verify parameters. */ -static void assert_valid_block(const heap_t *heap, const block_header_t *block) +__attribute__((noinline)) NOCLONE_ATTR static void assert_valid_block(const heap_t *heap, const block_header_t *block) { pool_t pool = tlsf_get_pool(heap->heap_data); void *ptr = block_to_ptr(block); @@ -137,12 +137,12 @@ void multi_heap_set_lock(multi_heap_handle_t heap, void *lock) heap->lock = lock; } -void inline multi_heap_internal_lock(multi_heap_handle_t heap) +void multi_heap_internal_lock(multi_heap_handle_t heap) { MULTI_HEAP_LOCK(heap->lock); } -void inline multi_heap_internal_unlock(multi_heap_handle_t heap) +void multi_heap_internal_unlock(multi_heap_handle_t heap) { MULTI_HEAP_UNLOCK(heap->lock); } @@ -333,7 +333,7 @@ bool multi_heap_check(multi_heap_handle_t heap, bool print_errors) return valid; } -static void multi_heap_dump_tlsf(void* ptr, size_t size, int used, void* user) +__attribute__((noinline)) static void multi_heap_dump_tlsf(void* ptr, size_t size, int used, void* user) { (void)user; MULTI_HEAP_STDERR_PRINTF("Block %p data, size: %d bytes, Free: %s \n", @@ -370,7 +370,7 @@ size_t multi_heap_minimum_free_size_impl(multi_heap_handle_t heap) return heap->minimum_free_bytes; } -static void multi_heap_get_info_tlsf(void* ptr, size_t size, int used, void* user) +__attribute__((noinline)) static void multi_heap_get_info_tlsf(void* ptr, size_t size, int used, void* user) { multi_heap_info_t *info = user; diff --git a/components/heap/multi_heap_internal.h b/components/heap/multi_heap_internal.h index 5be4dee608..131c60e27d 100644 --- a/components/heap/multi_heap_internal.h +++ b/components/heap/multi_heap_internal.h @@ -13,6 +13,14 @@ // limitations under the License. #pragma once +/* Define a noclone attribute when compiled with GCC as certain functions + * in the heap component should not be cloned by the compiler */ +#if defined __has_attribute && __has_attribute(noclone) +#define NOCLONE_ATTR __attribute((noclone)) +#else +#define NOCLONE_ATTR +#endif + /* Opaque handle to a heap block */ typedef const struct block_header_t *multi_heap_block_handle_t; diff --git a/components/heap/multi_heap_poisoning.c b/components/heap/multi_heap_poisoning.c index ca27f3f290..4b2d9bae2a 100644 --- a/components/heap/multi_heap_poisoning.c +++ b/components/heap/multi_heap_poisoning.c @@ -65,7 +65,7 @@ typedef struct { Returns the pointer to the actual usable data buffer (ie after 'head') */ -static uint8_t *poison_allocated_region(poison_head_t *head, size_t alloc_size) +__attribute__((noinline)) static uint8_t *poison_allocated_region(poison_head_t *head, size_t alloc_size) { uint8_t *data = (uint8_t *)(&head[1]); /* start of data ie 'real' allocated buffer */ poison_tail_t *tail = (poison_tail_t *)(data + alloc_size); @@ -89,7 +89,7 @@ static uint8_t *poison_allocated_region(poison_head_t *head, size_t alloc_size) Returns a pointer to the poison header structure, or NULL if the poison structures are corrupt. */ -static poison_head_t *verify_allocated_region(void *data, bool print_errors) +__attribute__((noinline)) static poison_head_t *verify_allocated_region(void *data, bool print_errors) { poison_head_t *head = (poison_head_t *)((intptr_t)data - sizeof(poison_head_t)); poison_tail_t *tail = (poison_tail_t *)((intptr_t)data + head->alloc_size); @@ -131,8 +131,12 @@ static poison_head_t *verify_allocated_region(void *data, bool print_errors) if swap_pattern is true, swap patterns in the buffer (ie replace MALLOC_FILL_PATTERN with FREE_FILL_PATTERN, and vice versa.) Returns true if verification checks out. + + This function has the attribute noclone to prevent the compiler to create a clone on flash where expect_free is removed (as this + function is called only with expect_free == true throughout the component). */ -static bool verify_fill_pattern(void *data, size_t size, bool print_errors, bool expect_free, bool swap_pattern) +__attribute__((noinline)) NOCLONE_ATTR +static bool verify_fill_pattern(void *data, size_t size, const bool print_errors, const bool expect_free, bool swap_pattern) { const uint32_t FREE_FILL_WORD = (FREE_FILL_PATTERN << 24) | (FREE_FILL_PATTERN << 16) | (FREE_FILL_PATTERN << 8) | FREE_FILL_PATTERN; const uint32_t MALLOC_FILL_WORD = (MALLOC_FILL_PATTERN << 24) | (MALLOC_FILL_PATTERN << 16) | (MALLOC_FILL_PATTERN << 8) | MALLOC_FILL_PATTERN; @@ -242,7 +246,9 @@ void *multi_heap_malloc(multi_heap_handle_t heap, size_t size) return data; } -void multi_heap_free(multi_heap_handle_t heap, void *p) +/* This function has the noclone attribute to prevent the compiler to optimize out the + * check for p == NULL and create a clone function placed in flash. */ +NOCLONE_ATTR void multi_heap_free(multi_heap_handle_t heap, void *p) { if (p == NULL) { return; From dd249a9ecdace7e9496f14d25f5e1db23e8a375f Mon Sep 17 00:00:00 2001 From: Guillaume Souchere Date: Tue, 6 Dec 2022 09:23:52 +0100 Subject: [PATCH 7/7] esp_system: fix placement of __stack_chk_fail from flash to RAM When stack check is enabled, certain functions (sometimes placed in RAM) are being decorated with stack guards and a call to __stask_chk_fail() in case ofr stack corruption. For this reason, __stack_chk_fail() must be placed in RAM too. Add stack check config in heap tests on all targets to find eventual flash to RAM calls due to stack checks when running callgraph_check.py --- components/esp_system/stack_check.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/components/esp_system/stack_check.c b/components/esp_system/stack_check.c index 50822f7baa..5b6f41fa27 100644 --- a/components/esp_system/stack_check.c +++ b/components/esp_system/stack_check.c @@ -30,10 +30,9 @@ __esp_stack_guard_setup (void) __stack_chk_guard = (void *)esp_random(); } -void __stack_chk_fail (void) +IRAM_ATTR void __stack_chk_fail (void) { - esp_rom_printf("\r\nStack smashing protect failure!\r\n\r\n"); - abort(); + esp_system_abort(DRAM_STR("Stack smashing protect failure!")); } #endif