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 debug links to printf/exit in NDEBUG builds #3881

Merged
merged 1 commit into from May 2, 2017

Conversation

Projects
None yet
8 participants
@geky
Member

geky commented Mar 3, 2017

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

@geky geky added the needs: review label Mar 3, 2017

@pan-

This comment has been minimized.

Member

pan- commented Mar 6, 2017

@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.

platform/mbed_error.h Outdated
mbed_error_vfprintf(format, arg);
va_end(arg);
#endif
while (1) {}

This comment has been minimized.

@0xc0170

0xc0170 Mar 6, 2017

Member

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

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

This comment has been minimized.

@pan-

pan- Mar 6, 2017

Member

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.

This comment has been minimized.

@geky

geky Mar 6, 2017

Member

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.

This comment has been minimized.

@geky

geky Mar 9, 2017

Member

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?

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

This comment has been minimized.

@0xc0170

0xc0170 Mar 6, 2017

Member

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

This comment has been minimized.

@geky

geky Mar 6, 2017

Member

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

platform/mbed_error.h Outdated
@@ -60,7 +67,17 @@
extern "C" {
#endif
void error(const char* format, ...);
static inline void error(const char *format, ...) {

This comment has been minimized.

@0xc0170

0xc0170 Mar 6, 2017

Member

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

how much do we gain?

This comment has been minimized.

@geky

geky Mar 6, 2017

Member

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

This comment has been minimized.

Member

geky commented Mar 6, 2017

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?)

@geky geky force-pushed the geky:log-no-io branch Mar 7, 2017

@tommikas

This comment has been minimized.

Contributor

tommikas commented Mar 8, 2017

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

@geky geky force-pushed the geky:log-no-io branch 2 times, most recently Mar 9, 2017

@geky

This comment has been minimized.

Member

geky commented Mar 9, 2017

@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.

@geky geky force-pushed the geky:log-no-io branch 2 times, most recently Mar 9, 2017

platform/mbed_error.c Outdated
// 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

This comment has been minimized.

@0xc0170

0xc0170 Mar 10, 2017

Member

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

This comment has been minimized.

@pan-

pan- Mar 10, 2017

Member

Quoting the spec:

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

@pan-

This comment has been minimized.

Member

pan- commented Mar 10, 2017

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

pan- requested changes Mar 10, 2017 edited

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

This comment has been minimized.

Contributor

adbridge commented Mar 14, 2017

@geky Can you please address @pan- comments?

@geky

This comment has been minimized.

Member

geky commented Mar 16, 2017

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-

This comment has been minimized.

Member

pan- commented Mar 16, 2017

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-

This comment has been minimized.

Member

sg- commented Mar 22, 2017

bump @geky

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

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

@geky geky force-pushed the geky:log-no-io branch Mar 22, 2017

@geky

This comment has been minimized.

Member

geky commented Mar 22, 2017

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-

This comment has been minimized.

Member

sg- commented Apr 12, 2017

@pan- Do you approve?

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

This comment has been minimized.

@pan-

pan- Apr 13, 2017

Member

The inclusion should be grouped with others.

This comment has been minimized.

@geky

geky Apr 13, 2017

Member

Can update 👍

@geky geky force-pushed the geky:log-no-io branch 2 times, most recently Apr 13, 2017

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

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 19, 2017

retest uvisor

@adbridge

This comment has been minimized.

Contributor

adbridge commented Apr 20, 2017

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

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

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Apr 24, 2017

@geky Have you updated the description?

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

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

Removed debug links to printf/exit in NDEBUG builds
Allows development of small applications where stdio is avoided

@geky geky force-pushed the geky:log-no-io branch to cfc223c Apr 24, 2017

@geky

This comment has been minimized.

Member

geky commented Apr 24, 2017

Should be updated
/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Apr 24, 2017

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

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

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Apr 26, 2017

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

@adbridge

This comment has been minimized.

Contributor

adbridge commented May 2, 2017

ok by me

@theotherjimmy theotherjimmy merged commit 7ace0cb into ARMmbed:master May 2, 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

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

Merge tag 'mbed-os-5.4.5' of github.com:ARMmbed/mbed-os into feature/…
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment