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

Remove excessive use of printf/scanf in mbed_fdopen/_open #4831

Merged
merged 4 commits into from Aug 24, 2017

Conversation

Projects
None yet
7 participants
@fahhem
Contributor

fahhem commented Jul 30, 2017

Description

From #4830, implementing Option A. This saves ~3.3kB in firmware that's not using scanf otherwise. It also likely saves a few bytes of stack since buf is 5 bytes smaller.

Status

READY

Migrations

NO

@bulislaw

This comment has been minimized.

Member

bulislaw commented Jul 31, 2017

/morph test

platform/mbed_retarget.cpp Outdated
@@ -231,7 +231,7 @@ extern "C" FILEHANDLE PREFIX(_open)(const char* name, int openmode) {
/* FILENAME: ":0x12345678" describes a FileHandle* */

This comment has been minimized.

@bulislaw

bulislaw Jul 31, 2017

Member

Filename in this comment should be updated.

This comment has been minimized.

@bulislaw

bulislaw Jul 31, 2017

Member

Well actually nevermind me, the file handle is the same.

platform/mbed_retarget.cpp Outdated
@@ -826,8 +826,12 @@ void mbed_set_unbuffered_stream(std::FILE *_file) {
*/
std::FILE *mbed_fdopen(FileHandle *fh, const char *mode)
{
char buf[12]; /* :0x12345678 + null byte */
std::sprintf(buf, ":%p", fh);
char buf[2 + sizeof(fh) + 1]; /* :(pointer) + null byte */

This comment has been minimized.

@bulislaw

bulislaw Jul 31, 2017

Member

Can we add a comment why are we doing that.

@bulislaw

This comment has been minimized.

Member

bulislaw commented Jul 31, 2017

Could you copy build statistics before and after (the sizes) they should be printed after linking.

/morph test

@fahhem

This comment has been minimized.

Contributor

fahhem commented Jul 31, 2017

Where would I get the build statistics? I only have the GCC_ARM toolchain on my system, and I can't seem to connect to jenkins or morph. Do they have the build stats or should I set my system up with the other toolchains?

@bulislaw

This comment has been minimized.

Member

bulislaw commented Jul 31, 2017

When you do mbed compile [...] on your terminal, at the end it prints you the different sizes.

@fahhem

This comment has been minimized.

Contributor

fahhem commented Jul 31, 2017

So, a confession... I don't use mbed-cli... I started off using platformio, but I wanted more control over what/how my code got built so I went with bazel so I could use newer toolchains and organize my code better (especially since I'm creating my own board).

I noticed these size changes by running arm-none-eabi-size on my resultant .elf files, which is likely what mbed-cli does (though I don't see it anywhere in that codebase). Are there any examples that I can build with/without these changes to provide more repeatable results?

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jul 31, 2017

@fahhem I'm glad that other build systems are working for you. You are correct in pointing out that we do not run arm-none-eabi-size as there is no equivalent for the ARMC5 and IAR EWARM toolchains. Instead, we parse the memory map file with the tools/memap.py script. For exactly the same output, you should be able to use that script directly on the arm-none-eabi-gcc memory map file. @bulislaw @geky What examples would you like to see?

@geky

This comment has been minimized.

Member

geky commented Jul 31, 2017

Hello! I like this pr, it looks great 👍

The most useful numbers would be percentage change in text+data+bss (snapshots before and after work), you're right you can get these from arm-none-eabi-size.

You can also run the memap script in mbed OS to get portable numbers:

PYTHONPATH=mbed-os python mbed-os/tools/memap.py -t GCC_ARM path/to/map/file/from/compiler/file.map

You can run the script with --help to see other options.

@geky

This comment has been minimized.

Member

geky commented Jul 31, 2017

Are there any examples that I can build with/without these changes to provide more repeatable results?

Just caught this. We have work in progress for a set of examples that pull in different components of mbed OS combined with better inspection into memory usage, but this isn't ready yet (probably will be around the 5.6 release). The examples we do have already pull in printf willy nilly, so they probably wouldn't show much.

Maybe blinky?

The memory savings you listed in the issue are good enough for me (~3.3kB text), we've been looking at the weight of different modules and have been aware of how heavy printf is.

@theotherjimmy theotherjimmy added needs: CI and removed needs: work labels Jul 31, 2017

@fahhem fahhem force-pushed the fahhem:less_scanf branch to af37520 Jul 31, 2017

@fahhem

This comment has been minimized.

Contributor

fahhem commented Jul 31, 2017

Before:

bazel-bazel_stm32/external/arm_none_eabi_gcc/bin/arm-none-eabi-size bazel-bin/tower_controller/firmware.elf
   text	   data	    bss	    dec	    hex	filename
  29920	    844	   7016	  37780	   9394	bazel-bin/tower_controller/firmware.elf

python mbed-os-5.5.3/tools/memap.py -t GCC_ARM output.map
+-----------+-------+-------+------+
| Module    | .text | .data | .bss |
+-----------+-------+-------+------+
| Fill      |    62 |     6 |   19 |
| Misc      | 29860 |   838 | 6997 |
| Subtotals | 29922 |   844 | 7016 |
+-----------+-------+-------+------+
Allocated Heap: unknown
Allocated Stack: unknown
Total Static RAM memory (data + bss): 7860 bytes
Total RAM memory (data + bss + heap + stack): 7860 bytes
Total Flash memory (text + data + misc): 30766 bytes

After:

arm-none-eabi-size bazel-bin/tower_controller/firmware.elf
   text	   data	    bss	    dec	    hex	filename
  26960	    480	   7016	  34456	   8698	bazel-bin/tower_controller/firmware.elf

python mbed-os-5.5.3/tools/memap.py -t GCC_ARM output.map
+-----------+-------+-------+------+
| Module    | .text | .data | .bss |
+-----------+-------+-------+------+
| Fill      |    54 |     6 |   19 |
| Misc      | 26898 |   474 | 6997 |
| Subtotals | 26952 |   480 | 7016 |
+-----------+-------+-------+------+
Allocated Heap: unknown
Allocated Stack: unknown
Total Static RAM memory (data + bss): 7496 bytes
Total RAM memory (data + bss + heap + stack): 7496 bytes
Total Flash memory (text + data + misc): 27432 bytes

This is after I rebased onto mbed-os/master, but looks like text went down 2,960 bytes, data went down 364 bytes, and the resulting file went down 3,324 bytes (the sum). It's a bit odd, but memap outputs a different subtotal for .text than me, off by 2 and 8...

@bulislaw

This comment has been minimized.

Member

bulislaw commented Aug 1, 2017

Looks good, it's a good day when you can save 3k :) @geky are you happy with the code change?

/morph test

char buf[12]; /* :0x12345678 + null byte */
std::sprintf(buf, ":%p", fh);
// This is to avoid scanf(buf, ":%.4s", fh) and the bloat it brings.
char buf[2 + sizeof(fh) + 1]; /* :(pointer) + null byte */

This comment has been minimized.

@pan-

pan- Aug 1, 2017

Member

The NULL terminator is not required. buf is not a NULL terminated string it is rather a pointer address prefixed with the character :.
I'm also wondering why the buffer have to be 7 byte long. sizeof(char) + sizeof(fh) byte should be sufficient.

This comment has been minimized.

@0xc0170

0xc0170 Aug 7, 2017

Member

@fahhem Can you respond to these questions please?

This comment has been minimized.

@fahhem

fahhem Aug 11, 2017

Contributor

I'm not aware of the internals of stdlib between the call to std::fopen and its call to _open, I'll take @pan-'s word that none of it does something that will read off the end of that buffer. The strcmp calls inside of _open are safe since none of them will read past the : at the start of buf.

@mbed-bot

This comment has been minimized.

mbed-bot commented Aug 1, 2017

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 921

All builds and test passed!

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 7, 2017

LGTM to me besides those 2 questions asked above for the size of buf

Thanks @fahhem for the issue report and fix

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 10, 2017

LGTM to me besides those 2 questions asked above for the size of buf

Any update? We might help then with this PR as it would be good to have it in, let us know

@fahhem fahhem force-pushed the fahhem:less_scanf branch to 88f9078 Aug 11, 2017

@fahhem

This comment has been minimized.

Contributor

fahhem commented Aug 11, 2017

Sorry, wasn't planning on working on this stuff this week, but I've dropped the null termination tentatively. It might cause infinite loops though

@pan-

This comment has been minimized.

Member

pan- commented Aug 11, 2017

Sorry, wasn't planning on working on this stuff this week, but I've dropped the null termination tentatively. It might cause infinite loops though

Why would it cause an infinite loop ? The NULL termination was in my opinion a bad thing for two reasons:

  • It is not a string. It is a pair composed of a character and a packed pointer.
  • It is not possible to trust the NULL termination character. One or more octet of the pointer can be equal to 0.
@fahhem

This comment has been minimized.

Contributor

fahhem commented Aug 11, 2017

If anything inside of stdlib does a strlen() or similar call, it will cause problems. 'Infinite' was an exaggeration, but it would at best take a long time, at worst cause a hard fault (at least on my platform).

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 14, 2017

@pan- All good?

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Aug 21, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Aug 22, 2017

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1069

All builds and test passed!

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Aug 22, 2017

@theotherjimmy theotherjimmy merged commit dd0a0fc into ARMmbed:master Aug 24, 2017

4 checks passed

Cam-CI uvisor Build & Test Success
Details
ci/morph-test Job has completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@fahhem fahhem deleted the fahhem:less_scanf branch Aug 26, 2017

exmachina-auto-deployer pushed a commit to exmachina-dev/mbed-os that referenced this pull request Sep 13, 2017

Merge tag 'mbed-os-5.5.6' of github.com:ARMmbed/mbed-os into 5.5.6
mbed OS 5.5.6 release

We are pleased to announce the [mbed OS 5.5.6
release](https://github.com/ARMmbed/mbed-os/releases/tag/mbed-os-5.5.6)
is now available.

This release includes ...

Known Issues

The following list of known issues apply to this release:

Contents

Ports for Upcoming Targets

[4608](ARMmbed#4608)
Support Nuvoton's new target NUMAKER_PFM_M487

[4840](ARMmbed#4840)
Add Support for TOSHIBA TMPM066 board

Fixes and Changes

[4801](ARMmbed#4801)
STM32 CAN: Fix issue with speed function calculation

[4808](ARMmbed#4808)
Make HAL & US tickers idle safe

[4812](ARMmbed#4812)
Use DSPI SDK driver API's in SPI HAL driver

[4832](ARMmbed#4832)
NUC472/M453: Fix several startup and hal bugs

[4842](ARMmbed#4842)
Add call to DAC_Enable as this is no longer done as part

[4849](ARMmbed#4849)
Allow using of malloc() for reserving the Nanostack's heap.

[4850](ARMmbed#4850)
Add list of defines to vscode exporter

[4863](ARMmbed#4863)
Optimize memory usage of wifi scan for REALTEK_RTW8195AM

[4869](ARMmbed#4869)
HAL LPCs SPI: Fix mask bits for SPI clock rate

[4873](ARMmbed#4873)
Fix Cortex-A cache file

[4878](ARMmbed#4878)
STM32 : Separate internal ADC channels with new pinmap

[4392](ARMmbed#4392)
Enhance memap, and configure depth level

[4895](ARMmbed#4895)
Turn on doxygen for DEVICE_* features

[4817](ARMmbed#4817)
Move RTX error handlers into RTX handler file

[4902](ARMmbed#4902)
Using CMSIS/RTX Exclusive access macro

[4923](ARMmbed#4923)
fix export static_files to zip

[4844](ARMmbed#4844)
bd: Add ProfilingBlockDevice for measuring higher-level applications

[4896](ARMmbed#4896)
target BLUEPILL_F103C8 compile fix

[4921](ARMmbed#4921)
Update gcc-arm-embedded PPA in Travis

[4926](ARMmbed#4926)
STM32L053x8: Refactor NUCLEO_L053R8 and DISCO_L053C8 targets

[4831](ARMmbed#4831)
Remove excessive use of printf/scanf in mbed_fdopen/_open

[4922](ARMmbed#4922)
bug fix: xdot clock config

[4935](ARMmbed#4935)
STM32: fix F410RB vectors size

[4940](ARMmbed#4940)
Update mbed-coap to version 4.0.9

[4941](ARMmbed#4941)
Update of MemoryPool.h header file.

You can fetch this release from the [mbed-os
GitHub](https://github.com/ARMmbed/mbed-os) repository,
using the tag "mbed-os-5.5.6".

Please feel free to ask any questions or provide feedback on this
release [on the forum](https://forums.mbed.com/),
or to contact us at [support@mbed.org](mailto:support@mbed.org).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment