minimal-printf: Fix handling of the two character sequence %% #11729

hugueskamba · 2019-10-22T14:30:05Z

Description

The two character sequence %% is used in standard implementations of
printf to print a single %. This is because % is essentially printf's
escape character for format specifiers and as \% cannot work printf
uses %%.
Therefore to be compatible with string buffers containing
%%, minimal-printf also needs to only print a single %.

To reproduce the bug.

Build mbed-os-example-blinky and program your target with the following command

$ mbed compile -t ARM -m <TARGET> --profile develop --profile mbed-os/tools/profile/extension/minimal_printf.json -f

Open a terminal to view the serial output of the target. It should display an incorrect out similar to:

=============================== SYSTEM INFO ================================
Mbed OS Version: 999999 
CPU ID: 0x410fc241 
Compiler ID: 2 
Compiler Version: 80200 
RAM0: Start 0x20000000 Size: 0x30000 
RAM1: Start 0x1fff0000 Size: 0x10000 
ROM0: Start 0x0 Size: 0x100000 
================= CPU STATS =================
Idle: 4%% Usage: 96%% 
================ HEAP STATS =================
Current heap: 1216
Max heap size: 1216
================ THREAD STATS ===============
ID: 0x20000e50 
Name: main 
State: 2 
Priority: 24 
Stack Size: 4096 
Stack Space: 3476 
ID: 0x20000fa0 
Name: rtx_idle 
State: 1 
Priority: 1 
Stack Size: 512 
Stack Space: 272 
ID: 0x20000f5c 
Name: rtx_timer 
State: 3 
Priority: 40 
Stack Size: 768 
Stack Space: 664

With the fix the output is similar to:

=============================== SYSTEM INFO  ================================
Mbed OS Version: 999999 
CPU ID: 0x410fc241 
Compiler ID: 2 
Compiler Version: 80200 
RAM0: Start 0x20000000 Size: 0x30000 
RAM1: Start 0x1fff0000 Size: 0x10000 
ROM0: Start 0x0 Size: 0x100000 
================= CPU STATS =================
Idle: 4% Usage: 96%
================ HEAP STATS =================
Current heap: 1216
Max heap size: 1216
================ THREAD STATS ===============
ID: 0x20000e50 
Name: main 
State: 2 
Priority: 24 
Stack Size: 4096 
Stack Space: 3476 
ID: 0x20000fa0 
Name: rtx_idle 
State: 1 
Priority: 1 
Stack Size: 512 
Stack Space: 272 
ID: 0x20000f5c 
Name: rtx_timer 
State: 3 
Priority: 40 
Stack Size: 768 
Stack Space: 664

Pull request type

[X] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@evedon @kjbracey-arm

Release Notes

ciarmcom · 2019-10-22T15:00:36Z

@hugueskamba, thank you for your changes.
@kjbracey-arm @evedon @ARMmbed/mbed-os-core @ARMmbed/mbed-os-maintainers please review.

kjbracey · 2019-10-23T08:10:11Z

platform/source/minimal-printf/mbed_printf_implementation.c

@@ -625,6 +625,9 @@ int mbed_minimal_formatted_string(char *buffer, size_t length, const char *forma
                    index = next_index;

                    mbed_minimal_formatted_string_void_pointer(buffer, length, &result, value, stream);
+                } else if (next == '%') {
+                    index = next_index;
+                    mbed_minimal_formatted_string_signed(buffer, length, &result, next, stream);


signed? Isn't this printing "25"? Surely that should read

mbed_minimal_formatted_string_character(buffer, length, &result, '%', stream);

Although, just to save space, I wonder if you could cheat and handle this in the overall else:

} else { /* Handle `%%` - skip the first `%`, so we print just `%` via the "unrecognized modifier" code */ if (next == `%`) { index++; } /* Write all characters between format beginning and unrecognized modifier */

I believe that handles what the standard requires - the conversion specifier %, with the entire conversion specification being %%. Behaviour of "%.5%` is undefined.

Although on the other hand that else loop is clunky by the way. Maybe, instead, the much simpler:

} else { // Unrecognised, or `%%`. Print the `%` that led us in. mbed_minimal_formatted_string_character(buffer, length, &result, '%', stream); if (next == '%') { // Continue printing loop after `%%` index = next_index; } // Otherwise we continue the printing loop after the leading `%`, so an // unrecognised thing like "Blah = %a" will just come out as "Blah = %a". }

Although, just to save space, I wonder if you could cheat and handle this in the overall

Your proposed change increases flash memory usage by 8 bytes.

This force-push uses mbed_minimal_formatted_string_character.

The first suggestion, I guess? What about the second - that should be a big reduction.

Although on the other hand that else loop is clunky by the way. Maybe, instead, the much simpler:

This force-push applies this suggested change for a 44 bytes flash memory saving.

Cool. I have one more suggestion that may make it even tighter. Adjust the entire loop to get rid of the else on if (format[index] == '%'). Make each normal recognised case continue instead, then % and unrecognised can fall into the standard character print by not continueing.

for (size_t index = 0; format[index] != '\0'; index++) { if (format[index] == '%') { //... char next = format[next_index]; if ((next == 'd') || (next == 'i')) { //... index = next_index; continue; } else if ((next == 'u') || (next == 'x') || (next == 'X')) { //... Note that putting `index = next_index` last in every case // should promote code sharing between each block index = next_index; continue; } else if (next == '%') { index = next_index; } } mbed_minimal_formatted_string_character(buffer, length, &result, format[index], stream); }

That's an optimisation worth trying but I'd rather that we do it in a different PR as we are getting behind on other tasks.

evedon

Please add test coverage in greentea minimal-printf test suite.

The two character sequence %% is used in standard implementations of printf to print a single %. This is because % is essentially printf's escape character for format specifiers and as \% cannot work printf uses %%. Therefore to be compatible with string buffers containing %%, minimal-printf also needs to only print a single %.

hugueskamba · 2019-10-23T10:32:15Z

Please add test coverage in greentea minimal-printf test suite.

@evedon
Done.

mbed-ci · 2019-10-23T13:10:46Z

Test run: FAILED

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

Failed test jobs:

jenkins-ci/mbed-os-ci_exporter

0xc0170 · 2019-10-23T13:14:03Z

Internal CI fault ,restarted exporters

ciarmcom requested review from evedon, kjbracey and a team October 22, 2019 15:00

ciarmcom added the needs: review label Oct 22, 2019

hugueskamba force-pushed the hk-fix-minimal-printf-percentage-printing branch from 7297f37 to 8e89d99 Compare October 23, 2019 07:07

kjbracey reviewed Oct 23, 2019

View reviewed changes

hugueskamba force-pushed the hk-fix-minimal-printf-percentage-printing branch 2 times, most recently from ad4eca6 to 1ebd043 Compare October 23, 2019 08:45

0xc0170 approved these changes Oct 23, 2019

View reviewed changes

hugueskamba force-pushed the hk-fix-minimal-printf-percentage-printing branch from 1ebd043 to a29776a Compare October 23, 2019 09:42

evedon reviewed Oct 23, 2019

View reviewed changes

hugueskamba force-pushed the hk-fix-minimal-printf-percentage-printing branch from a29776a to ceffb6d Compare October 23, 2019 10:31

evedon approved these changes Oct 23, 2019

View reviewed changes

kjbracey approved these changes Oct 23, 2019

View reviewed changes

0xc0170 added needs: CI and removed needs: review labels Oct 23, 2019

0xc0170 added ready for merge release-version: 5.14.2 and removed needs: CI labels Oct 23, 2019

0xc0170 merged commit 6240335 into ARMmbed:master Oct 23, 2019

0xc0170 removed the ready for merge label Oct 23, 2019

hugueskamba deleted the hk-fix-minimal-printf-percentage-printing branch October 23, 2019 14:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

minimal-printf: Fix handling of the two character sequence %% #11729

minimal-printf: Fix handling of the two character sequence %% #11729

Uh oh!

hugueskamba commented Oct 22, 2019

Uh oh!

ciarmcom commented Oct 22, 2019

Uh oh!

kjbracey Oct 23, 2019

Uh oh!

hugueskamba Oct 23, 2019

Uh oh!

kjbracey Oct 23, 2019

Uh oh!

hugueskamba Oct 23, 2019

Uh oh!

kjbracey Oct 23, 2019

Uh oh!

evedon Oct 23, 2019

Uh oh!

evedon left a comment

Uh oh!

hugueskamba commented Oct 23, 2019

Uh oh!

mbed-ci commented Oct 23, 2019

Uh oh!

0xc0170 commented Oct 23, 2019

Uh oh!

Uh oh!

minimal-printf: Fix handling of the two character sequence %% #11729

minimal-printf: Fix handling of the two character sequence %% #11729

Uh oh!

Conversation

hugueskamba commented Oct 22, 2019

Description

Pull request type

Reviewers

Release Notes

Uh oh!

ciarmcom commented Oct 22, 2019

Uh oh!

kjbracey Oct 23, 2019

Choose a reason for hiding this comment

Uh oh!

hugueskamba Oct 23, 2019

Choose a reason for hiding this comment

Uh oh!

kjbracey Oct 23, 2019

Choose a reason for hiding this comment

Uh oh!

hugueskamba Oct 23, 2019

Choose a reason for hiding this comment

Uh oh!

kjbracey Oct 23, 2019

Choose a reason for hiding this comment

Uh oh!

evedon Oct 23, 2019

Choose a reason for hiding this comment

Uh oh!

evedon left a comment

Choose a reason for hiding this comment

Uh oh!

hugueskamba commented Oct 23, 2019

Uh oh!

mbed-ci commented Oct 23, 2019

Test run: FAILED

Uh oh!

0xc0170 commented Oct 23, 2019

Uh oh!

Uh oh!