Merge pull request #21438 from maribu/sys/newlib/fix-stdio

build system: use thread-safe stdio

Author: maribu

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