Skip to content

replace all c & c++ standard to nano #7604

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

Closed
wants to merge 1 commit into from

Conversation

netanelgonen
Copy link
Contributor

@netanelgonen netanelgonen commented Jul 26, 2018

Description

replace standart c & c++ libraries to the nano impelantation.

currently only for arm gcc

the reduction is

original libc

Total Static RAM memory (data + bss): 78204 bytes
Total Flash memory (text + data): 386128 bytes

with libc_nano

Total Static RAM memory (data + bss): 76124 bytes
Total Flash memory (text + data): 354492 bytes

with nano c and nano cpp

Total Static RAM memory (data + bss): 76140 bytes
Total Flash memory (text + data): 341828 bytes

total: 386128 - 341828 = 44300 bytes saved

Pull request type

[ ] Fix
[X ] Refactor
[ ] New target
[ ] Feature
[ ] Breaking change

@netanelgonen
Copy link
Contributor Author

checking the option for ARM, ARM6 and IAR

@0xc0170
Copy link
Contributor

0xc0170 commented Jul 26, 2018

is this actually same as specifying nano specs? If yes, are you aware of thread safety in these libraries?

Can you provide reference for this change (documentation where this _nano is explained?)

@netanelgonen
Copy link
Contributor Author

Hi @0xc0170 , actually I don't know if this is the same as nano-specs. I ran the mbed-client test and it passed so, not issue should be here.
maybe can we run the mbed-os morph test to see its full impact?
the almost 40K save worth the change and definitely worth the check at least in my opinion.

@alekshex
Copy link
Contributor

If mbed-os tests and client tests will all pass on this PR. This will give a good indication.

@pan-
Copy link
Member

pan- commented Jul 26, 2018

is this actually same as specifying nano specs? If yes, are you aware of thread safety in these libraries?

Yes; it is the dirty way of doing it.

* C Libraries usage *

This toolchain is released with two prebuilt C libraries based on newlib:
one is the standard newlib and the other is newlib-nano for code size.
To distinguish them, we rename the size optimized libraries as:

  libc.a --> libc_nano.a
  libg.a --> libg_nano.a

To use newlib-nano, users should provide additional gcc compile and link time
option:
 --specs=nano.specs

At compile time, a 'newlib.h' header file especially configured for newlib-nano
will be used if --specs=nano.specs is passed to the compiler.

Nano.specs also handles two additional gcc libraries: libstdc++_nano.a and
libsupc++_nano.a, which are optimized for code size.

@0xc0170
Copy link
Contributor

0xc0170 commented Jul 26, 2018

@pan- 👍 Based on that, these 2 libs then are not thread safe (at least from what I recall for GCC ARM v6 that we currently use)

What I would like to reference here #4877 - newlib and make it thread safe - there was PR earlier that needed some work referenced there in the issue. The PR captured details how to make newlib shipped in gcc v6 thread safe - if you look at files, you can find there some changes to our codebase as well - rtx/retarget.

the almost 40K save worth the change and definitely worth the check at least in my opinion.

This would be probably good to chase ! Make newlib thread safe and default for GCC ARM

Update: my initial assumption was wrong. those 3 libs are just compilied for size

@cmonr cmonr requested review from a team July 26, 2018 15:35
@pan-
Copy link
Member

pan- commented Jul 26, 2018

Please note that using nano also has an effect on the I/O formats supported (no C99, no floats if not helped by the linker, ...). The 40K comparison does not compare apples to apples.

Side note: It should be sage to use stdc++_nano and supc++_nano as it is libstdc++ compiled with the option -Os instead of -O2.

@0xc0170
Copy link
Contributor

0xc0170 commented Jul 27, 2018

Nano.specs also handles two additional gcc libraries: libstdc++_nano.a and
libsupc++_nano.a, which are optimized for code size.

OK so that would be the goal of this PR. I would like to check how these are compiled (difference between the regular one). If it's really just optimization or something else we should be aware of.

@netanelgonen Basically, I would like to understand what we gain vs loose with this config change (not just data savings)

Copy link
Contributor

@0xc0170 0xc0170 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nano.specs file achieves almost the identical result than this PR. There is definitely memory saving but what we loose comes with higher cost (we switched to standard newlib).

Newlib specifies specs so we should use them instead of changing arguments.

@theotherjimmy
Copy link
Contributor

@netanelgonen Are you aware that there's a config option for this? It's called target.default_lib and it can be set to std or small. small does nano specs on GCC_ARM and microlib on ARMCC.

@netanelgonen
Copy link
Contributor Author

@theotherjimmy actually I was not aware of that. if this is the same we should use this.
@0xc0170 what is the high cost? the thread safety, if so as you state there is a PR on that, why not just check this and merge it?

@0xc0170
Copy link
Contributor

0xc0170 commented Jul 30, 2018

@theotherjimmy actually I was not aware of that. if this is the same we should use this.

If you use small lib (microlib for ARMCC5, newlib nano for GCC ARM), you get single thread. No thread safety. However, good point. We already have small default lib that is used for some smaller targets.

@0xc0170 what is the high cost? the thread safety, if so as you state there is a PR on that, why not just check this and merge it?

If you read the discussion there, it had some outstanding work. It needs to be picked up and be resolved (=needs work).

@0xc0170
Copy link
Contributor

0xc0170 commented Aug 3, 2018

I am closing this as the discussion should continue in #4877

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants