From d6797867fbf0ba47e554ac4969f71b6c971ca2bc Mon Sep 17 00:00:00 2001 From: Marian Buschsieweke Date: Fri, 10 Nov 2023 11:12:20 +0100 Subject: [PATCH 1/2] sys/flash_utils: Fix signature in documentation The function signature in the Doxygen generated doc was incorrect. Given that the implementation does have the correct signature, the impact may not be that large. Still, incorrect doc is confusing, so let's fix it. --- sys/include/flash_utils.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sys/include/flash_utils.h b/sys/include/flash_utils.h index dc4721f1c7..44f54b8991 100644 --- a/sys/include/flash_utils.h +++ b/sys/include/flash_utils.h @@ -207,7 +207,7 @@ void flash_puts(FLASH_ATTR const char *flash); * @param[in] n number of bytes to copy * */ -void * flash_memcpy(void *dest, FLASH_ATTR const char *src, size_t n); +void * flash_memcpy(void *dest, FLASH_ATTR const void *src, size_t n); #elif !IS_ACTIVE(HAS_FLASH_UTILS_ARCH) # define FLASH_ATTR # define PRIsflash "s" From cee7cccfd0e3b418abfd053655c68f5975fa796d Mon Sep 17 00:00:00 2001 From: Marian Buschsieweke Date: Fri, 10 Nov 2023 11:13:22 +0100 Subject: [PATCH 2/2] cpu/avr8_common/flash_utils: use C and linker for aliases `flash_()` is implemented by `_P()` provided by the AVR libc on AVR targets. Previously, the preprocessor was used to do the aliasing, but this causes issues with LLVM: The signatures of e.g. `printf_P()` expects `const char *`, whereas flash utils expects `FLASH_ATTR const char *`. For GCC this will just implicitly drop the `FLASH_ATTR`, while it requires an explicit cast for LLVM. To implement the explicit cast, `static inline` function wrappers where used instead where possible. But for the variadic functions (e.g. `printf(fmt, ...)`) the linker is used to provide the aliases, as there is no way to pass the variadic functions throw in C. The alternative would be to implement `flash_printf()` by calling `vprintf_P()`, but that increased ROM size quite a bit. Finally, a work around for a bug in Ubuntu's toolchain has been added: An unused function that calls to `printf_P()`, `fprintf_P()` and `snprintf_P()`. Since this function is garbage collected anyway, it has no impact on the generated ELF file. --- cpu/avr8_common/Makefile.include | 4 + cpu/avr8_common/include/flash_utils_arch.h | 78 +++++++++++++++---- .../work_around_for_shitty_ubuntu_toolchain.c | 15 ++++ 3 files changed, 82 insertions(+), 15 deletions(-) create mode 100644 cpu/avr8_common/work_around_for_shitty_ubuntu_toolchain.c diff --git a/cpu/avr8_common/Makefile.include b/cpu/avr8_common/Makefile.include index ce73116c97..295bdea2aa 100644 --- a/cpu/avr8_common/Makefile.include +++ b/cpu/avr8_common/Makefile.include @@ -7,4 +7,8 @@ ifneq (,$(filter printf_float,$(USEMODULE))) LINKFLAGS += -Wl,-u,vfprintf -lprintf_flt -lm endif +# Add aliases for flash_printf, flash_fprintf, flash_snprintf: +LINKFLAGS += -Wl,--defsym=flash_printf=printf_P +LINKFLAGS += -Wl,--defsym=flash_fprintf=fprintf_P +LINKFLAGS += -Wl,--defsym=flash_snprintf=snprintf_P include $(RIOTMAKE)/arch/avr8.inc.mk diff --git a/cpu/avr8_common/include/flash_utils_arch.h b/cpu/avr8_common/include/flash_utils_arch.h index 9663da8b89..43127e65e4 100644 --- a/cpu/avr8_common/include/flash_utils_arch.h +++ b/cpu/avr8_common/include/flash_utils_arch.h @@ -21,7 +21,7 @@ #define FLASH_UTILS_ARCH_H #include -#include +#include #include #ifdef __cplusplus @@ -35,20 +35,68 @@ extern "C" { #define FLASH_ATTR __flash #define PRIsflash "S" #define TO_FLASH(x) __extension__({static FLASH_ATTR const char __c[] = (x); &__c[0];}) -#define flash_strcmp strcmp_P -#define flash_strncmp strncmp_P -#define flash_strlen strlen_P -#define flash_strcpy strcpy_P -#define flash_strncpy strncpy_P -#define flash_printf printf_P -/* avrlibc seemingly forgot to provide vprintf_P(), but vfprintf_P() is there */ -#define flash_vprintf(fmt, arglist) vfprintf_P(stdout, fmt, arglist) -#define flash_fprintf fprintf_P -#define flash_vfprintf vfprintf_P -#define flash_snprintf snprintf_P -#define flash_vsnprintf vsnprintf_P -#define flash_puts puts_P -#define flash_memcpy memcpy_P + +static inline int flash_strcmp(const char *ram, FLASH_ATTR const char *flash) +{ + return strcmp_P(ram, (const char *)flash); +} + +static inline int flash_strncmp(const char *ram, FLASH_ATTR const char *flash, size_t n) +{ + return strncmp_P(ram, (const char *)flash, n); +} + +static inline size_t flash_strlen(FLASH_ATTR const char *flash) +{ + return strlen_P((const char *)flash); +} + +static inline char * flash_strcpy(char *ram, FLASH_ATTR const char *flash) +{ + return strcpy_P(ram, (const char *)flash); +} + +static inline char * flash_strncpy(char *ram, FLASH_ATTR const char *flash, size_t n) +{ + return strncpy_P(ram, (const char *)flash, n); +} + + +static inline int flash_vprintf(FLASH_ATTR const char *flash, va_list args) +{ + /* vprintf_P() is not provided by avr-libc. But vfprintf_P() with + * stdout as stream can be used to implement it */ + return vfprintf_P(stdout, (const char *)flash, args); +} + +static inline int flash_vfprintf(FILE *stream, FLASH_ATTR const char *flash, + va_list args) +{ + return vfprintf_P(stream, (const char *)flash, args); +} + +static inline int flash_vsnprintf(char *buf, size_t buf_len, + FLASH_ATTR const char *flash, va_list args) +{ + return vsnprintf_P(buf, buf_len, (const char *)flash, args); +} + +static inline void flash_puts(FLASH_ATTR const char *flash) +{ + puts_P((const char *)flash); +} + +static inline void * flash_memcpy(void *dest, FLASH_ATTR const void *src, + size_t n) +{ + return memcpy_P(dest, (const void *)src, n); +} + +/* aliases need to be provided by the linker, as passing through va-args is + * not possible */ +int flash_printf(FLASH_ATTR const char *flash, ...); +int flash_fprintf(FILE *stream, FLASH_ATTR const char *flash, ...); +int flash_snprintf(char *buf, size_t buf_len, FLASH_ATTR const char *flash, ...); #endif /* Doxygen */ diff --git a/cpu/avr8_common/work_around_for_shitty_ubuntu_toolchain.c b/cpu/avr8_common/work_around_for_shitty_ubuntu_toolchain.c new file mode 100644 index 0000000000..dfe4535ffc --- /dev/null +++ b/cpu/avr8_common/work_around_for_shitty_ubuntu_toolchain.c @@ -0,0 +1,15 @@ +#include +#include + +/* The outdated linker from Ubuntu's toolchain contains a bug in which it will + * garbage collect symbols referenced only by --defsym= command line options, + * and subsequently complain that the symbols are not defined. Adding other + * references to those symbols from an unused function makes that buggy linker + * happy. Since this function is never used, it will be garbage collected and + * not impact the ROM size. */ +void work_around_for_shitty_ubuntu_toolchain_and_not_expected_to_be_called(void) +{ + printf_P(NULL); + fprintf_P(stdout, NULL); + snprintf_P(NULL, 0, NULL); +}