From f4ab2beaa844f3890d73f4878ac1959e4f27a5bd Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Wed, 2 Sep 2020 14:04:04 +0200 Subject: [PATCH 1/3] freertos: don't clobber a4 while spilling register windows Commit 891eb3b0 was fixing an issue with PS and EPC1 not being preserved after the window spill procedure. It did so by saving PS in a2 and EPC1 in a4. However the a4 register may be a live register of another window in the call stack, and if it is overwritten and then spilled to the stack, then the corresponding register value will end up being corrupted. In practice the problem would show up as an IllegalInstruction exception, when trying to return from a function when a0 value was 0x40020. Fix by using a0 register instead of a4 as scratch. Also fix a comment about xthal_save_extra_nw, as this function in fact doesn't clobber a4 or a5 because XCHAL_NCP_NUM_ATMPS is defined as 1. Closes https://github.com/espressif/esp-idf/issues/5758 --- .../freertos/test/test_context_save_clobber.S | 55 +++++++++++++++++++ .../freertos/test/test_context_save_clobber.c | 43 +++++++++++++++ components/freertos/xtensa/xtensa_context.S | 20 ++++--- 3 files changed, 111 insertions(+), 7 deletions(-) create mode 100644 components/freertos/test/test_context_save_clobber.S create mode 100644 components/freertos/test/test_context_save_clobber.c diff --git a/components/freertos/test/test_context_save_clobber.S b/components/freertos/test/test_context_save_clobber.S new file mode 100644 index 0000000000..a9af1baa0a --- /dev/null +++ b/components/freertos/test/test_context_save_clobber.S @@ -0,0 +1,55 @@ +/* Helper function for the test case in test_context_save_clobber.c */ + +#if defined(__XTENSA__) +#include "xtensa/config/core-isa.h" +#if defined(XCHAL_HAVE_WINDOWED) + + .data +recursion_count: + .word 0 + + .text + .global test_context_save_clober_func + .type test_context_save_clober_func,@function + .align 4 + +/* This function recursively calls itself via call4, making sure each frame + * uses only 4 registers. For this reason, recursion count can not be + * a function argument (it would have to be in a6) and is placed into .data + * section above. + */ +test_context_save_clober_func: + entry a1, 16 + + /* load recursion count from memory */ + movi a3, recursion_count + l32i a2, a3, 0 + + /* if it is zero, initialize it to 16 (=64 physical registers / 4 registers per call) */ + bnez a2, 1f + movi a2, 16 +1: + + /* decrement the counter and write it back */ + addi a2, a2, -1 + s32i a2, a3, 0 + + /* counter not zero? do a recursive call */ + beqz a2, wait + call4 test_context_save_clober_func + j end + +wait: + /* Counter has reached zero, and we have 16 frames on the stack. + * Delay for a few seconds, expecting in interrupt to happen. + */ + movi a3, 100000000 +1: + addi a3, a3, -1 + bnez a3, 1b + +end: + retw + +#endif // XCHAL_HAVE_WINDOWED +#endif // __XTENSA__ diff --git a/components/freertos/test/test_context_save_clobber.c b/components/freertos/test/test_context_save_clobber.c new file mode 100644 index 0000000000..7fd18cb57a --- /dev/null +++ b/components/freertos/test/test_context_save_clobber.c @@ -0,0 +1,43 @@ +#include "unity.h" +#include "esp_intr_alloc.h" + +#if defined(__XTENSA__) +#include "xtensa/config/core-isa.h" +#include "xtensa/hal.h" +#if defined(XCHAL_HAVE_WINDOWED) +/* Regression test for a0 register being corrupted in _xt_context_save. + * + * The idea in this test is to have a function which recursively calls itself + * with call4, eventually filling up all the register windows. At that point, + * it does some lengthy operation. If an interrupt occurs at that point, and + * corrupts a0 register of one of the windows, this will cause an exception + * when the recursive function returns. + */ + + +/* See test_context_save_clober_func.S */ +extern void test_context_save_clober_func(void); + +static void int_timer_handler(void *arg) +{ + xthal_set_ccompare(1, xthal_get_ccount() + 10000); + (*(int*) arg)++; +} + +TEST_CASE("context save doesn't corrupt return address register", "[freertos]") +{ + /* set up an interrupt */ + intr_handle_t ih; + int int_triggered = 0; + TEST_ESP_OK(esp_intr_alloc(ETS_INTERNAL_TIMER1_INTR_SOURCE, 0, int_timer_handler, &int_triggered, &ih)); + xthal_set_ccompare(1, xthal_get_ccount() + 10000); + + /* fill all the windows and delay a bit, waiting for an interrupt to happen */ + test_context_save_clober_func(); + + esp_intr_free(ih); + TEST_ASSERT_GREATER_THAN(0, int_triggered); +} + +#endif // XCHAL_HAVE_WINDOWED +#endif // __XTENSA__ diff --git a/components/freertos/xtensa/xtensa_context.S b/components/freertos/xtensa/xtensa_context.S index c46c8e4f2a..cb8c8ccbe3 100644 --- a/components/freertos/xtensa/xtensa_context.S +++ b/components/freertos/xtensa/xtensa_context.S @@ -156,7 +156,7 @@ _xt_context_save: movi a3, -XCHAL_EXTRA_SA_ALIGN and a2, a2, a3 /* align dynamically >16 bytes */ # endif - call0 xthal_save_extra_nw /* destroys a0,2,3,4,5 */ + call0 xthal_save_extra_nw /* destroys a0,2,3 */ #endif #ifndef __XTENSA_CALL0_ABI__ @@ -173,17 +173,23 @@ _xt_context_save: * at entry (CINTLEVEL=max(PS.INTLEVEL, XCHAL_EXCM_LEVEL) when PS.EXCM is set. * Since WindowOverflow exceptions will trigger inside SPILL_ALL_WINDOWS, * need to save/restore EPC1 as well. + * Note: even though a4-a15 are saved into the exception frame, we should not + * clobber them until after SPILL_ALL_WINDOWS. This is because these registers + * may contain live windows belonging to previous frames in the call stack. + * These frames will be spilled by SPILL_ALL_WINDOWS, and if the register was + * used as a temporary by this code, the temporary value would get stored + * onto the stack, instead of the real value. */ rsr a2, PS /* to be restored after SPILL_ALL_WINDOWS */ - movi a4, PS_INTLEVEL_MASK - and a3, a2, a4 /* get the current INTLEVEL */ + movi a0, PS_INTLEVEL_MASK + and a3, a2, a0 /* get the current INTLEVEL */ bgeui a3, XCHAL_EXCM_LEVEL, 1f /* calculate max(INTLEVEL, XCHAL_EXCM_LEVEL) */ movi a3, XCHAL_EXCM_LEVEL 1: - movi a4, PS_UM | PS_WOE /* clear EXCM, enable window overflow, set new INTLEVEL */ - or a3, a3, a4 + movi a0, PS_UM | PS_WOE /* clear EXCM, enable window overflow, set new INTLEVEL */ + or a3, a3, a0 wsr a3, ps - rsr a4, EPC1 /* to be restored after SPILL_ALL_WINDOWS */ + rsr a0, EPC1 /* to be restored after SPILL_ALL_WINDOWS */ addi sp, sp, XT_STK_FRMSZ /* go back to spill register region */ SPILL_ALL_WINDOWS /* place the live register windows there */ @@ -191,7 +197,7 @@ _xt_context_save: wsr a2, PS /* restore to the value at entry */ rsync - wsr a4, EPC1 /* likewise */ + wsr a0, EPC1 /* likewise */ #endif /* __XTENSA_CALL0_ABI__ */ From e2cb7ed9ca83991e07041005eb0ed5234d7db32e Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Wed, 2 Sep 2020 15:44:18 +0200 Subject: [PATCH 2/3] Revert "CXX: removed exception windowspill test" This reverts commit f3e180de726f715fcb82d031d2dff5ddbece20cd. --- components/cxx/test/test_cxx.cpp | 54 ++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/components/cxx/test/test_cxx.cpp b/components/cxx/test/test_cxx.cpp index e5cd24e71a..5e4483c37e 100644 --- a/components/cxx/test/test_cxx.cpp +++ b/components/cxx/test/test_cxx.cpp @@ -251,6 +251,60 @@ TEST_CASE("c++ exceptions emergency pool", "[cxx] [exceptions] [ignore] [leaks=" #endif } + +#define TIMEOUT 19 + +#define RECURSION 19 + +static esp_timer_handle_t crash_timer; + +static uint32_t result = 0; + +uint32_t calc_fac(uint32_t n) { + if (n == 1 || n == 0) { + return 1; + } else { + return n * calc_fac(n - 1); + } +} + +static void timer_cb(void *arg) { + result = calc_fac(RECURSION); +} + +// TODO: Not a unit test, refactor to integration test/system test, etc. +TEST_CASE("frequent interrupts don't interfere with c++ exceptions", "[cxx] [exceptions] [leaks=816]") +{// if exception workaround is disabled, this is almost guaranteed to fail + const esp_timer_create_args_t timer_args { + timer_cb, + NULL, + ESP_TIMER_TASK, + "crash_timer" + }; + + TEST_ESP_OK(esp_timer_create(&timer_args, &crash_timer)); + TEST_ESP_OK(esp_timer_start_periodic(crash_timer, TIMEOUT)); + + for (int i = 0; i < 500; i++) { + bool thrown_value = false; + try { + throw true; + } catch (bool e) { + thrown_value = e; + } + + if (thrown_value) { + printf("ex thrown %d\n", i); + } else { + printf("ex not thrown\n"); + TEST_ASSERT(false); + } + } + + TEST_ESP_OK(esp_timer_stop(crash_timer)); + TEST_ESP_OK(esp_timer_delete(crash_timer)); +} + #else // !CONFIG_COMPILER_CXX_EXCEPTIONS TEST_CASE("std::out_of_range exception when -fno-exceptions", "[cxx][reset=abort,SW_CPU_RESET]") From 60e4c02963e948c293031f9259665c12e9f443d2 Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Thu, 24 Sep 2020 18:52:57 +0200 Subject: [PATCH 3/3] ci: add unit test job --- tools/ci/config/target-test.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/ci/config/target-test.yml b/tools/ci/config/target-test.yml index 42f46b3a9c..5a054e4360 100644 --- a/tools/ci/config/target-test.yml +++ b/tools/ci/config/target-test.yml @@ -546,7 +546,7 @@ UT_034: UT_035: extends: .unit_test_s2_template - parallel: 36 + parallel: 37 tags: - ESP32S2_IDF - UT_T1_1