Minimal-printf: Fix wrapping of printf functions for the ARM compiler #12238

hugueskamba · 2020-01-10T16:14:53Z

Summary of changes

As the ARM compiler is in GNU compatible mode, it defines GNU which
(based on pre-processor condition in mbed_printf_implentation.h)
causes the printf functions to be wrapped using wrap* instead of
$Sub$$*.

This commit modifies the pre-processor
conditions to check for __GNUC__last in order to correctly
substitute the printf functions depending on the toolchain in use.

It also gets rid of $Super$$* substitution as it is not needed. $Super$$
is used to identify the original unpatched functions.

Missing substitutions for ARM compiler internal optimized "printfs" are
also added.

Impact of changes

Migration actions required

Documentation

Pull request type

[X] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[X] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers

@kjbracey-arm @evedon

evedon · 2020-01-10T16:36:20Z

Based on the documentation you found, shouldn't we also modify the wrapping for vprintf?

hugueskamba · 2020-01-10T16:57:10Z

Based on the documentation you found, shouldn't we also modify the wrapping for vprintf?

I have not found any documentation that says that __2vsnprintf does not exist. What I found was documentation about the printf functions that have optimised __Nprintf variants. It does not mention vprintf family of functions so I assumed vsnprintf does not have such optimised variants. I tested it because mbed_error_printf uses vnsprintf.

I have nothing to suggest that __Nvprintf does not exist, I would have to test vprintf with a sample application.

hugueskamba · 2020-01-10T17:16:44Z

Based on the documentation you found, shouldn't we also modify the wrapping for vprintf?

I have not found any documentation that says that __2vsnprintf does not exist. What I found was documentation about the printf functions that have optimised __Nprintf variants. It does not mention vprintf family of functions so I assumed vsnprintf does not have such optimised variants. I tested it because mbed_error_printf uses vnsprintf.

I have nothing to suggest that __Nvprintf does not exist, I would have to test vprintf with a sample application.

@evedon
vprintf substitution function had an incorrect signature and its content was also wrong.
This force-push adds the fix for vprintf. The PR description and title have been updated.
Thanks

0xc0170 · 2020-01-13T08:48:23Z

CI started

kjbracey

This seems wrong to me, or at least I don't understand why it improves anything - I've started discussion with Hugues offline.

kjbracey · 2020-01-13T08:55:48Z

platform/source/minimal-printf/mbed_printf_armlink_overrides.c

@@ -66,17 +66,12 @@ int $Sub$$__2snprintf(char *buffer, size_t length, const char *format, ...)
    return result;
 }

-int $Sub$$__2vprintf(char *buffer, const char *format, ...)
+int $Sub$$vprintf(const char *format, va_list arguments)


Basic issue: isn't this now overriding vprintf twice? Isn't there already a definition of $Sub$$vprintf in mbed_printf_wrapper.c? That's how it looks to me reading it, but then it apparently isn't working?

It is a little tricky.
The ARM compiler does not recognise int SUB_VPRINTF(const char *format, va_list arguments){} as a definition for vprintf in mbed_printf_wrapper.c. However, it recognises it when it is changed to int $Sub$$vprintf(const char *format, va_list arguments){}.

It seems the ARM compiler requires $Sub$$ to be in the function signature when it is defined.
So all those SUB_* definitions in mbed_printf_wrapper.c are ignored for the ARM compiler. Only the definitions in mbed_printf_armliink_overrides.c count.

SUB_VPRINTF is defined as $Sub$$vprintf in mbed_printf_wrapper.c, isn't it? I must be missing something.

SUB_VPRINTF is defined as $Sub$$vprintf in mbed_printf_wrapper.c, isn't it? I must be missing something.

You are right but I am not sure how the compiler handles $Sub$$ if a pre-processor definition is also used. What I do know is that it does use the definition in mbed_printf_wrapper.c.

Wouldn't expect any oddity. Compiler documentation simply says "The dollar character $ is allowed in identifiers." Preprocessor should treat that the same as any other identifier character then.

Working with Hugues, we've established that mbed_printf_wrapper.c is actually generating the __wrap functions rather than the $Sub$$ functions because it tests like

#if defined(__GNUC__) #elif defined(TOOLCHAIN_ARM) || defined(__ICCARM)

But the ARM compiler is in GCC-"compatible" mode, so is defining __GNUC__.

Suggest reversing those blocks.

Then mbed_printf_wrapper.c should work for the base functions, but mbed_printf_armlink_overrides needs to provide all the corresponding __2 functions. That includes the v functions, as we've established that the compiler will use __2vprintf if it knows the format. (The mbed_error_printf doesn't know the format, so uses vsnprintf, but __2vsnprintf is still possible in principle).

It looks like mbed_prinf_armlink_overrides is missing __2vsprintf, __2fprintf and __2vfprintf.

I wonder if possibly you should eliminate the repetition and avoid inconsistency by putting all the definitions into a header file. Then do

#if defined __ARMCC_VERSION || defined __ICCARM__ /* Intercept both printf and __2printf using $Sub$$ */ #define SUB_PRINTF $Sub$$printf #include "printf_wrappers.h" #undef SUB_PRINTF #define SUB_PRINTF $Sub$$__2printf #include "printf_wrappers.h" #elif defined __GNUC__ /* Intercept both printf using __wrap_ */ #define SUB_PRINTF __wrap_printf #include " printf_wrappers.h" #endif

Actually, even neater, do what mbed_retarget.cpp does to determine the prefix. You don't need one define per function, just a define to paste on the $Sub$$, $Sub$$__2 or __wrap_ prefix. See its PREFIX(x) macro.

So you define int PREFIX(printf)(const char *, ...) rather than int SUB_PRINTF(const char *, ...)

mbed-ci · 2020-01-13T09:45:21Z

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 1
Build artifacts

As the ARM compiler is in GNU compatible mode, it defines __GNU__ which (based on pre-processor condition in mbed_printf_implentation.h) causes the printf functions to be wrapped using _wrap_* instead of $Sub$$*. This commit modifies the pre-processor conditions to check for __GNUC__last in order to correctly substitute the printf functions depending on the toolchain in use. It also gets rid of $Super$$* substitution as it is not needed. $Super$$ is used to identify the original unpatched functions. Missing substitutions for ARM compiler internal optimized "printfs" are also added.

kjbracey

I'd still rather see the two sets combined to avoid duplication and potential errors, as suggested in my previous comment. (I've not cross-checked the two files).

Maybe something for a follow-up PR (along with my __attribute__((naked)) suggestion from chat to remove space taken by _printf_c etc).

But this seems like a valid fix now.

adbridge · 2020-01-13T13:08:18Z

@kjbracey-arm let's get this in for now as it affects nightly :)

adbridge · 2020-01-13T13:09:57Z

CI started

mbed-ci · 2020-01-13T14:14:45Z

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 2
Build artifacts

evedon approved these changes Jan 10, 2020

View reviewed changes

hugueskamba force-pushed the hk-minimal_printf-fix-vsnprintf branch from a401a3e to b2483c1 Compare January 10, 2020 17:13

hugueskamba changed the title ~~Minimal-printf: Fix wrapping of vsnprintf for ARM compiler~~ Minimal-printf: Fix wrapping of vsnprintf and vprintf for ARM compiler Jan 10, 2020

hugueskamba force-pushed the hk-minimal_printf-fix-vsnprintf branch from b2483c1 to fb075bc Compare January 10, 2020 17:14

0xc0170 added the needs: CI label Jan 13, 2020

0xc0170 approved these changes Jan 13, 2020

View reviewed changes

kjbracey suggested changes Jan 13, 2020

View reviewed changes

0xc0170 added needs: work and removed needs: CI labels Jan 13, 2020

kjbracey reviewed Jan 13, 2020

View reviewed changes

hugueskamba force-pushed the hk-minimal_printf-fix-vsnprintf branch from fb075bc to 99cd2ce Compare January 13, 2020 11:11

hugueskamba changed the title ~~Minimal-printf: Fix wrapping of vsnprintf and vprintf for ARM compiler~~ Minimal-printf: Fix wrapping of printf functions for the ARM compiler Jan 13, 2020

hugueskamba force-pushed the hk-minimal_printf-fix-vsnprintf branch from 99cd2ce to ba01aba Compare January 13, 2020 11:15

hugueskamba force-pushed the hk-minimal_printf-fix-vsnprintf branch from ba01aba to 3b59f6c Compare January 13, 2020 11:25

kjbracey approved these changes Jan 13, 2020

View reviewed changes

adbridge added needs: CI release-version: 6.0.0-alpha-1 First pre-release version of 6.0.0 and removed needs: work labels Jan 13, 2020

0xc0170 removed the needs: CI label Jan 13, 2020

0xc0170 merged commit 9243abb into ARMmbed:master Jan 13, 2020

hugueskamba deleted the hk-minimal_printf-fix-vsnprintf branch January 14, 2020 08:04

Minimal-printf: Fix wrapping of printf functions for the ARM compiler #12238

Minimal-printf: Fix wrapping of printf functions for the ARM compiler #12238

Uh oh!

Conversation

hugueskamba commented Jan 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of changes

Impact of changes

Migration actions required

Documentation

Pull request type

Test results

Reviewers

Uh oh!

evedon commented Jan 10, 2020

Uh oh!

hugueskamba commented Jan 10, 2020

Uh oh!

hugueskamba commented Jan 10, 2020

Uh oh!

0xc0170 commented Jan 13, 2020

Uh oh!

kjbracey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kjbracey Jan 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbed-ci commented Jan 13, 2020

Uh oh!

kjbracey left a comment

Choose a reason for hiding this comment

Uh oh!

adbridge commented Jan 13, 2020

Uh oh!

adbridge commented Jan 13, 2020

Uh oh!

mbed-ci commented Jan 13, 2020

Uh oh!

Uh oh!

hugueskamba commented Jan 10, 2020 •

edited

Loading

kjbracey Jan 13, 2020 •

edited

Loading