Skip to content
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.

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

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

Merged

Conversation

@hugueskamba
Copy link
Contributor

hugueskamba commented Oct 22, 2019

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.

  1. 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
  1. 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 ciarmcom requested review from evedon, kjbracey-arm and ARMmbed/mbed-os-maintainers Oct 22, 2019
@ciarmcom

This comment has been minimized.

Copy link
Member

ciarmcom commented Oct 22, 2019

@hugueskamba hugueskamba force-pushed the hugueskamba:hk-fix-minimal-printf-percentage-printing branch from 7297f37 to 8e89d99 Oct 23, 2019
@@ -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);

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Oct 23, 2019

Contributor

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".
}

This comment has been minimized.

Copy link
@hugueskamba

hugueskamba Oct 23, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Oct 23, 2019

Contributor

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

This comment has been minimized.

Copy link
@hugueskamba

hugueskamba Oct 23, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Oct 23, 2019

Contributor

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);
}

This comment has been minimized.

Copy link
@evedon

evedon Oct 23, 2019

Contributor

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.

@hugueskamba hugueskamba force-pushed the hugueskamba:hk-fix-minimal-printf-percentage-printing branch 2 times, most recently from ad4eca6 to 1ebd043 Oct 23, 2019
@hugueskamba hugueskamba force-pushed the hugueskamba:hk-fix-minimal-printf-percentage-printing branch from 1ebd043 to a29776a Oct 23, 2019
Copy link
Contributor

evedon left a comment

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 hugueskamba force-pushed the hugueskamba:hk-fix-minimal-printf-percentage-printing branch from a29776a to ceffb6d Oct 23, 2019
@hugueskamba

This comment has been minimized.

Copy link
Contributor Author

hugueskamba commented Oct 23, 2019

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

@evedon
Done.

@evedon
evedon approved these changes Oct 23, 2019
@0xc0170 0xc0170 added needs: CI and removed needs: review labels Oct 23, 2019
@mbed-ci

This comment has been minimized.

Copy link

mbed-ci commented Oct 23, 2019

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

This comment has been minimized.

Copy link
Member

0xc0170 commented Oct 23, 2019

Internal CI fault ,restarted exporters

@0xc0170 0xc0170 merged commit 6240335 into ARMmbed:master Oct 23, 2019
26 checks passed
26 checks passed
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Success!
Details
travis-ci/docs Success!
Details
travis-ci/doxy-spellcheck Success!
Details
travis-ci/events Success! Runtime is 8707 cycles.
Details
travis-ci/gitattributestest Success!
Details
travis-ci/include_check Success!
Details
travis-ci/licence_check Success!
Details
travis-ci/littlefs Success! Code size is 8464B.
Details
travis-ci/psa-autogen Success!
Details
travis-ci/tools-py2.7 Success!
Details
travis-ci/tools-py3.5 Success!
Details
travis-ci/tools-py3.6 Success!
Details
travis-ci/tools-py3.7 Success!
Details
@hugueskamba hugueskamba deleted the hugueskamba:hk-fix-minimal-printf-percentage-printing branch Oct 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.