Skip to content

minimal-printf should not bypass the retargetting code #11235

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 29, 2019
Merged

minimal-printf should not bypass the retargetting code #11235

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions TESTS/mbed_platform/minimal-printf/compliance/mbed_printf.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ int mbed_printf(const char *format, ...)
{
va_list arguments;
va_start(arguments, format);
int result = mbed_minimal_formatted_string(NULL, LONG_MAX, format, arguments, NULL);
int result = mbed_minimal_formatted_string(NULL, LONG_MAX, format, arguments, stdout);
va_end(arguments);

return result;
Expand All @@ -41,15 +41,14 @@ int mbed_snprintf(char *buffer, size_t length, const char *format, ...)

int mbed_vprintf(const char *format, va_list arguments)
{
return mbed_minimal_formatted_string(NULL, LONG_MAX, format, arguments, NULL);
return mbed_minimal_formatted_string(NULL, LONG_MAX, format, arguments, stdout);
}

int mbed_vsnprintf(char *buffer, size_t length, const char *format, va_list arguments)
{
return mbed_minimal_formatted_string(buffer, length, format, arguments, NULL);
}

#if MBED_CONF_PLATFORM_MINIMAL_PRINTF_ENABLE_FILE_STREAM
int mbed_fprintf(FILE *stream, const char *format, ...)
{
va_list arguments;
Expand All @@ -64,4 +63,3 @@ int mbed_vfprintf(FILE *stream, const char *format, va_list arguments)
{
return mbed_minimal_formatted_string(NULL, LONG_MAX, format, arguments, stream);
}
#endif
2 changes: 0 additions & 2 deletions TESTS/mbed_platform/minimal-printf/compliance/mbed_printf.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,6 @@ int mbed_vprintf(const char *format, va_list arguments);
*/
int mbed_vsnprintf(char *buffer, size_t length, const char *format, va_list arguments);

#if MBED_CONF_PLATFORM_MINIMAL_PRINTF_ENABLE_FILE_STREAM
/**
* Minimal fprintf
*
Expand All @@ -66,7 +65,6 @@ int mbed_fprintf(FILE *stream, const char *format, ...);
* Prints directly to file stream without using malloc.
*/
int mbed_vfprintf(FILE *stream, const char *format, va_list arguments);
#endif

#ifdef __cplusplus
}
Expand Down
7 changes: 0 additions & 7 deletions TESTS/mbed_platform/minimal-printf/compliance/test_app.json
View file
Open in desktop

This file was deleted.

8 changes: 0 additions & 8 deletions platform/mbed_lib.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,18 +128,10 @@
"help": "Use the MPU if available to fault execution from RAM and writes to ROM. Can be disabled to reduce image size.",
"value": true
},
"minimal-printf-console-output": {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should minimal-printf-enable-file-stream also be removed given mbed_minimal_putchar no longer considers it?
If it should be removed, references in mbed_printf.c/h and mbed_printf_wrapper.c also should be removed.

Copy link
Collaborator

@hugueskamba hugueskamba Aug 29, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ec7e232

"help": "Console output when using mprintf profile. Options: UART, SWO",
"value": "UART"
},
"minimal-printf-enable-64-bit": {
"help": "Enable printing 64 bit integers when using mprintf profile",
"value": true
},
"minimal-printf-enable-file-stream": {
"help": "Enable printing to a FILE stream when using mprintf profile",
"value": true
},
"minimal-printf-enable-floating-point": {
"help": "Enable floating point printing when using mprintf profile",
"value": true
Expand Down
14 changes: 3 additions & 11 deletions platform/source/minimal-printf/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
Library supports both printf and snprintf in 1252 bytes of flash.

Prints directly to stdio/UART without using malloc. All flags and precision modifiers are ignored.
There is no error handling if a writing error occurs.

Supports:
* %d: signed integer [h, hh, (none), l, ll, z, j, t].
Expand All @@ -30,22 +31,14 @@ Floating point limitations:

Minimal printf is configured by the following parameters defined in `platform/mbed_lib.json`:

```
```json
{
"name": "platform",
"config": {
"minimal-printf-console-output": {
"help": "Console output when using minimal-printf profile. Options: UART, SWO",
"value": "UART"
},
"minimal-printf-enable-64-bit": {
"help": "Enable printing 64 bit integers when using minimal-printf profile",
"value": true
},
"minimal-printf-enable-file-stream": {
"help": "Enable printing to a FILE stream when using minimal-printf profile",
"value": true
},
"minimal-printf-enable-floating-point": {
"help": "Enable floating point printing when using minimal-printf profile",
"value": true
Expand All @@ -64,10 +57,9 @@ If your target does not require some options then you can override the default c

In mbed_app.json:

```
```json
"target_overrides": {
"*": {
"platform.minimal-printf-enable-file-stream": false,
"platform.minimal-printf-enable-floating-point": false,
"platform.minimal-printf-set-floating-point-max-decimals": 6,
"platform.minimal-printf-enable-64-bit": false
Expand Down
63 changes: 1 addition & 62 deletions platform/source/minimal-printf/mbed_printf_implementation.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,59 +26,10 @@
/***************************/
#if TARGET_LIKE_MBED

#define CONSOLE_OUTPUT_UART 1
#define CONSOLE_OUTPUT_SWO 2
#define mbed_console_concat_(x) CONSOLE_OUTPUT_##x
#define mbed_console_concat(x) mbed_console_concat_(x)
#define CONSOLE_OUTPUT mbed_console_concat(MBED_CONF_PLATFORM_MINIMAL_PRINTF_CONSOLE_OUTPUT)

#if MBED_CONF_PLATFORM_STDIO_CONVERT_NEWLINES
static char mbed_stdio_out_prev = 0;
#endif

#if CONSOLE_OUTPUT == CONSOLE_OUTPUT_UART
#if DEVICE_SERIAL
/*
Serial initialization and new line replacement is a direct copy from mbed_retarget.cpp
If the static modifier were to be removed, this part of the code would not be necessary.
*/
#include "hal/serial_api.h"

static serial_t stdio_uart = { 0 };

/* module variable for keeping track of initialization */
static bool not_initialized = true;

static void init_serial()
{
if (not_initialized) {
not_initialized = false;

serial_init(&stdio_uart, STDIO_UART_TX, STDIO_UART_RX);
#if MBED_CONF_PLATFORM_STDIO_BAUD_RATE
serial_baud(&stdio_uart, MBED_CONF_PLATFORM_STDIO_BAUD_RATE);
#endif
}
}

#define MBED_INITIALIZE_PRINT(x) { init_serial(); }
#define MBED_PRINT_CHARACTER(x) { serial_putc(&stdio_uart, x); }

#else

#define MBED_INITIALIZE_PRINT(x)
#define MBED_PRINT_CHARACTER(x)

#endif // if DEVICE_SERIAL

#elif CONSOLE_OUTPUT == CONSOLE_OUTPUT_SWO

#include "hal/itm_api.h"

#define MBED_INITIALIZE_PRINT(x) { mbed_itm_init(); }
#define MBED_PRINT_CHARACTER(x) { mbed_itm_send(ITM_PORT_SWO, x); }

#endif // if CONSOLE_OUTPUT

/***************************/
/* Linux */
Expand All @@ -88,8 +39,6 @@ static void init_serial()
#define MBED_CONF_PLATFORM_MINIMAL_PRINTF_ENABLE_FLOATING_POINT 1
#define MBED_CONF_PLATFORM_MINIMAL_PRINTF_SET_FLOATING_POINT_MAX_DECIMALS 6
#define MBED_CONF_PLATFORM_MINIMAL_PRINTF_ENABLE_64_BIT 1
#define MBED_INITIALIZE_PRINT(x) { ; }
#define MBED_PRINT_CHARACTER(x) { printf("%c", x); }
#endif

#ifndef MBED_CONF_PLATFORM_MINIMAL_PRINTF_ENABLE_FLOATING_POINT
Expand Down Expand Up @@ -177,14 +126,7 @@ static void mbed_minimal_putchar(char *buffer, size_t length, int *result, char
if (buffer) {
buffer[*result] = data;
} else {
#if MBED_CONF_PLATFORM_MINIMAL_PRINTF_ENABLE_FILE_STREAM
if (stream) {
fputc(data, (FILE *) stream);
} else
#endif
{
MBED_PRINT_CHARACTER(data);
}
fputc(data, stream);
}
}
/* increment 'result' even if data was not written. This ensures that
Expand Down Expand Up @@ -434,9 +376,6 @@ static void mbed_minimal_formatted_string_string(char *buffer, size_t length, in
*/
int mbed_minimal_formatted_string(char *buffer, size_t length, const char *format, va_list arguments, FILE *stream)
{
/* initialize output if needed */
MBED_INITIALIZE_PRINT();

int result = 0;
bool empty_buffer = false;

Expand Down
24 changes: 3 additions & 21 deletions platform/source/minimal-printf/mbed_printf_wrapper.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,12 +34,10 @@
#define SUB_VSPRINTF __wrap_vsprintf
#define SUPER_VSNPRINTF __real_vsnprintf
#define SUB_VSNPRINTF __wrap_vsnprintf
#if MBED_CONF_PLATFORM_MINIMAL_PRINTF_ENABLE_FILE_STREAM
#define SUPER_FPRINTF __real_fprintf
#define SUB_FPRINTF __wrap_fprintf
#define SUPER_VFPRINTF __real_vfprintf
#define SUB_VFPRINTF __wrap_vfprintf
#endif
#elif defined(TOOLCHAIN_ARM) /* ARMC5/ARMC6 */\
|| defined(__ICCARM__) /* IAR */
#define SUPER_PRINTF $Super$$printf
Expand All @@ -54,33 +52,19 @@
#define SUB_VSPRINTF $Sub$$vsprintf
#define SUPER_VSNPRINTF $Super$$vsnprintf
#define SUB_VSNPRINTF $Sub$$vsnprintf
#if MBED_CONF_PLATFORM_MINIMAL_PRINTF_ENABLE_FILE_STREAM
#define SUPER_FPRINTF $Super$$fprintf
#define SUB_FPRINTF $Sub$$fprintf
#define SUPER_VFPRINTF $Super$$vfprintf
#define SUB_VFPRINTF $Sub$$vfprintf
#endif
#else
#warning "This compiler is not yet supported."
#endif

// Prevent optimization of printf() by the ARMCC or IAR compiler.
// This is done to prevent optimization which can cause printf() to be
// substituted with a different function (e.g. puts()) and cause
// the output to be missing some strings.
// Note: Optimization prevention for other supported compilers is done
// via compiler optional command line arguments.
#if defined(__CC_ARM) /* ARMC5 */
#pragma push
#pragma O0
#elif defined(__ICCARM__) /* IAR */
#pragma optimize=none
#endif
int SUB_PRINTF(const char *format, ...)
{
va_list arguments;
va_start(arguments, format);
int result = mbed_minimal_formatted_string(NULL, LONG_MAX, format, arguments, NULL);
int result = mbed_minimal_formatted_string(NULL, LONG_MAX, format, arguments, stdout);
va_end(arguments);

return result;
Expand Down Expand Up @@ -108,7 +92,7 @@ int SUB_SNPRINTF(char *buffer, size_t length, const char *format, ...)

int SUB_VPRINTF(const char *format, va_list arguments)
{
return mbed_minimal_formatted_string(NULL, LONG_MAX, format, arguments, NULL);
return mbed_minimal_formatted_string(NULL, LONG_MAX, format, arguments, stdout);
}

int SUB_VSPRINTF(char *buffer, const char *format, va_list arguments)
Expand All @@ -121,7 +105,6 @@ int SUB_VSNPRINTF(char *buffer, size_t length, const char *format, va_list argum
return mbed_minimal_formatted_string(buffer, length, format, arguments, NULL);
}

#if MBED_CONF_PLATFORM_MINIMAL_PRINTF_ENABLE_FILE_STREAM
int SUB_FPRINTF(FILE *stream, const char *format, ...)
{
va_list arguments;
Expand All @@ -136,6 +119,5 @@ int SUB_VFPRINTF(FILE *stream, const char *format, va_list arguments)
{
return mbed_minimal_formatted_string(NULL, LONG_MAX, format, arguments, stream);
}
#endif

#endif // MBED_MINIMAL_PRINTF
#endif // MBED_MINIMAL_PRINTF
4 changes: 2 additions & 2 deletions tools/profiles/extensions/minimal-printf.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
{
"GCC_ARM": {
"common": ["-DMBED_MINIMAL_PRINTF", "-fno-builtin-printf"],
"common": ["-DMBED_MINIMAL_PRINTF"],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this removed?

Copy link
Contributor

@kjbracey kjbracey Aug 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the change is making printf work "normally", so the compiler no longer needs to treat it as "abnormal".

Without this option, the compiler can transform printf("Hello\n") into puts("Hello"), or printf(".") into putchar('.'). That would have triggered the problem we're now fixing.

Putting this option in meant output was consistent as long as the user always used printf and never did puts or putchar themselves.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. I need to understand how that retarget work.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not so much about how the retargetting works in detail as the fact you're short-circuiting it

printf, putchar and puts are all supposed to go to the same place as fputc(stdout).

If those things don't all go to the same place, or skip the buffering among only some paths, then things get weird when you mix them.

(The retargetting controls where fputc(stdout) goes, and then all other I/O calls follow).

"ld": ["-Wl,--wrap,printf", "-Wl,--wrap,sprintf", "-Wl,--wrap,snprintf",
"-Wl,--wrap,vprintf", "-Wl,--wrap,vsprintf", "-Wl,--wrap,vsnprintf",
"-Wl,--wrap,fprintf", "-Wl,--wrap,vfprintf"]
},
"ARMC6": {
"common": ["-DMBED_MINIMAL_PRINTF", "-fno-builtin-printf"]
"common": ["-DMBED_MINIMAL_PRINTF"]
},
"ARM": {
"common": ["-DMBED_MINIMAL_PRINTF"]
Expand Down