Remove debug links to printf/exit in NDEBUG builds #3881

geky · 2017-03-03T17:34:20Z

Allows development of small applications where stdio is avoided, also optimizes out debug strings and format arguments.

~~Biggest change is that exit is no longer called. Not sure if that's important (destructors?).~~

Another change to note is that the linkage of error is now the same as debug (always inlined). This is important to optimize out error strings, but removes the ability for applications to substitute in their own error function.

EDIT: Only debug is optimized out now. As @pan- pointed out, error can be removed by redefining the function in user code. There's no real benefit to optimizing out error except for consistency, but that comes at the cost of preventing users from redefining the error function. An update to the error function can come at a later date if we find it necessary.

Bare main before (ARM, K64F):

+--------------------------+-------+-------+-------+
| Module                   | .text | .data |  .bss |
+--------------------------+-------+-------+-------+
| Misc                     |  5062 |    16 |  1372 |
| drivers                  |   468 |     4 |    28 |
| features/filesystem      |   366 |     0 |     0 |
| features/storage         |    30 |     0 |   184 |
| hal                      |   450 |     8 |     0 |
| platform                 |  1265 |    16 |   352 |
| rtos                     |   128 |     8 |     0 |
| rtos/rtx                 |  5886 |   100 |  8396 |
| targets/TARGET_Freescale |  5928 |   100 |   324 |
| Subtotals                | 19583 |   252 | 10656 |
+--------------------------+-------+-------+-------+

Bare main after (ARM, K64F):

+--------------------------+-------+-------+------+
| Module                   | .text | .data | .bss |
+--------------------------+-------+-------+------+
| Misc                     |   348 |     0 | 1024 |
| features/storage         |    30 |     0 |  184 |
| hal                      |    48 |     0 |    0 |
| platform                 |    24 |     8 |    0 |
| rtos                     |    32 |     8 |    0 |
| rtos/rtx                 |  5704 |   100 | 6764 |
| targets/TARGET_Freescale |  3344 |    88 |  112 |
| Subtotals                |  9530 |   204 | 8084 |
+--------------------------+-------+-------+------+

cc @pan-, @0xc0170

pan- · 2017-03-06T10:58:20Z

@geky by calling exit, error was indirectly calling mbed_die as indicated in the function documentation, replacing exit by an infinite loop break this behavior.
The documentation of error also not state that no message will be outputted if NDEBUG is defined.

I'm favorable to such change but they are breaking changes for users and should be documented accordingly.

0xc0170 · 2017-03-06T11:37:05Z

platform/mbed_error.h

+    mbed_error_vfprintf(format, arg);
+    va_end(arg);
+#endif
+    while (1) {}


This instead of exit() - reason? I cant find it in the commit message.

We do not care what exit() does? just a trap here?

exit flush the file standard IOs. Because exit reference them, a lot of code from the IO subsystem is pulled in even if IOs function are not used directly in the actual program.

I've made a PR 6 month ago which address this issue: #2741 .
It allows user to disable flush of standard IOs in mbed_app.json.

Let me know if there is something better to do here, should I just call mbed_die?

Even with the change in #2741, the current definition of error is still preventing the format strings/arguments from being garbage collected by this compiler. This is ~1.5Kbytes of flash and grows with the number of calls to error.

mbed_die still pulls in ~2Kbytes into the base image (for gpio to flash leds?).

I've changed error to still call exit in develop/release so streams are at least flushed while developing. This is a change between develop/debug, but it may be a worthwhile tradeoff for the space savings in the general case. Thoughts?

0xc0170 · 2017-03-06T11:38:11Z

platform/mbed_debug.h


 /** Output a debug message
 *
 * @param format printf-style format string, followed by variables
 */
 static inline void debug(const char *format, ...) {
+#if DEVICE_STDIO_MESSAGES && !defined(NDEBUG)


Just checking - is NDEBUG defined in the default profile = not breaking for mbed 2 ?

NDEBUG is not defined in the default profile, not sure how that affects mbed 2
https://github.com/ARMmbed/mbed-os/blob/master/tools/profiles/develop.json

0xc0170 · 2017-03-06T11:41:06Z

platform/mbed_error.h

@@ -60,7 +67,17 @@
 extern "C" {
 #endif

-void error(const char* format, ...);
+
+static inline void error(const char *format, ...) {


removes the ability for applications to substitute in their own error function.

how much do we gain?

Here's the footprint (additional 1.5k flash) with debug correctly optimized out and @pan-'s workaround for the fflushs in exit. There's no stdio being dragged in, but the format strings/arguments can't be garbage collected by the compiler:

+--------------------------+-------+-------+------+ | Module | .text | .data | .bss | +--------------------------+-------+-------+------+ | Misc | 348 | 0 | 1024 | | features/storage | 30 | 0 | 184 | | hal | 232 | 8 | 0 | | platform | 188 | 16 | 0 | | rtos | 36 | 8 | 0 | | rtos/rtx | 5766 | 100 | 6764 | | targets/TARGET_Freescale | 4712 | 92 | 112 | | Subtotals | 11312 | 224 | 8084 | +--------------------------+-------+-------+------+

Note: The savings difference grow with the more calls to error

geky · 2017-03-06T23:33:59Z

they are breaking changes for users and should be documented accordingly.

Agreed, is there additional documentation we should add besides updating the doxygen for error? (Should this be a part of the next minor release?)

tommikas · 2017-03-08T08:25:04Z

@geky Getting a linking error: OdinWiFiInterface.cpp:(.text._ZN4rtos5QueueIPvLm1EEC2Ev[_ZN4rtos5QueueIPvLm1EEC5Ev]+0x28): undefined reference to 'error'

geky · 2017-03-09T21:12:24Z

@tommikas, thanks for reporting that! I've added a bit of a hacky workaround to provide external linkage to the error function for backwards compatiblity. It is still gced if unused.

0xc0170 · 2017-03-10T10:10:46Z

platform/mbed_error.c

+// Workaround to provide an externally linkable error function for binary
+// compatiblity with precompiled libraries. Will be gced by compiler if
+// not used at link time. Tiny bit hacky.
+#define static extern


I dont think we should allow this in our codebase. If linker is not smart enough, complain there.

Quoting the spec:

2 A translation unit shall not #define or #undef names lexically identical to keywords.

pan- · 2017-03-10T11:31:20Z

@geky I can't reproduce your results on your application which does something.
Here are mine for mbed-os-example-blinky:

"platform.stdio-flush-at-exit": false, NDEBUG defined, mbed-os-5.4.0

+--------------------------+-------+-------+------+
| Module                   | .text | .data | .bss |
+--------------------------+-------+-------+------+
| Misc                     |   406 |     4 | 1024 |
| features/storage         |    30 |     0 |  184 |
| hal                      |   232 |     8 |    0 |
| platform                 |   212 |    16 |    0 |
| rtos                     |    36 |     8 |    0 |
| rtos/rtx                 |  5766 |   100 | 6764 |
| targets/TARGET_Freescale |  4736 |    92 |  112 |
| Subtotals                | 11418 |   228 | 8084 |
+--------------------------+-------+-------+------+
Allocated Heap: unknown
Allocated Stack: unknown
Total Static RAM memory (data + bss): 8312 bytes
Total RAM memory (data + bss + heap + stack): 8312 bytes
Total Flash memory (text + data + misc): 11646 bytes

"platform.stdio-flush-at-exit": false, NDEBUG defined, mbed-os-5.4.0 + this PR

+--------------------------+-------+-------+------+
| Module                   | .text | .data | .bss |
+--------------------------+-------+-------+------+
| Misc                     |   406 |     4 | 1024 |
| features/storage         |    30 |     0 |  184 |
| hal                      |   232 |     8 |    0 |
| platform                 |   106 |     8 |    0 |
| rtos                     |    36 |     8 |    0 |
| rtos/rtx                 |  5760 |   100 | 6764 |
| targets/TARGET_Freescale |  4736 |    92 |  112 |
| Subtotals                | 11306 |   220 | 8084 |
+--------------------------+-------+-------+------+
Allocated Heap: unknown
Allocated Stack: unknown
Total Static RAM memory (data + bss): 8304 bytes
Total RAM memory (data + bss + heap + stack): 8304 bytes
Total Flash memory (text + data + misc): 11526 bytes

If I use an empty main rather than blinky the results are different and match yours:

"platform.stdio-flush-at-exit": false, NDEBUG defined, mbed-os-5.4.0

+--------------------------+-------+-------+------+
| Module                   | .text | .data | .bss |
+--------------------------+-------+-------+------+
| Misc                     |   350 |     0 | 1024 |
| features/storage         |    30 |     0 |  184 |
| hal                      |   232 |     8 |    0 |
| platform                 |   188 |    16 |    0 |
| rtos                     |    36 |     8 |    0 |
| rtos/rtx                 |  5766 |   100 | 6764 |
| targets/TARGET_Freescale |  4712 |    92 |  112 |
| Subtotals                | 11314 |   224 | 8084 |
+--------------------------+-------+-------+------+
Allocated Heap: unknown
Allocated Stack: unknown
Total Static RAM memory (data + bss): 8308 bytes
Total RAM memory (data + bss + heap + stack): 8308 bytes
Total Flash memory (text + data + misc): 11538 bytes

"platform.stdio-flush-at-exit": false, NDEBUG defined, mbed-os-5.4.0 + this PR

################################################################
Bare main - NO FLUSH AT EXIT NDEBUG + patch
+--------------------------+-------+-------+------+
| Module                   | .text | .data | .bss |
+--------------------------+-------+-------+------+
| Misc                     |   350 |     0 | 1024 |
| features/storage         |    30 |     0 |  184 |
| hal                      |    48 |     0 |    0 |
| platform                 |    24 |     8 |    0 |
| rtos                     |    32 |     8 |    0 |
| rtos/rtx                 |  5704 |   100 | 6764 |
| targets/TARGET_Freescale |  3344 |    88 |  112 |
| Subtotals                |  9532 |   204 | 8084 |
+--------------------------+-------+-------+------+
Allocated Heap: unknown
Allocated Stack: unknown
Total Static RAM memory (data + bss): 8288 bytes
Total RAM memory (data + bss + heap + stack): 8288 bytes
Total Flash memory (text + data + misc): 9736 bytes

Same results can be obtained without this patch just by redefining mbed_die as an infinite loop in the application:

+--------------------------+-------+-------+------+
| Module                   | .text | .data | .bss |
+--------------------------+-------+-------+------+
| Misc                     |   352 |     0 | 1024 |
| features/storage         |    30 |     0 |  184 |
| hal                      |    48 |     0 |    0 |
| platform                 |    38 |     8 |    0 |
| rtos                     |    32 |     8 |    0 |
| rtos/rtx                 |  5710 |   100 | 6764 |
| targets/TARGET_Freescale |  3344 |    88 |  112 |
| Subtotals                |  9554 |   204 | 8084 |
+--------------------------+-------+-------+------+
Allocated Heap: unknown
Allocated Stack: unknown
Total Static RAM memory (data + bss): 8288 bytes
Total RAM memory (data + bss + heap + stack): 8288 bytes
Total Flash memory (text + data + misc): 9758 bytes

mbed_die depends on DigitalIO, it is the reason of the 1.5K added if error call exit.

pan-

I would like to keep the NDEBUG exclusion in mbed_debug.h.

I believe the rest of the patch is not needed/desirable:

inlining of error forbid user code to redefine their own error code and might break existing applications.
not calling mbed_die in case of error will not help users which might want visible sign that something went wrong.
The optimization is a side effect of not calling this function.
The system is already flexible enough, mbed_die and error can be redefined in user application.

adbridge · 2017-03-14T14:22:18Z

@geky Can you please address @pan- comments?

geky · 2017-03-16T00:31:18Z

Thanks for waiting for my late response again.

@pan-'s right that we have a workaround for this extra code size mbed_die brings in (if a device manages to not use digital io at all). It looks like we still have the ~30 bytes from format strings, but it doesn't sound like it's worth working around.

The asymmetry between debug and error makes me concerned, but I can't find a technical argument for making them consistent, in which case the compatibility break is not worth it.

I'll limit the NDEBUG behaviour to debug, if we do find a compelling argument for changing the behaviour or error it can always be a separate pr.

pan- · 2017-03-16T11:27:03Z

The asymmetry between debug and error makes me concerned, but I can't find a technical argument for making them consistent, in which case the compatibility break is not worth it.

@geky I must have miss something but what kind of consistency or symmetry do you want for debug and error ?

sg- · 2017-03-22T11:07:51Z

bump @geky

@geky I must have miss something but what kind of consistency or symmetry do you want for debug and error ?

geky · 2017-03-22T15:47:35Z

Sorry about the delay. It just doesn't sit well that debug is an inlined function and error is a weak function. A user may see that they can overload error and try to overload debugging for their own logging. Perhaps debug should be a weak function as well?

Also I removed the error changes, let me know if I missed anything.

sg- · 2017-04-12T21:25:50Z

@pan- Do you approve?

pan- · 2017-04-13T12:21:21Z

rtos/rtx/TARGET_CORTEX_M/RTX_Conf_CM.c

@@ -244,7 +244,7 @@ void os_idle_demon (void) {
 /*----------------------------------------------------------------------------
 *      RTX Errors
 *---------------------------------------------------------------------------*/
-extern void error(const char* format, ...);
+#include "mbed_error.h"


The inclusion should be grouped with others.

Can update 👍

0xc0170 · 2017-04-19T08:17:15Z

retest uvisor

adbridge · 2017-04-20T14:30:40Z

@geky can we please update the PR title and description to be clear about what this PR is now changing and why? Thanks

theotherjimmy · 2017-04-24T15:24:34Z

@geky Have you updated the description?

@pan- Could you review again? your comments seem to have been taken care of.

Allows development of small applications where stdio is avoided

geky · 2017-04-24T15:47:17Z

Should be updated
/morph test

mbed-bot · 2017-04-24T23:55:54Z

Result: SUCCESS

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

/morph test

Output

mbed Build Number: 80

All builds and test passed!

Already updated

theotherjimmy · 2017-04-26T21:41:07Z

@sg- @adbridge Any objections? I'll merge this tomorrow afternoon Austin time.

adbridge · 2017-05-02T11:05:03Z

ok by me

…lwip_broadcast Release mbed OS 5.4.5 and mbed lib v142 Ports for Upcoming Targets Fixes and Changes 4059: [Silicon Labs] Rename targets ARMmbed#4059 4115: Support for Qt Creator Generic project export and associated Makefile ARMmbed#4115 3915: Feature vscode ARMmbed#3915 4205: tests: race test - add not supported for single threaded env ARMmbed#4205 4187: [NCS36510] Reduce default heap size allocated by IAR to 1/4 of RAM ARMmbed#4187 4145: test - add nanostack to examples.json file ARMmbed#4145 4093: Update.py: New feature - update a branch instead of a fork, plus general improvements. ARMmbed#4093 4225: fixed missing device_name for xDot and removed progen ARMmbed#4225 4243: Config: config header file should contain new line ARMmbed#4243 4251: Fix C++11 build error w/ u-blox EVK-ODIN-W2 ARMmbed#4251 4236: STM32 Fixed warning related to __packed redefinition ARMmbed#4236 4224: Add `mbed new .` output to export ARMmbed#4224 4190: LPC4088: Enable LWIP feature ARMmbed#4190 4136: Error when bootloader is specified but does not exist ARMmbed#4136 3881: Remove debug links to printf/exit in NDEBUG builds ARMmbed#3881 4260: Inherit Xadow M0 target from LPC11U35_501 ARMmbed#4260 4249: Add consistent button names across targets ARMmbed#4249 4254: Removed unused variable in TARGET_NXP/lpc17_emac.c ARMmbed#4254

geky added the needs: review label Mar 3, 2017

0xc0170 reviewed Mar 6, 2017

View reviewed changes

geky force-pushed the log-no-io branch from ac6c7b3 to b6e9702 Compare March 7, 2017 22:21

geky force-pushed the log-no-io branch 2 times, most recently from db6b73c to 74ee476 Compare March 9, 2017 21:11

geky force-pushed the log-no-io branch 2 times, most recently from 3f17cca to 0f6945e Compare March 9, 2017 21:17

0xc0170 reviewed Mar 10, 2017

View reviewed changes

pan- requested changes Mar 10, 2017

View reviewed changes

adbridge added the needs: work label Mar 14, 2017

sg- removed the needs: review label Mar 22, 2017

geky force-pushed the log-no-io branch from 0f6945e to 47de3d6 Compare March 22, 2017 15:42

pan- previously requested changes Apr 13, 2017

View reviewed changes

geky force-pushed the log-no-io branch 2 times, most recently from b6e9702 to 27f4bfc Compare April 13, 2017 17:01

sg- added needs: CI and removed needs: work labels Apr 19, 2017

adbridge added needs: work and removed needs: CI labels Apr 20, 2017

geky changed the title ~~Remove links to printf/exit in NDEBUG builds~~ Remove debug links to printf/exit in NDEBUG builds Apr 24, 2017

Removed debug links to printf/exit in NDEBUG builds

cfc223c

Allows development of small applications where stdio is avoided

geky force-pushed the log-no-io branch from 27f4bfc to cfc223c Compare April 24, 2017 15:40

0xc0170 removed the needs: work label Apr 25, 2017

0xc0170 approved these changes Apr 25, 2017

View reviewed changes

0xc0170 added the ready for merge label Apr 25, 2017

theotherjimmy merged commit 7ace0cb into ARMmbed:master May 2, 2017

theotherjimmy added release-version: 5.4.5 and removed ready for merge labels May 2, 2017

Remove debug links to printf/exit in NDEBUG builds #3881

Remove debug links to printf/exit in NDEBUG builds #3881

Uh oh!

Conversation

geky commented Mar 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pan- commented Mar 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geky Mar 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geky Mar 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geky commented Mar 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tommikas commented Mar 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

geky commented Mar 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pan- commented Mar 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

"platform.stdio-flush-at-exit": false, NDEBUG defined, mbed-os-5.4.0

"platform.stdio-flush-at-exit": false, NDEBUG defined, mbed-os-5.4.0 + this PR

"platform.stdio-flush-at-exit": false, NDEBUG defined, mbed-os-5.4.0

"platform.stdio-flush-at-exit": false, NDEBUG defined, mbed-os-5.4.0 + this PR

Uh oh!

pan- left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adbridge commented Mar 14, 2017

Uh oh!

geky commented Mar 16, 2017

Uh oh!

pan- commented Mar 16, 2017

Uh oh!

sg- commented Mar 22, 2017

Uh oh!

geky commented Mar 22, 2017

Uh oh!

sg- commented Apr 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Apr 19, 2017

Uh oh!

adbridge commented Apr 20, 2017

Uh oh!

theotherjimmy commented Apr 24, 2017

Uh oh!

geky commented Apr 24, 2017

Uh oh!

mbed-bot commented Apr 24, 2017

Output

Uh oh!

theotherjimmy commented Apr 26, 2017

geky commented Mar 3, 2017 •

edited

Loading

geky Mar 6, 2017 •

edited

Loading

geky Mar 6, 2017 •

edited

Loading

geky commented Mar 6, 2017 •

edited

Loading

tommikas commented Mar 8, 2017 •

edited

Loading

pan- commented Mar 10, 2017 •

edited

Loading

pan- left a comment •

edited

Loading