From 03bc6488de1b1b1bfc6503c2019ab89b5ed0912f Mon Sep 17 00:00:00 2001 From: Jakob Hasse Date: Wed, 21 Dec 2022 14:25:28 +0800 Subject: [PATCH] bugfix(esp_system): made watchpoint setting configuration-dependent esp_execute_shared_stack_function always restored the stack watchpoint regardless of CONFIG_FREERTOS_WATCHPOINT_END_OF_STACK. This would lead to an abondoned but active watchpoint on a former stack once the task calling esp_execute_shared_stack_function is deleted, if CONFIG_FREERTOS_WATCHPOINT_END_OF_STACK is inactive. This has been fixed now. Closes https://github.com/espressif/esp-idf/issues/10414 --- .../port/arch/riscv/expression_with_stack.c | 20 ++--- .../port/arch/xtensa/expression_with_stack.c | 20 ++--- tools/ci/check_copyright_ignore.txt | 2 - tools/test_apps/.build-test-rules.yml | 4 + .../system/test_watchpoint/CMakeLists.txt | 9 +++ .../system/test_watchpoint/README.md | 2 + .../test_watchpoint/main/CMakeLists.txt | 2 + .../system/test_watchpoint/main/main.c | 73 +++++++++++++++++++ .../test_watchpoint/pytest_watchpoint.py | 12 +++ .../system/test_watchpoint/sdkconfig.defaults | 1 + 10 files changed, 117 insertions(+), 28 deletions(-) create mode 100644 tools/test_apps/system/test_watchpoint/CMakeLists.txt create mode 100644 tools/test_apps/system/test_watchpoint/README.md create mode 100644 tools/test_apps/system/test_watchpoint/main/CMakeLists.txt create mode 100644 tools/test_apps/system/test_watchpoint/main/main.c create mode 100644 tools/test_apps/system/test_watchpoint/pytest_watchpoint.py create mode 100644 tools/test_apps/system/test_watchpoint/sdkconfig.defaults diff --git a/components/esp_system/port/arch/riscv/expression_with_stack.c b/components/esp_system/port/arch/riscv/expression_with_stack.c index 07d22bf3aa..a47d3f1193 100644 --- a/components/esp_system/port/arch/riscv/expression_with_stack.c +++ b/components/esp_system/port/arch/riscv/expression_with_stack.c @@ -1,16 +1,8 @@ -// Copyright 2015-2019 Espressif Systems (Shanghai) PTE LTD -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. +/* + * SPDX-FileCopyrightText: 2015-2022 Espressif Systems (Shanghai) CO LTD + * + * SPDX-License-Identifier: Apache-2.0 + */ #include #include @@ -66,7 +58,9 @@ void esp_execute_shared_stack_function(SemaphoreHandle_t lock, void *stack, size //Restore current task stack: current->pxDummy6 = (StackType_t *)current_task_stack; +#if CONFIG_FREERTOS_WATCHPOINT_END_OF_STACK vPortSetStackWatchpoint(current->pxDummy6); +#endif portEXIT_CRITICAL(&shared_stack_spinlock); xSemaphoreGive(lock); diff --git a/components/esp_system/port/arch/xtensa/expression_with_stack.c b/components/esp_system/port/arch/xtensa/expression_with_stack.c index 22ba16268b..7d8e714250 100644 --- a/components/esp_system/port/arch/xtensa/expression_with_stack.c +++ b/components/esp_system/port/arch/xtensa/expression_with_stack.c @@ -1,16 +1,8 @@ -// Copyright 2015-2019 Espressif Systems (Shanghai) PTE LTD -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. +/* + * SPDX-FileCopyrightText: 2015-2022 Espressif Systems (Shanghai) CO LTD + * + * SPDX-License-Identifier: Apache-2.0 + */ #include #include @@ -75,7 +67,9 @@ void esp_execute_shared_stack_function(SemaphoreHandle_t lock, void *stack, size //Restore current task stack: current->pxDummy6 = (StackType_t *)current_task_stack; +#if CONFIG_FREERTOS_WATCHPOINT_END_OF_STACK vPortSetStackWatchpoint(current->pxDummy6); +#endif portEXIT_CRITICAL(&xtensa_shared_stack_spinlock); xSemaphoreGive(lock); diff --git a/tools/ci/check_copyright_ignore.txt b/tools/ci/check_copyright_ignore.txt index 2e154f41d2..05f6720488 100644 --- a/tools/ci/check_copyright_ignore.txt +++ b/tools/ci/check_copyright_ignore.txt @@ -558,8 +558,6 @@ components/esp_system/include/esp_freertos_hooks.h components/esp_system/include/esp_int_wdt.h components/esp_system/include/esp_private/dbg_stubs.h components/esp_system/include/esp_private/panic_internal.h -components/esp_system/port/arch/riscv/expression_with_stack.c -components/esp_system/port/arch/xtensa/expression_with_stack.c components/esp_system/port/public_compat/brownout.h components/esp_system/port/public_compat/cache_err_int.h components/esp_system/port/public_compat/trax.h diff --git a/tools/test_apps/.build-test-rules.yml b/tools/test_apps/.build-test-rules.yml index 4f90e9521a..501fc3d94d 100644 --- a/tools/test_apps/.build-test-rules.yml +++ b/tools/test_apps/.build-test-rules.yml @@ -174,3 +174,7 @@ tools/test_apps/system/ram_loadable_app: tools/test_apps/system/startup: enable: - if: INCLUDE_DEFAULT == 1 or IDF_TARGET in ["esp32h4", "esp32c6"] # preview targets + +tools/test_apps/system/test_watchpoint: + enable: + - if: IDF_TARGET in ["esp32", "esp32c3"] # Just one test per architecture diff --git a/tools/test_apps/system/test_watchpoint/CMakeLists.txt b/tools/test_apps/system/test_watchpoint/CMakeLists.txt new file mode 100644 index 0000000000..d89fbfdd8a --- /dev/null +++ b/tools/test_apps/system/test_watchpoint/CMakeLists.txt @@ -0,0 +1,9 @@ +# The following lines of boilerplate have to be in your project's +# CMakeLists in this exact order for cmake to work correctly +cmake_minimum_required(VERSION 3.5) + +include($ENV{IDF_PATH}/tools/cmake/project.cmake) + +set(COMPONENTS main) + +project(test_watchpoint) diff --git a/tools/test_apps/system/test_watchpoint/README.md b/tools/test_apps/system/test_watchpoint/README.md new file mode 100644 index 0000000000..1fb88efd15 --- /dev/null +++ b/tools/test_apps/system/test_watchpoint/README.md @@ -0,0 +1,2 @@ +| Supported Targets | ESP32 | ESP32-C3 | +| ----------------- | ----- | -------- | diff --git a/tools/test_apps/system/test_watchpoint/main/CMakeLists.txt b/tools/test_apps/system/test_watchpoint/main/CMakeLists.txt new file mode 100644 index 0000000000..cf2c455cb5 --- /dev/null +++ b/tools/test_apps/system/test_watchpoint/main/CMakeLists.txt @@ -0,0 +1,2 @@ +idf_component_register(SRCS "main.c" + INCLUDE_DIRS ".") diff --git a/tools/test_apps/system/test_watchpoint/main/main.c b/tools/test_apps/system/test_watchpoint/main/main.c new file mode 100644 index 0000000000..78430cecc9 --- /dev/null +++ b/tools/test_apps/system/test_watchpoint/main/main.c @@ -0,0 +1,73 @@ +/* + * SPDX-FileCopyrightText: 2023 Espressif Systems (Shanghai) CO LTD + * + * SPDX-License-Identifier: Unlicense OR CC0-1.0 + */ + +/* + * This test checks that there are no orphan watchpoints are left on temporary stacks used by + * esp_execute_shared_stack_function(). + */ + +#include +#include +#include "esp_expression_with_stack.h" +#include "freertos/FreeRTOS.h" +#include "freertos/task.h" + +#define TEST_TASK_STACK_SIZE 1024 +#define SHARED_STACK_SIZE 8192 + +void external_stack_function(void) +{ + printf("Executing this printf from external stack! \n"); +} + +/** + * The two stacks are statically allocated so that we can later examine them and trip any potential remaining + * watchpoints. + */ +uint8_t static test_task_stack[TEST_TASK_STACK_SIZE]; +uint8_t static shared_stack[SHARED_STACK_SIZE]; + +static void shared_stack_executer(void *arg) +{ + SemaphoreHandle_t printf_lock = xSemaphoreCreateMutex(); + assert(printf_lock != NULL); + + //Call the desired function using the macro helper: + esp_execute_shared_stack_function(printf_lock, + shared_stack, + sizeof(shared_stack), + external_stack_function); + + vSemaphoreDelete(printf_lock); + + vTaskDelete(NULL); +} + +void app_main(void) +{ + StaticTask_t task_buffer; + TaskHandle_t task_handle = xTaskCreateStatic(shared_stack_executer, + "test_shared_test_task", + sizeof(test_task_stack), + NULL, + tskIDLE_PRIORITY + 1, + test_task_stack, + &task_buffer); + + assert(task_handle); + + vTaskDelay(pdMS_TO_TICKS(500)); + + // If any watchpoints have been left on the temporary stacks, we will trigger them here: + for (int i = 0; i < sizeof(test_task_stack); i++) { + test_task_stack[i] = 0; + } + for (int i = 0; i < sizeof(shared_stack); i++) { + shared_stack[i] = 0; + } + + printf("stacks clean\n"); +} diff --git a/tools/test_apps/system/test_watchpoint/pytest_watchpoint.py b/tools/test_apps/system/test_watchpoint/pytest_watchpoint.py new file mode 100644 index 0000000000..0d61d757b3 --- /dev/null +++ b/tools/test_apps/system/test_watchpoint/pytest_watchpoint.py @@ -0,0 +1,12 @@ +# SPDX-FileCopyrightText: 2023 Espressif Systems (Shanghai) CO LTD +# SPDX-License-Identifier: CC0-1.0 + +import pytest +from pytest_embedded import Dut + + +@pytest.mark.esp32 +@pytest.mark.esp32c3 +@pytest.mark.generic +def test_watchpoint(dut: Dut) -> None: + dut.expect_exact('stacks clean', timeout=3) diff --git a/tools/test_apps/system/test_watchpoint/sdkconfig.defaults b/tools/test_apps/system/test_watchpoint/sdkconfig.defaults new file mode 100644 index 0000000000..e1ed9340db --- /dev/null +++ b/tools/test_apps/system/test_watchpoint/sdkconfig.defaults @@ -0,0 +1 @@ +CONFIG_FREERTOS_WATCHPOINT_END_OF_STACK=n