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

fahhem · 2017-07-30T10:27:36Z

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 · 2017-07-31T11:15:29Z

/morph test

bulislaw · 2017-07-31T11:30:46Z

platform/mbed_retarget.cpp

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


Filename in this comment should be updated.

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

bulislaw · 2017-07-31T11:36:58Z

platform/mbed_retarget.cpp

@@ -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 */


Can we add a comment why are we doing that.

bulislaw · 2017-07-31T11:37:53Z

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

/morph test

fahhem · 2017-07-31T13:28:18Z

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 · 2017-07-31T13:29:52Z

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

fahhem · 2017-07-31T15:36:19Z

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 · 2017-07-31T15:39:37Z

@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 · 2017-07-31T15:49:40Z

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 · 2017-07-31T15:59:15Z

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.

fahhem · 2017-07-31T16:38:39Z

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 · 2017-08-01T07:51:06Z

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

/morph test

pan- · 2017-08-01T08:36:00Z

platform/mbed_retarget.cpp

-    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 */


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.

@fahhem Can you respond to these questions please?

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 · 2017-08-01T09:40:09Z

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 · 2017-08-07T13:31:20Z

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

Thanks @fahhem for the issue report and fix

0xc0170 · 2017-08-10T11:48:10Z

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 · 2017-08-11T04:27:23Z

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- · 2017-08-11T07:17:36Z

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 · 2017-08-11T07:58:03Z

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 · 2017-08-14T10:08:38Z

@pan- All good?

theotherjimmy · 2017-08-21T15:35:43Z

/morph test

mbed-bot · 2017-08-22T10:01:47Z

Result: SUCCESS

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

/morph test

Output

mbed Build Number: 1069

All builds and test passed!

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 [[email protected]](mailto:[email protected]).

bulislaw reviewed Jul 31, 2017

View reviewed changes

theotherjimmy added the needs: work label Jul 31, 2017

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

fahhem and others added 3 commits July 31, 2017 09:29

Remove excessive use of printf/scanf in mbed_fdopen/_open

e5ef69a

Update comments for code review

199d108

Use MBED_STATIC_ASSERT instead of static_assert

af37520

fahhem force-pushed the less_scanf branch from 938b56a to af37520 Compare July 31, 2017 16:29

pan- reviewed Aug 1, 2017

View reviewed changes

theotherjimmy added needs: review and removed needs: CI labels Aug 7, 2017

Remove null byte at the end of fdopen's faux filename

88f9078

fahhem force-pushed the less_scanf branch from 8da5b1c to 88f9078 Compare August 11, 2017 04:22

0xc0170 approved these changes Aug 14, 2017

View reviewed changes

0xc0170 added ready for merge and removed needs: review labels Aug 21, 2017

theotherjimmy added needs: CI and removed ready for merge labels Aug 21, 2017

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

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

theotherjimmy added release-version: 5.5.6 and removed ready for merge labels Aug 24, 2017

fahhem deleted the less_scanf branch August 26, 2017 21:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

fahhem commented Jul 30, 2017

bulislaw commented Jul 31, 2017

bulislaw Jul 31, 2017 •

edited

Loading

bulislaw Jul 31, 2017

bulislaw Jul 31, 2017

bulislaw commented Jul 31, 2017

fahhem commented Jul 31, 2017

bulislaw commented Jul 31, 2017

fahhem commented Jul 31, 2017

theotherjimmy commented Jul 31, 2017

geky commented Jul 31, 2017

geky commented Jul 31, 2017

fahhem commented Jul 31, 2017

bulislaw commented Aug 1, 2017

pan- Aug 1, 2017 •

edited

Loading

0xc0170 Aug 7, 2017

fahhem Aug 11, 2017

mbed-bot commented Aug 1, 2017

0xc0170 commented Aug 7, 2017 •

edited

Loading

0xc0170 commented Aug 10, 2017

fahhem commented Aug 11, 2017

pan- commented Aug 11, 2017

fahhem commented Aug 11, 2017

0xc0170 commented Aug 14, 2017

theotherjimmy commented Aug 21, 2017

mbed-bot commented Aug 22, 2017

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

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

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

Conversation

fahhem commented Jul 30, 2017

Description

Status

Migrations

bulislaw commented Jul 31, 2017

bulislaw Jul 31, 2017 • edited Loading

Choose a reason for hiding this comment

bulislaw Jul 31, 2017

Choose a reason for hiding this comment

bulislaw Jul 31, 2017

Choose a reason for hiding this comment

bulislaw commented Jul 31, 2017

fahhem commented Jul 31, 2017

bulislaw commented Jul 31, 2017

fahhem commented Jul 31, 2017

theotherjimmy commented Jul 31, 2017

geky commented Jul 31, 2017

geky commented Jul 31, 2017

fahhem commented Jul 31, 2017

bulislaw commented Aug 1, 2017

pan- Aug 1, 2017 • edited Loading

Choose a reason for hiding this comment

0xc0170 Aug 7, 2017

Choose a reason for hiding this comment

fahhem Aug 11, 2017

Choose a reason for hiding this comment

mbed-bot commented Aug 1, 2017

Result: SUCCESS

Output

0xc0170 commented Aug 7, 2017 • edited Loading

0xc0170 commented Aug 10, 2017

fahhem commented Aug 11, 2017

pan- commented Aug 11, 2017

fahhem commented Aug 11, 2017

0xc0170 commented Aug 14, 2017

theotherjimmy commented Aug 21, 2017

mbed-bot commented Aug 22, 2017

Result: SUCCESS

Output

bulislaw Jul 31, 2017 •

edited

Loading

pan- Aug 1, 2017 •

edited

Loading

0xc0170 commented Aug 7, 2017 •

edited

Loading