From 1094c0812939ebca8d35a1b937a1402cc4573bb6 Mon Sep 17 00:00:00 2001 From: Kevin Bracey Date: Mon, 10 Sep 2018 11:15:08 +0300 Subject: [PATCH 1/5] Add support for UARTSerial::write from critical section --- drivers/UARTSerial.cpp | 21 +++++++++++++++++++++ drivers/UARTSerial.h | 3 +++ 2 files changed, 24 insertions(+) diff --git a/drivers/UARTSerial.cpp b/drivers/UARTSerial.cpp index d1ba724418..5026c9eb3b 100644 --- a/drivers/UARTSerial.cpp +++ b/drivers/UARTSerial.cpp @@ -134,6 +134,23 @@ void UARTSerial::sigio(Callback func) core_util_critical_section_exit(); } +/* Special synchronous write designed to work from critical section, such + * as in mbed_error_vfprintf. + */ +ssize_t UARTSerial::write_unbuffered(const char *buf_ptr, size_t length) +{ + while (!_txbuf.empty()) { + tx_irq(); + } + + for (size_t data_written = 0; data_written < length; data_written++) { + SerialBase::_base_putc(*buf_ptr++); + data_written++; + } + + return length; +} + ssize_t UARTSerial::write(const void *buffer, size_t length) { size_t data_written = 0; @@ -143,6 +160,10 @@ ssize_t UARTSerial::write(const void *buffer, size_t length) return 0; } + if (core_util_in_critical_section()) { + return write_unbuffered(buf_ptr, length); + } + api_lock(); // Unlike read, we should write the whole thing if blocking. POSIX only diff --git a/drivers/UARTSerial.h b/drivers/UARTSerial.h index 667591bf82..792d3e0f36 100644 --- a/drivers/UARTSerial.h +++ b/drivers/UARTSerial.h @@ -238,6 +238,9 @@ private: /** Release mutex */ virtual void api_unlock(void); + /** Unbuffered write - invoked when write called from critical section */ + ssize_t write_unbuffered(const char *buf_ptr, size_t length); + /** Software serial buffers * By default buffer size is 256 for TX and 256 for RX. Configurable through mbed_app.json */ From 78f4b4bc823d8a4129259cb364cf26b809b7cad8 Mon Sep 17 00:00:00 2001 From: Kevin Bracey Date: Mon, 10 Sep 2018 11:15:37 +0300 Subject: [PATCH 2/5] Make mbed_error not serial-specific Use write() on current output device instead - this works on the assumption that write() is safe to call from critical section. UARTSerial has previously been upgraded to support this, and this also improves the behaviour when buffered serial is in use - the current buffered output will be fully flushed before outputting the error message. --- platform/mbed_board.c | 23 +++++-------------- .../TARGET_CORTEX_M/mbed_rtx_fault_handler.c | 1 - 2 files changed, 6 insertions(+), 18 deletions(-) diff --git a/platform/mbed_board.c b/platform/mbed_board.c index 0c837cfb32..03b7c4c59f 100644 --- a/platform/mbed_board.c +++ b/platform/mbed_board.c @@ -18,13 +18,8 @@ #include "platform/mbed_wait_api.h" #include "platform/mbed_toolchain.h" #include "platform/mbed_interface.h" +#include "platform/mbed_retarget.h" #include "platform/mbed_critical.h" -#include "hal/serial_api.h" - -#if DEVICE_SERIAL -extern int stdio_uart_inited; -extern serial_t stdio_uart; -#endif WEAK void mbed_die(void) { @@ -61,30 +56,24 @@ void mbed_error_printf(const char *format, ...) void mbed_error_vfprintf(const char *format, va_list arg) { -#if DEVICE_SERIAL #define ERROR_BUF_SIZE (128) core_util_critical_section_enter(); char buffer[ERROR_BUF_SIZE]; int size = vsnprintf(buffer, ERROR_BUF_SIZE, format, arg); if (size > 0) { - if (!stdio_uart_inited) { - serial_init(&stdio_uart, STDIO_UART_TX, STDIO_UART_RX); - } -#if MBED_CONF_PLATFORM_STDIO_CONVERT_NEWLINES +#if MBED_CONF_PLATFORM_STDIO_CONVERT_NEWLINES || MBED_CONF_PLATFORM_STDIO_CONVERT_TTY_NEWLINES char stdio_out_prev = '\0'; for (int i = 0; i < size; i++) { if (buffer[i] == '\n' && stdio_out_prev != '\r') { - serial_putc(&stdio_uart, '\r'); + const char cr = '\r'; + write(STDERR_FILENO, &cr, 1); } - serial_putc(&stdio_uart, buffer[i]); + write(STDERR_FILENO, &buffer[i], 1); stdio_out_prev = buffer[i]; } #else - for (int i = 0; i < size; i++) { - serial_putc(&stdio_uart, buffer[i]); - } + write(STDERR_FILENO, buffer, size); #endif } core_util_critical_section_exit(); -#endif } diff --git a/rtos/TARGET_CORTEX/TARGET_CORTEX_M/mbed_rtx_fault_handler.c b/rtos/TARGET_CORTEX/TARGET_CORTEX_M/mbed_rtx_fault_handler.c index d9545b3f91..20607431e9 100644 --- a/rtos/TARGET_CORTEX/TARGET_CORTEX_M/mbed_rtx_fault_handler.c +++ b/rtos/TARGET_CORTEX/TARGET_CORTEX_M/mbed_rtx_fault_handler.c @@ -18,7 +18,6 @@ #include "device.h" #include "platform/mbed_error.h" #include "platform/mbed_interface.h" -#include "hal/serial_api.h" #ifndef MBED_FAULT_HANDLER_DISABLED #include "mbed_rtx_fault_handler.h" From d05c60ee3ff3859370f131d521c056b8b3da83a3 Mon Sep 17 00:00:00 2001 From: Kevin Bracey Date: Mon, 10 Sep 2018 11:15:51 +0300 Subject: [PATCH 3/5] Sync output devices on exit Mbed retarget does an `fflush` on stdout and stderr on exit - this flushes the C library buffers (if it is buffering), but doesn't flush any device buffers (eg UARTSerial's TX buffer). Add sync() calls to the output device to do this. --- platform/mbed_retarget.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/platform/mbed_retarget.cpp b/platform/mbed_retarget.cpp index a29d727c21..711ba0c6ae 100644 --- a/platform/mbed_retarget.cpp +++ b/platform/mbed_retarget.cpp @@ -1250,6 +1250,8 @@ extern "C" void exit(int return_code) #if MBED_CONF_PLATFORM_STDIO_FLUSH_AT_EXIT fflush(stdout); fflush(stderr); + fsync(STDOUT_FILENO); + fsync(STDERR_FILENO); #endif #endif From c989845d5a1a5af19b9da9a83fc55bb1a432a703 Mon Sep 17 00:00:00 2001 From: Kevin Bracey Date: Tue, 11 Sep 2018 11:18:32 +0300 Subject: [PATCH 4/5] mbed_error_vfprintf -> mbed_error_vprintf Name vfprintf doesn't make sense - if we have mbed_error_printf, this is vprintf. --- drivers/UARTSerial.cpp | 2 +- platform/mbed_board.c | 9 +++++++-- platform/mbed_error.c | 2 +- platform/mbed_error.h | 2 +- platform/mbed_interface.h | 7 +++++++ 5 files changed, 17 insertions(+), 5 deletions(-) diff --git a/drivers/UARTSerial.cpp b/drivers/UARTSerial.cpp index 5026c9eb3b..085257dd81 100644 --- a/drivers/UARTSerial.cpp +++ b/drivers/UARTSerial.cpp @@ -135,7 +135,7 @@ void UARTSerial::sigio(Callback func) } /* Special synchronous write designed to work from critical section, such - * as in mbed_error_vfprintf. + * as in mbed_error_vprintf. */ ssize_t UARTSerial::write_unbuffered(const char *buf_ptr, size_t length) { diff --git a/platform/mbed_board.c b/platform/mbed_board.c index 03b7c4c59f..0db7f930ed 100644 --- a/platform/mbed_board.c +++ b/platform/mbed_board.c @@ -50,11 +50,11 @@ void mbed_error_printf(const char *format, ...) { va_list arg; va_start(arg, format); - mbed_error_vfprintf(format, arg); + mbed_error_vprintf(format, arg); va_end(arg); } -void mbed_error_vfprintf(const char *format, va_list arg) +void mbed_error_vprintf(const char *format, va_list arg) { #define ERROR_BUF_SIZE (128) core_util_critical_section_enter(); @@ -77,3 +77,8 @@ void mbed_error_vfprintf(const char *format, va_list arg) } core_util_critical_section_exit(); } + +void mbed_error_vfprintf(const char *format, va_list arg) +{ + mbed_error_vprintf(format, arg); +} diff --git a/platform/mbed_error.c b/platform/mbed_error.c index 20e0efba1c..d833189d98 100644 --- a/platform/mbed_error.c +++ b/platform/mbed_error.c @@ -89,7 +89,7 @@ WEAK void error(const char *format, ...) #ifndef NDEBUG va_list arg; va_start(arg, format); - mbed_error_vfprintf(format, arg); + mbed_error_vprintf(format, arg); va_end(arg); #endif exit(1); diff --git a/platform/mbed_error.h b/platform/mbed_error.h index 1286b119ed..14adf03735 100644 --- a/platform/mbed_error.h +++ b/platform/mbed_error.h @@ -42,7 +42,7 @@ extern "C" { #else //MBED_CONF_PLATFORM_MAX_ERROR_FILENAME_LEN #if MBED_CONF_PLATFORM_MAX_ERROR_FILENAME_LEN > 64 //We have to limit this to 64 bytes since we use mbed_error_printf for error reporting -//and mbed_error_vfprintf uses 128bytes internal buffer which may not be sufficient for anything +//and mbed_error_vprintf uses 128bytes internal buffer which may not be sufficient for anything //longer that 64 bytes with the current implementation. #error "Unsupported error filename buffer length detected, max supported length is 64 chars. Please change MBED_CONF_PLATFORM_MAX_ERROR_FILENAME_LEN or max-error-filename-len in configuration." #endif diff --git a/platform/mbed_interface.h b/platform/mbed_interface.h index 48594916e8..0bb009a3fa 100644 --- a/platform/mbed_interface.h +++ b/platform/mbed_interface.h @@ -26,6 +26,7 @@ #include +#include "mbed_toolchain.h" #include "device.h" /* Mbed interface mac address @@ -146,9 +147,15 @@ void mbed_error_printf(const char *format, ...); * @param arg Variable arguments list * */ +void mbed_error_vprintf(const char *format, va_list arg); + +/** @deprecated Renamed to mbed_error_vprintf to match functionality */ +MBED_DEPRECATED_SINCE("mbed-os-5.11", + "Renamed to mbed_error_vprintf to match functionality.") void mbed_error_vfprintf(const char *format, va_list arg); /** @}*/ + #ifdef __cplusplus } #endif From 2df322c43dab717ffd1237ce0072b3a2ca3baa39 Mon Sep 17 00:00:00 2001 From: Kevin Bracey Date: Tue, 11 Sep 2018 11:40:23 +0300 Subject: [PATCH 5/5] Don't overrun in error prints vsprintf returns the amount it would have written if the buffer had been big enough, but we used that value directly when outputting, thus overrunning memory and dumping stack contents. Indicate truncation by inserting an ellipsis and newline. Slightly increase the buffer size, so that we don't slightly decrease the maximum printable characters because of the ellipsis insertion. Partially addresses https://github.com/ARMmbed/mbed-os/issues/6850 by forcing a newline when truncation happens - often truncation will drop a newline and prevent a flush. --- platform/mbed_board.c | 40 +++++++++++++++++++++++----------------- 1 file changed, 23 insertions(+), 17 deletions(-) diff --git a/platform/mbed_board.c b/platform/mbed_board.c index 0db7f930ed..f462a24cb8 100644 --- a/platform/mbed_board.c +++ b/platform/mbed_board.c @@ -56,25 +56,31 @@ void mbed_error_printf(const char *format, ...) void mbed_error_vprintf(const char *format, va_list arg) { -#define ERROR_BUF_SIZE (128) core_util_critical_section_enter(); - char buffer[ERROR_BUF_SIZE]; - int size = vsnprintf(buffer, ERROR_BUF_SIZE, format, arg); - if (size > 0) { -#if MBED_CONF_PLATFORM_STDIO_CONVERT_NEWLINES || MBED_CONF_PLATFORM_STDIO_CONVERT_TTY_NEWLINES - char stdio_out_prev = '\0'; - for (int i = 0; i < size; i++) { - if (buffer[i] == '\n' && stdio_out_prev != '\r') { - const char cr = '\r'; - write(STDERR_FILENO, &cr, 1); - } - write(STDERR_FILENO, &buffer[i], 1); - stdio_out_prev = buffer[i]; - } -#else - write(STDERR_FILENO, buffer, size); -#endif + char buffer[132]; + int size = vsnprintf(buffer, sizeof buffer, format, arg); + if (size >= sizeof buffer) { + /* Output was truncated - indicate by overwriting last 4 bytes of buffer + * with ellipsis and newline. + * (Note that although vsnprintf always leaves a NUL terminator, we + * don't need a terminator and can use the entire buffer) + */ + memcpy(&buffer[sizeof buffer - 4], "...\n", 4); + size = sizeof buffer; } +#if MBED_CONF_PLATFORM_STDIO_CONVERT_NEWLINES || MBED_CONF_PLATFORM_STDIO_CONVERT_TTY_NEWLINES + char stdio_out_prev = '\0'; + for (int i = 0; i < size; i++) { + if (buffer[i] == '\n' && stdio_out_prev != '\r') { + const char cr = '\r'; + write(STDERR_FILENO, &cr, 1); + } + write(STDERR_FILENO, &buffer[i], 1); + stdio_out_prev = buffer[i]; + } +#else + write(STDERR_FILENO, buffer, size); +#endif core_util_critical_section_exit(); }