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

Add minimal printf #11051

Merged
merged 56 commits into from Aug 9, 2019

Conversation

@evedon
Copy link
Contributor

commented Jul 15, 2019

Description

This PR imports https://github.com/ARMmbed/minimal-printf into mbed-os.
To replace the standard implementation of the printf functions with the ones from the minimal-printf library, the custom minimal-printf profile should be used when compiling with mbed-cli. For example:

 $ mbed compile -t <toolchain> -m <target> --profile release --profile minimal-printf

Minimal-printf supports floating point and stream printing by default. It has the following restrictions:

  • All floating points are treated as %f.
  • No support for inf, infinity or nan
  • All flags and precision modifiers are ignored.

Pelion Device Management Client example compiled with GCC_ARM, release profile compared to current master (with std printf):

Total Static RAM memory (data + bss): 57712(+0) bytes
Total Flash memory (text + data): 370186(-17490) bytes

Pull request type

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

Reviewers

@bulislaw @hugueskamba

Release Notes

To replace the standard implementation of the printf functions with the ones from the minimal-printf library, the custom minimal-printf profile should be used when compiling with mbed-cli. For example:

 $ mbed compile -t <toolchain> -m <target> --profile release --profile minimal-printf
marcuschangarm and others added 30 commits Oct 15, 2017
 Supports %d %u %X %p %s
 Doesn't use malloc
Use mbed_app.json to enable floating points.
Automatically ignore flags and width specifiers.
This makes it possible to use the minimal printf and snprintf
through mbed_retarget.cpp
Add support for width specifiers
This is used by mbed-os-linker-report and doesn't increase the code
size.
Add "-g" to 'release' and 'develop' profiles
ssize_t doesn't exist at all in armcc.
Remove references to ssize_t in the code
This commit adds mostly integer (and buffer) overflow checks for the
current buffer index (`result` variable).
- Improved comments to explain the checks on 'result'.
- Check for non-NULL format specifier.
[BUGFIX][IOTUC-18] Library fixes
The minimal-printf implementation supports a number of length modifiers
(j, z and t) that are not supported by the native mbed OS libc
implementation. The compliance test has tests for these modifiers, which
means that it isn't possible to check the output of mbed-printf against
a known good implementation (libc's printf) when running on mbed OS.
This, in turn, can give the impression that the tests for these
modifiers pass, when that might not be the case. To address this issue,
this PR removes the tests for these modifiers in mbed OS.

This PR was created because some of the tests for these modifiers
actually fail in Linux, for example:

```
>>> Running case #3: 'printf %u'...
hhu: 0
hhu: 0
hhu: 255
hhu: 255
hu: 0
hu: 0
hu: 65535
hu: 65535
u: 0
u: 0
u: 4294967295
u: 4294967295
lu: 0
lu: 0
lu: 4294967295
lu: 4294967295
llu: 0
llu: 0
llu: 18446744073709551615
llu: 18446744073709551615
ju: 0
ju: 0
ju: 4294967295
ju: 18446744073709551615
:188::FAIL: Expected 7 Was 16
>>> 'printf %u': 0 passed, 1 failed with reason 'Assertion Failed'
```
In the implementation, don't always display double hex digits when
printing with "%X". This is in line with the behaviour observed both
in mbed OS's printf (Newlib) and Linux's printf (glibc).
In the tests, always compare the baseline result with the result
returned by the minimal printf implementation, instead of comparing
with a constant value.
The printf(3) man page says "The functions snprintf() and vsnprintf() do
not write more than size bytes (including the terminating null byte
('\0')).  If the output was truncated due to this limit, then the return
value is the number of characters (excluding the terminating null byte)
which would have been written to the final string if enough space had been
available." The last part of this spec (returning the number of
characters which would have been written to the string if enough space
had been available) was not implemented in minimal-printf. This PR
changes that by redirecting all the processed characters through the
"minimal_printf_putchar" helper function. This function will not write
to the buffer if there's no space left, but it will always increment the
number of written characters, in order to match the above description.
minimal_printf_putchar also contains checks for overflows, so these
checks are no longer needed in the rest of the code.

Other changes:

- In some cases, mbed_minimal_formatted_string didn't put the string
  terminator in the output buffer. This PR ensures that this always
  happens.
- Fixed a bug in printed hexadecimal numbers introduced by a previous
  commit.
- Added buffer overflow tests for snprintf.
Implementation and test fixes
Change the default output from STDIO UART to SWO by overriding:

    "minimal-printf.console-output": "SWO"
@evedon

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2019

This PR needs cleanup for the Git history as it seems to contain many merge commits and therefore looks like has more changes than what it actually has.

Please remove the merge commits and rebase your work on top of master. It makes history and review much cleaner. Thanks.

I have squashed all the commits done for the integration in mbed-os and review comments into the last two commits. All previous commits come from the original library and it would beb useful to keep them.

All comments have now been addressed.
@bulislaw could you review?

@evedon

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

commented Aug 1, 2019

Test run: FAILED

Summary: 3 of 4 test jobs failed
Build number : 4
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-IAR
  • jenkins-ci/mbed-os-ci_build-GCC_ARM
  • jenkins-ci/mbed-os-ci_build-ARM
@mbed-ci

This comment has been minimized.

Copy link

commented Aug 1, 2019

Test run: FAILED

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

Failed test jobs:

  • jenkins-ci/mbed-os-ci_exporter
@evedon evedon added needs: CI and removed needs: review labels Aug 2, 2019
@mbed-ci

This comment has been minimized.

Copy link

commented Aug 2, 2019

Test run: SUCCESS

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

@evedon

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2019

Run 5 seemed to have failed due to a license issue

19:25:57  [2019-08-01T18:25:57.065Z] + grep Unable to connect to the license server ../exporter_build_log_NUCLEO_L073RZ_gnuarmeclipse.log
[Pipeline] echo
19:25:57  [2019-08-01T18:25:57.079Z] Throwing the error

Job restarted and now successful.

@evedon evedon added ready for merge and removed needs: CI labels Aug 2, 2019
@evedon

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2019

@SeppoTakalo Are you happy with the commit history from the original library + three extra commits for this PR?

@mbed-ci

This comment has been minimized.

Copy link

commented Aug 5, 2019

Test run: SUCCESS

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

Copy link
Contributor

left a comment

LGTM but optimization of the arm toolchain should improved in the future as per #11051 (comment).

@evedon

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2019

Any review from @ARMmbed/mbed-os-tools ?

@evedon

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2019

Any review from @ARMmbed/mbed-os-tools ?

The part of this PR which affects tools has already been reviewed (and modified) by @madchutney.

PR is ready to go.

@0xc0170
0xc0170 approved these changes Aug 9, 2019
@0xc0170 0xc0170 merged commit fafd0a5 into ARMmbed:master Aug 9, 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 8673 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 8448B.
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
@0xc0170 0xc0170 changed the title Minimal printf Add minimal printf Aug 9, 2019
@0xc0170 0xc0170 removed the ready for merge label Aug 9, 2019
@kjbracey-arm

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2019

I see that there can be some ambiguity and possibly wrong behaviour between the retarget code for the console and minimal-printf. So if I understand your suggestion correctly, the next step would be to compile out the retarget code if minimal-printf is used.

My issue is that this conflates two independent things - "minimal printf" and "simplified stdin/out retargetting", and attempts to short-circuit the retargetting in printf itself, without actually removing it.

This PR fails with a very simple test program:

int main() {
    printf("2+2=%d\n", 2+2);
    puts("done");
}

The minimal printf should not be trying to work around the retargetting - it should just use fputc.

If you want to simplify the retargetting, do it properly in a separate PR - that could make it so that fputc() always wrote to the serial port.

And the two options - simplified printf and simplified retargetting should be totally independent of each other. There's no reason to link them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.