From f69b6667b0792cf1fadd0d9c0440c2380a7fd415 Mon Sep 17 00:00:00 2001 From: Marian Buschsieweke Date: Fri, 2 May 2025 08:58:17 +0200 Subject: [PATCH 1/7] pkg/mpaland-printf: fix dependencies - `arch_64bit` depends on long long support (rather than just enabling it by default), as otherwise `%p` will not work correctly ==> add hard dependency - On 32 bit platforms support of printing long long is not almost free ==> do not enable long long on `arch_32bit` by default - The ESP SDK contains binary blobs that already link against newlib and mpaland-printf is not ABI compatible with newlib's stdio, it is only API compatible. ==> mark mpaland-printf as incompatible to ESP MCUs Co-authored-by: crasbe --- pkg/mpaland-printf/Makefile.dep | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/pkg/mpaland-printf/Makefile.dep b/pkg/mpaland-printf/Makefile.dep index dcb79a696c48..ee8fa39902be 100644 --- a/pkg/mpaland-printf/Makefile.dep +++ b/pkg/mpaland-printf/Makefile.dep @@ -3,8 +3,12 @@ # is not compatible with AVR MCUs. FEATURES_BLACKLIST += arch_avr8 -# On 32 bit and 64 bit systems support for long long is so cheap, that we -# can enable it by default. Users can still disable it, though. -ifneq (,$(filter arch_32bit arch_64bit,$(FEATURES_USED))) - DEFAULT_MODULE += printf_long_long +# On ESP it is not easily possible to replace newlib's stdio, as there is only +# API compatibility and not ABI compatibility between mpaland-printf and newlib's +# printf and the ESP SDK contains binary blobs. +FEATURES_BLACKLIST += arch_esp + +# On 64 bit systems, we depend on long long support to be able to support `%p` +ifneq (,$(filter arch_64bit,$(FEATURES_USED))) + USEMODULE += printf_long_long endif From 2ed57cb1542bfbccd27937fb736e08191ab488f1 Mon Sep 17 00:00:00 2001 From: Marian Buschsieweke Date: Fri, 25 Apr 2025 07:50:56 +0200 Subject: [PATCH 2/7] build system: introduce `bug_%` feature category This introduces a new feature category to declare which bugs are present in a given build that we cannot fix in RIOT, but need to work around. These bugs may be silicon bugs, software bugs in ROM, software bugs in binary blobs needed for a platform, or bugs in the toolchain. These features behave the same way as `arch_%` features: Every provided bug is always used, so that we can inspect `FEATURES_USED` to check which bugs need to be worked around. Co-authored-by: crasbe --- Makefile.include | 2 ++ features.yaml | 7 +++++++ 2 files changed, 9 insertions(+) diff --git a/Makefile.include b/Makefile.include index 9e242cd6b9b4..1d7b1c9febbe 100644 --- a/Makefile.include +++ b/Makefile.include @@ -423,6 +423,8 @@ include $(RIOTMAKE)/kconfig.mk FEATURES_REQUIRED += $(filter arch_%,$(FEATURES_PROVIDED)) # always select CPU core features FEATURES_REQUIRED += $(filter cpu_core_%,$(FEATURES_PROVIDED)) +# always "select" bug affecting the current build +FEATURES_REQUIRED += $(filter bug_%,$(FEATURES_PROVIDED)) # check if required features are provided and update $(FEATURES_USED) include $(RIOTMAKE)/features_check.inc.mk diff --git a/features.yaml b/features.yaml index 6174f93f4ba3..91594f52bbd7 100644 --- a/features.yaml +++ b/features.yaml @@ -922,3 +922,10 @@ groups: open and solder bridges SB4, SB5 and SB6 are closed instead, SPI2 is connected to the STMmod+/Pmod connector. Request this feature to use SPI2 with this board configuration. + +- title: Bugs in hardware, toolchain, or binary blobs + help: This allows modelling bugs found in boards, so that workarounds can + be enabled for affected builds by inspecting "FEATURES_USED". Any + feature prefixed with "bug_" provided in a build will always be used, + regardless of build configuration. + features: From 26232218548c8a881f96cd21e3581ee727dc17f2 Mon Sep 17 00:00:00 2001 From: Marian Buschsieweke Date: Fri, 25 Apr 2025 07:56:06 +0200 Subject: [PATCH 3/7] build system: use thread-safe stdio This makes use of the new bug modeling to declare all platforms that can use newlib and have no reentrancy hooks as affected by the non-thread-safe stdio bug. (Which is every platform but ESP* and AVR, the former because the reentrancy hooks are provided, the latter because we do not and never will support newlib on them.) Building on that, the mpaland-printf package is used when newlib is used and the bug is present. This way we can rely on the stdio being thread-safe on every platform and not causing random crashes at run time. Co-authored-by: mguetschow --- cpu/cortexm_common/Makefile.features | 3 ++- cpu/msp430/Makefile.features | 1 + cpu/riscv_common/Makefile.features | 1 + features.yaml | 7 +++++++ makefiles/features_existing.inc.mk | 1 + sys/Makefile.dep | 7 +++++++ 6 files changed, 19 insertions(+), 1 deletion(-) diff --git a/cpu/cortexm_common/Makefile.features b/cpu/cortexm_common/Makefile.features index c94905807fd4..1e6056dcf3c0 100644 --- a/cpu/cortexm_common/Makefile.features +++ b/cpu/cortexm_common/Makefile.features @@ -1,5 +1,6 @@ FEATURES_PROVIDED += arch_32bit FEATURES_PROVIDED += arch_arm +FEATURES_PROVIDED += bug_newlib_broken_stdio FEATURES_PROVIDED += cortexm_svc FEATURES_PROVIDED += cpp FEATURES_PROVIDED += cpu_check_address @@ -9,8 +10,8 @@ FEATURES_PROVIDED += libstdcpp FEATURES_PROVIDED += newlib FEATURES_PROVIDED += periph_flashpage_aux FEATURES_PROVIDED += periph_pm -FEATURES_PROVIDED += puf_sram FEATURES_PROVIDED += picolibc +FEATURES_PROVIDED += puf_sram FEATURES_PROVIDED += ssp # cortex-m33, cortex-m4f and cortex-m7 provide FPU support diff --git a/cpu/msp430/Makefile.features b/cpu/msp430/Makefile.features index 7107cccde90f..a272d3665da1 100644 --- a/cpu/msp430/Makefile.features +++ b/cpu/msp430/Makefile.features @@ -14,6 +14,7 @@ endif FEATURES_PROVIDED += arch_16bit FEATURES_PROVIDED += arch_msp430 +FEATURES_PROVIDED += bug_newlib_broken_stdio FEATURES_PROVIDED += cpu_$(CPU_FAM) FEATURES_PROVIDED += dbgpin FEATURES_PROVIDED += newlib diff --git a/cpu/riscv_common/Makefile.features b/cpu/riscv_common/Makefile.features index 46f9f267b4c9..f48285ffeee5 100644 --- a/cpu/riscv_common/Makefile.features +++ b/cpu/riscv_common/Makefile.features @@ -4,6 +4,7 @@ endif FEATURES_PROVIDED += arch_32bit FEATURES_PROVIDED += arch_riscv +FEATURES_PROVIDED += bug_newlib_broken_stdio FEATURES_PROVIDED += cpp FEATURES_PROVIDED += libstdcpp FEATURES_PROVIDED += newlib diff --git a/features.yaml b/features.yaml index 91594f52bbd7..7113af63e6c5 100644 --- a/features.yaml +++ b/features.yaml @@ -929,3 +929,10 @@ groups: feature prefixed with "bug_" provided in a build will always be used, regardless of build configuration. features: + - name: bug_newlib_broken_stdio + help: newlib's stdio is not inherently thread-safe, but depends on the + reentrancy hooks to be provided for correct operation. When + reentrancy is not enabled, using newlib's stdio is racy, causing + random crashes at runtime. Use of this feature will cause an + alternative stdio implementation to be used if (and only if) newlib + is used as standard c lib. diff --git a/makefiles/features_existing.inc.mk b/makefiles/features_existing.inc.mk index de51f5cd7655..6420ccd089cd 100644 --- a/makefiles/features_existing.inc.mk +++ b/makefiles/features_existing.inc.mk @@ -44,6 +44,7 @@ FEATURES_EXISTING := \ ble_phy_coded \ board_bat_voltage \ bootloader_stm32 \ + bug_newlib_broken_stdio \ can_rx_mailbox \ cortexm_fpu \ cortexm_mpu \ diff --git a/sys/Makefile.dep b/sys/Makefile.dep index dd583117fa04..381a2d4bd764 100644 --- a/sys/Makefile.dep +++ b/sys/Makefile.dep @@ -229,6 +229,13 @@ ifneq (,$(filter newlib,$(USEMODULE))) endif endif endif + ifneq (,$(filter bug_newlib_broken_stdio,$(FEATURES_USED))) + # work around broken newlib stdio by using alternative stdio implementation, + # unless stdio_null is used, which is inherently thread-safe :) + ifeq (,$(filter stdio_null,$(USEMODULE))) + USEPKG += mpaland-printf + endif + endif endif ifneq (,$(filter posix_select,$(USEMODULE))) From 674dede8aee91f5a72f5bd38405f918421a53605 Mon Sep 17 00:00:00 2001 From: Marian Buschsieweke Date: Fri, 25 Apr 2025 08:03:23 +0200 Subject: [PATCH 4/7] sys/newlib: drop workaround for stdio This drops a workaround that initialized newlib's reentrancy structure on boot to reduce the chances of crashes when using the non-thread-safe (unless reentrancy hooks are provided) stdio implementation of newlib. Now that the newlib stdio implementation is only ever used if it is thread-safe, we no longer need a workaround that reduces the chance of crashes on concurrent use of stdio. --- sys/newlib_syscalls_default/syscalls.c | 18 +----------------- 1 file changed, 1 insertion(+), 17 deletions(-) diff --git a/sys/newlib_syscalls_default/syscalls.c b/sys/newlib_syscalls_default/syscalls.c index a19d706647dc..fda4333b9f5b 100644 --- a/sys/newlib_syscalls_default/syscalls.c +++ b/sys/newlib_syscalls_default/syscalls.c @@ -144,23 +144,7 @@ static const struct heap heaps[NUM_HEAPS] = { */ void _init(void) { - /* Definition copied from newlib/libc/stdio/local.h */ - extern void __sinit (struct _reent *); - - /* When running multiple threads: Initialize reentrant structure before the - * scheduler starts. This normally happens upon the first stdio function - * called. However, if no boot message happens this can result in two - * concurrent "first calls" to stdio in data corruption, if no locking is - * used. Except for ESP (which is using its own syscalls.c anyway), this - * currently is the case in RIOT. */ - if (MAXTHREADS > 1) { - /* Also, make an exception for riotboot, which does not use stdio - * at all. This would pull in stdio and increase .text size - * significantly there */ - if (!IS_USED(MODULE_RIOTBOOT)) { - __sinit(_REENT); - } - } + /* nothing to do here */ } /** From 64c76246c5ed7ed47507a72cd05a357cb3e06d3f Mon Sep 17 00:00:00 2001 From: Marian Buschsieweke Date: Mon, 14 Jul 2025 22:58:56 +0200 Subject: [PATCH 5/7] core/assert: always declare _assert_panic() / _assert_failure() That way users can rely on elimation of dead code from the optimizer instead of having to use preprocessor conditionals when feature-gating assert implementations behind `!NDEBUG`. Co-authored-by: benpicco Co-authored-by: crasbe Co-authored-by: mguetschow --- core/lib/include/assert.h | 66 ++++++++++++++++++++++----------------- 1 file changed, 38 insertions(+), 28 deletions(-) diff --git a/core/lib/include/assert.h b/core/lib/include/assert.h index 8891a95cc2bb..cda77e5999f6 100644 --- a/core/lib/include/assert.h +++ b/core/lib/include/assert.h @@ -44,7 +44,7 @@ extern "C" { * CFLAGS += -DDEBUG_ASSERT_VERBOSE * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ */ -#define DEBUG_ASSERT_VERBOSE +# define DEBUG_ASSERT_VERBOSE /** * @brief Activate breakpoints for @ref assert() when defined @@ -58,32 +58,39 @@ extern "C" { * execution is stopped directly in the debugger. Otherwise the execution stops * in an endless while loop. */ -#define DEBUG_ASSERT_BREAKPOINT +# define DEBUG_ASSERT_BREAKPOINT #else /* we should not include custom headers in standard headers */ -#define _likely(x) __builtin_expect((uintptr_t)(x), 1) +# define _likely(x) __builtin_expect((uintptr_t)(x), 1) #endif +#ifndef __NORETURN +# if defined(__GNUC__) || defined(DOXYGEN) /** - * @def __NORETURN - * @brief hidden (__) NORETURN definition + * @brief Same as @ref NORETURN * @internal * - * Duplicating the definitions of kernel_defines.h as these are unsuitable for - * system header files like the assert.h. - * kernel_defines.h would define symbols that are not reserved. + * We are not using @ref NORETURN from `compiler_hints.h` here to avoid RIOT + * dependencies for standard C headers */ -#ifndef __NORETURN -#ifdef __GNUC__ -#define __NORETURN __attribute__((noreturn)) -#else /*__GNUC__*/ -#define __NORETURN -#endif /*__GNUC__*/ -#endif /*__NORETURN*/ +# define __NORETURN __attribute__((noreturn)) +# else +# define __NORETURN +# endif +#endif + +/** + * @brief Internal function to trigger a panic with a failed assertion as + * identifying cause + * @internal + * @warning This is an internal function. API changes may happen without regard + * for out-of tree uses. + * + * The implementation will identify the cause of the panic as a blown assertion, + * e.g. via a log output. + */ +__NORETURN void _assert_panic(void); -#ifdef NDEBUG -#define assert(ignore)((void)0) -#elif defined(DEBUG_ASSERT_VERBOSE) /** * @brief Function to handle failed assertion * @@ -95,6 +102,10 @@ extern "C" { * @param[in] line The code line of @p file the assertion failed in */ __NORETURN void _assert_failure(const char *file, unsigned line); + +#ifdef NDEBUG +# define assert(ignore)((void)0) +#elif defined(DEBUG_ASSERT_VERBOSE) /** * @brief abort the program if assertion is false * @@ -132,27 +143,26 @@ __NORETURN void _assert_failure(const char *file, unsigned line); * * @see http://pubs.opengroup.org/onlinepubs/9699919799/functions/assert.html */ -#define assert(cond) (_likely(cond) ? (void)0 : _assert_failure(__FILE__, __LINE__)) +# define assert(cond) (_likely(cond) ? (void)0 : _assert_failure(__FILE__, __LINE__)) #else /* DEBUG_ASSERT_VERBOSE */ -__NORETURN void _assert_panic(void); -#define assert(cond) (_likely(cond) ? (void)0 : _assert_panic()) +# define assert(cond) (_likely(cond) ? (void)0 : _assert_panic()) #endif /* DEBUG_ASSERT_VERBOSE */ #if !defined __cplusplus -#if __STDC_VERSION__ >= 201112L +# if __STDC_VERSION__ >= 201112L /** * @brief c11 static_assert() macro */ -#define static_assert(...) _Static_assert(__VA_ARGS__) -#else +# define static_assert(...) _Static_assert(__VA_ARGS__) +# else /** * @brief static_assert for c-version < c11 * * Generates a division by zero compile error when cond is false */ -#define static_assert(cond, ...) \ - { enum { static_assert_failed_on_div_by_0 = 1 / (!!(cond)) }; } -#endif +# define static_assert(cond, ...) \ + { enum { static_assert_failed_on_div_by_0 = 1 / (!!(cond)) }; } +# endif #endif /** @@ -161,7 +171,7 @@ __NORETURN void _assert_panic(void); * If the assertion failed in an interrupt, the system will still panic. */ #ifndef DEBUG_ASSERT_NO_PANIC -#define DEBUG_ASSERT_NO_PANIC (1) +# define DEBUG_ASSERT_NO_PANIC (1) #endif #ifdef __cplusplus From 3415656a4afe9ada47c843b0506824e338608782 Mon Sep 17 00:00:00 2001 From: Marian Buschsieweke Date: Tue, 27 May 2025 23:06:42 +0200 Subject: [PATCH 6/7] sys/newlib_syscalls_default: wrap newlib's assert implementation This avoids inconsistent output when external code gets linked in that directly links against newlib's assert implementation (e.g. binary blobs or packages that do not add `core/lib/include` to the include paths). This also greatly benefits wrapping printf, as newlib's `__assert_func()` directly links to internal printf functions of newlib. Co-authored-by: crasbe --- makefiles/libc/newlib.mk | 4 ++++ pkg/mpaland-printf/Makefile.include | 4 ++++ sys/newlib_syscalls_default/syscalls.c | 18 ++++++++++++++++++ 3 files changed, 26 insertions(+) diff --git a/makefiles/libc/newlib.mk b/makefiles/libc/newlib.mk index c34b3545d045..b0d729f43a15 100644 --- a/makefiles/libc/newlib.mk +++ b/makefiles/libc/newlib.mk @@ -100,3 +100,7 @@ ifeq (1,$(USE_NEWLIB_NANO)) INCLUDES := -isystem $(NEWLIB_NANO_INCLUDE_DIR) $(INCLUDES) endif endif + +# In case externally compiled code gets linked in, it may use newlib's assert +# implementation. We wrap that to RIOT's assert implementation for consistency. +LINKFLAGS += -Wl,-wrap=__assert_func diff --git a/pkg/mpaland-printf/Makefile.include b/pkg/mpaland-printf/Makefile.include index e1d77aaeae2d..5a11eebce648 100644 --- a/pkg/mpaland-printf/Makefile.include +++ b/pkg/mpaland-printf/Makefile.include @@ -19,6 +19,10 @@ LINKFLAGS += -Wl,-wrap=puts # stdio, we will catch any instance of both mpaland-printf and newlib's stdio # being both linked in. LINKFLAGS += -Wl,-wrap=_printf_common +LINKFLAGS += -Wl,-wrap=_fprintf_r +LINKFLAGS += -Wl,-wrap=_printf_r +LINKFLAGS += -Wl,-wrap=iprintf +LINKFLAGS += -Wl,-wrap=fiprintf # Workaround for bug in the newlib headers shipped with e.g. Ubuntu 24.04 LTS # not defining PRId64 / PRIu64 / PRIx64 / PRIo64 in due to an issue diff --git a/sys/newlib_syscalls_default/syscalls.c b/sys/newlib_syscalls_default/syscalls.c index fda4333b9f5b..4c6f2a623863 100644 --- a/sys/newlib_syscalls_default/syscalls.c +++ b/sys/newlib_syscalls_default/syscalls.c @@ -34,6 +34,7 @@ #include #include +#include "assert.h" #include "log.h" #include "modules.h" #include "periph/pm.h" @@ -646,3 +647,20 @@ clock_t _times_r(struct _reent *ptr, struct tms *ptms) return (-1); } + +/** + * @brief Wrapper for newlib's assert implementation + */ +NORETURN +void __wrap___assert_func (const char *file, int line, const char *func, const char *expr) +{ + (void)file; + (void)line; + (void)func; + (void)expr; +#ifdef DEBUG_ASSERT_VERBOSE + _assert_failure(file, line); +#else + _assert_panic(); +#endif +} From 4a84f6f58e47c330c813a5eba5ab6efe45d6c64c Mon Sep 17 00:00:00 2001 From: Marian Buschsieweke Date: Thu, 21 Aug 2025 10:35:46 +0200 Subject: [PATCH 7/7] tests/sys/snprintf: use printf_long_long This is required to support printing 64 bit numbers, as is done in the test. In addition that 64 bit formatting test is feature gated, so that users can simply comment out the `USEMODULE += printf_long_long` to fit tiny boards. Co-authored-by: mguetschow --- tests/sys/snprintf/Makefile | 5 +++++ tests/sys/snprintf/main.c | 9 ++++++++- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/tests/sys/snprintf/Makefile b/tests/sys/snprintf/Makefile index d5e587b491b3..db6f70cd7577 100644 --- a/tests/sys/snprintf/Makefile +++ b/tests/sys/snprintf/Makefile @@ -4,4 +4,9 @@ include ../Makefile.sys_common # to compile due to PRI*64 macros not being defined FEATURES_BLACKLIST := arch_avr8 +# This enables support for printing 64 bit numbers. You can comment this out +# to shrink the test app. The test will skip the corresponding test +# automatically if printf_long_long is not used. +USEMODULE += printf_long_long + include $(RIOTBASE)/Makefile.include diff --git a/tests/sys/snprintf/main.c b/tests/sys/snprintf/main.c index bf9b33886779..29a61f25863c 100644 --- a/tests/sys/snprintf/main.c +++ b/tests/sys/snprintf/main.c @@ -27,6 +27,8 @@ #include #include +#include "modules.h" + static bool failed = false; static void check(const char *expected, const char *got, int retval) @@ -277,7 +279,12 @@ int main(void) test_int8(); test_int16(); test_int32(); - test_int64(); + if (IS_USED(MODULE_PRINTF_LONG_LONG)) { + test_int64(); + } + else { + puts("WARNING: Module printf_long_long not used, skipping test for 64 bit"); + } test_size(); test_flags_widths();