build system: use thread-safe stdio

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

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

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

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

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

features.yaml

@@ -922,3 +922,17 @@ 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:
+  - 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.

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 \

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

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

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 <inttypes.h> due to an issue

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)))

sys/newlib_syscalls_default/syscalls.c

@@ -34,6 +34,7 @@
 #include <sys/unistd.h>
 #include <unistd.h>
 
+#include "assert.h"
 #include "log.h"
 #include "modules.h"
 #include "periph/pm.h"
@@ -144,23 +145,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 */
 }
 
 /**
@@ -662,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
+}

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

tests/sys/snprintf/main.c

@@ -27,6 +27,8 @@
 #include <string.h>
 #include <unistd.h>
 
+#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();