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 configuration for IO flushing during exit. #2741

Merged
merged 1 commit into from Sep 28, 2016

Conversation

Projects
None yet
4 participants
@pan-
Member

pan- commented Sep 19, 2016

Description

This PR allows application to enable or disable the flush of standard I/O's when exit is called.
This change can save a lot of space for applications which don't use standard stream because even if NDEBUG is enabled, a lot of the I/O subsystem is stilled pulled when the exit function invoke fflush over standard streams.

By default, the streams are still flushed but the user can override this behavior in the configuration file:

{
        "target_overrides": {
                "*": {
                        "core.stdio-flush-at-exit": false
                }
        }
}

Related PRs

With #2715, this PR allows user to completely remove the IO subsystem from their application.

Gain:

The test have been conducted upon mbed-os-example-blinky compiled with NDEBUG enabled.

  • Without the patch:
GCC ARMCC IAR
RAM 11832 10152 7970
ROM 37008 26868 21266
  • With this patch and #2715:
GCC ARMCC IAR
RAM 11288 bytes 8020 bytes 7350 bytes
ROM 16640 bytes 12686 bytes 12398 bytes
  • Difference:
GCC ARMCC IAR
RAM -544 bytes (4.6%) -2132 bytes (-21%) -620 bytes (7.8%)
ROM -20368 bytes (-55%) -14686 bytes (-52.8%) -8868 bytes (-41.7%)
Makes flush of IOs at exit configurable.
This change allows program which doesn't use the IO subsystem to
completelly get rid of it in the binary generated.

IO's are still flushed by default but it can be overriden in
configuration.
@pan-

This comment has been minimized.

Show comment
Hide comment
@pan-

pan- Sep 19, 2016

Member

@0xc0170 @sg- Could you review this PR ?

Member

pan- commented Sep 19, 2016

@0xc0170 @sg- Could you review this PR ?

@0xc0170

This comment has been minimized.

Show comment
Hide comment
@0xc0170

0xc0170 Sep 20, 2016

Member

The footprint results and preserving the current behavior +1.

error() function shouldn't call abort() ? If it does, then this config is not needed, as when assert/error happens, abort is called, and it's not expected to flush streams. A caller (mbed_error() in this case) would flush buffers that uses. However abort() would need to be defined, currently is not (I assume that's why exit is invoked).

What do you think ?

Member

0xc0170 commented Sep 20, 2016

The footprint results and preserving the current behavior +1.

error() function shouldn't call abort() ? If it does, then this config is not needed, as when assert/error happens, abort is called, and it's not expected to flush streams. A caller (mbed_error() in this case) would flush buffers that uses. However abort() would need to be defined, currently is not (I assume that's why exit is invoked).

What do you think ?

@sg-

This comment has been minimized.

Show comment
Hide comment
@sg-

sg- Sep 20, 2016

Member

I think using the assert() -> abort() makes sense not flushing the streams and reserving error() and flushing the streams for non optimized builds which can be guarded by NDEBUG for the size reduction

@0xc0170 is this what you were suggesting?

Member

sg- commented Sep 20, 2016

I think using the assert() -> abort() makes sense not flushing the streams and reserving error() and flushing the streams for non optimized builds which can be guarded by NDEBUG for the size reduction

@0xc0170 is this what you were suggesting?

@0xc0170

This comment has been minimized.

Show comment
Hide comment
@0xc0170

0xc0170 Sep 21, 2016

Member

I think using the assert() -> abort() makes sense not flushing the streams and reserving error() and flushing the streams for non optimized builds which can be guarded by NDEBUG for the size reduction

Yes, assert() -> abort(), and I would apply the same for error(). As both are error conditions (should not happen), and shall invoke abort. However, this might be considered breaking, we could flush stderr (the stream that is used in mbed_error, I believe it's stderr) prior calling abort (the other pull request protects that via NDEBUG, would be active only for debug builds). The result would be the almost the same as you described.

exit() however would stay as it is this change-set would not be needed anymore.

@pan- Does that sound feasible?

Member

0xc0170 commented Sep 21, 2016

I think using the assert() -> abort() makes sense not flushing the streams and reserving error() and flushing the streams for non optimized builds which can be guarded by NDEBUG for the size reduction

Yes, assert() -> abort(), and I would apply the same for error(). As both are error conditions (should not happen), and shall invoke abort. However, this might be considered breaking, we could flush stderr (the stream that is used in mbed_error, I believe it's stderr) prior calling abort (the other pull request protects that via NDEBUG, would be active only for debug builds). The result would be the almost the same as you described.

exit() however would stay as it is this change-set would not be needed anymore.

@pan- Does that sound feasible?

@pan-

This comment has been minimized.

Show comment
Hide comment
@pan-

pan- Sep 21, 2016

Member

@sg- @0xc0170 assert() -> abort() makes a lot of sense, it is the behavior defined by the C and C++ standard!

I still see two issues:

  • _exit: This is the routine we override for newlib, it flush IO but it shouldn't. This routine is the stub called directly by abort or the one called after at_exit processing and IO flushing by exit.
  • exit is still referenced by the c runtime (crt0.s).
Member

pan- commented Sep 21, 2016

@sg- @0xc0170 assert() -> abort() makes a lot of sense, it is the behavior defined by the C and C++ standard!

I still see two issues:

  • _exit: This is the routine we override for newlib, it flush IO but it shouldn't. This routine is the stub called directly by abort or the one called after at_exit processing and IO flushing by exit.
  • exit is still referenced by the c runtime (crt0.s).
@sg-

This comment has been minimized.

Show comment
Hide comment
@sg-

sg- Sep 22, 2016

Member

_exit: This is the routine we override for newlib, it flush IO but it shouldn't. This routine is the stub called directly by abort or the one called after at_exit processing and IO flushing by exit.

That seems like incorrect behavior and should be an easy fix

exit is still referenced by the c runtime (crt0.s)

given this is intended for the return value of main and even if main returns there is no where to go it seems we could patch this to use _exit or is that nonsense?

Member

sg- commented Sep 22, 2016

_exit: This is the routine we override for newlib, it flush IO but it shouldn't. This routine is the stub called directly by abort or the one called after at_exit processing and IO flushing by exit.

That seems like incorrect behavior and should be an easy fix

exit is still referenced by the c runtime (crt0.s)

given this is intended for the return value of main and even if main returns there is no where to go it seems we could patch this to use _exit or is that nonsense?

@0xc0170

This comment has been minimized.

Show comment
Hide comment
@0xc0170

0xc0170 Sep 23, 2016

Member

@pan- Let's resolve this one today (those 2 issues remaining about _exit, exit), I'll talk to you

Member

0xc0170 commented Sep 23, 2016

@pan- Let's resolve this one today (those 2 issues remaining about _exit, exit), I'll talk to you

@0xc0170

This comment has been minimized.

Show comment
Hide comment
@0xc0170

0xc0170 Sep 26, 2016

Member

To summarise the discussion with @pan- :

we overwrite _exit for gcc (it's called via exit()). If we remove flushing stdout/err, its not valid for other toolchains (quick look at IAR, app should overwrite exit, they don't provide similar hook as gcc does ,thus we use exit for it). I think this is the way to go for now to have this flushing configurable via config.

The above issues about assert and abort shall be handled separately.

@sg- What do you think?

Member

0xc0170 commented Sep 26, 2016

To summarise the discussion with @pan- :

we overwrite _exit for gcc (it's called via exit()). If we remove flushing stdout/err, its not valid for other toolchains (quick look at IAR, app should overwrite exit, they don't provide similar hook as gcc does ,thus we use exit for it). I think this is the way to go for now to have this flushing configurable via config.

The above issues about assert and abort shall be handled separately.

@sg- What do you think?

@sg-

This comment has been minimized.

Show comment
Hide comment
@sg-

sg- Sep 27, 2016

Member

/morph test

Member

sg- commented Sep 27, 2016

/morph test

@sg- sg- added needs: CI and removed needs: review labels Sep 27, 2016

@sg-

This comment has been minimized.

Show comment
Hide comment
@sg-

sg- Sep 27, 2016

Member

@mbed-bot: TEST

HOST_OSES=ALL
BUILD_TOOLCHAINS=ALL
TARGETS=ALL

Member

sg- commented Sep 27, 2016

@mbed-bot: TEST

HOST_OSES=ALL
BUILD_TOOLCHAINS=ALL
TARGETS=ALL

@mbed-bot

This comment has been minimized.

Show comment
Hide comment
@mbed-bot

mbed-bot Sep 27, 2016

Result: SUCCESS

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

/morph test

Output

mbed Build Number: 962

All builds and test passed!

mbed-bot commented Sep 27, 2016

Result: SUCCESS

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

/morph test

Output

mbed Build Number: 962

All builds and test passed!

@mbed-bot

This comment has been minimized.

Show comment
Hide comment
@mbed-bot

mbed-bot Sep 27, 2016

[Build 986]
SUCCESS: Building succeeded and tests were run! Be sure to check the test results

mbed-bot commented Sep 27, 2016

[Build 986]
SUCCESS: Building succeeded and tests were run! Be sure to check the test results

@sg- sg- added ready for merge and removed needs: CI labels Sep 27, 2016

@sg- sg- merged commit 50c30d3 into ARMmbed:master Sep 28, 2016

6 of 10 checks passed

Cambridge CI ${profile_id} Failed
Details
Cambridge CI mbed-os_uvisor_profile_0 Something went wrong. Investigate! No test results found.
Details
Cambridge CI mbed-os_uvisor_profile_1 Something went wrong. Investigate! No test results found.
Details
continuous-integration/jenkins/pr-merge This commit is being built
Details
Cambridge CI mbed-os_uvisor_profile_2 All is well No test results found.
Details
Cambridge CI mbed-os_uvisor_profile_3 All is well No test results found.
Details
ci/morph-test Job has started
Details
continuous-integration/mbedci Merged build finished. No test results found.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
mbed-os CI Checks Build finished.
Details

pan- added a commit to pan-/mbed that referenced this pull request Oct 21, 2016

@pan- pan- deleted the pan-:conf_flush_at_exit branch Jul 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment