-
Notifications
You must be signed in to change notification settings - Fork 172
newlib: enable long long support #101
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
Conversation
1b27f5b to
4926990
Compare
|
Ok, so I tested qemu_cortex_m3: (Didn't test qemu_x86 which has MMU and other bloat enabled, making results much less conclusive.) So yeah, it's huge code increase. And as much as dislike bloat, I'd give this patch +0.75. The whole situation is why we have minlibc. Let it continue to be, and let it be the default. And let's continue to carefully craft and optimize it. But we need the other opposite of spectrum - a libc which just allows existing real-world apps to work on Zephyr, without being swamped with patching them. |
Well, I don't know people who'd be strict on unbloated code size and would counter my arguments above. Let me randomly ping @nashif and @mike-scott to spread awareness. |
Posted RFC to the mailing list: https://lists.zephyrproject.org/g/devel/message/6201 |
|
I wanted to see what the current state is across toolchains. Zephyr's SDK is built only with newlib nano, unlike Arm Embedded which builds both standard and nano newlib and allows selection at build time. I ran the sample below under various configurations using the ZTC is Zephyr SDK 0.10.3 Flash and SRAM space and increases relative to 0.10.3 minimal libc:
Relative to 0.10.3 PR101 adds 13304 bytes FLASH and 364 bytes SRAM in a newlib configuration. Also it's clear that PR101 does not enable all the newlib features of Arm Embedded, since that adds another 11 K FLASH and 2.5 K RAM. I don't know what newlib nano has that minimal libc doesn't, but adding an option for newlib nano to the Zephyr SDK might catch a few cases where minimal isn't good enough, but full is too big. Newlib also has a policy for what's in nano, avoiding arguments about any additions. Test ProgramZephyr toolchain 0.10.3, minimal libc:Output:Zephyr toolchain 0.10.3, newlib libc:Output:Zephyr toolchain 0.10.3-PR-101, minimal libc:Output:Zephyr toolchain 0.10.3-PR-101, newlib libc:Output:Arm Embedded 8-2019-q3-update, minimal libcOutput:Arm Embedded 8-2019-q3-update, newlib nano libcOutput:Arm Embedded 8-2019-q3-update, newlib libcOutput: |
|
I don't see anything in upstream newlib about nano being deprecated. Being able to select a libc with no more features than the application needs is a real advantage. The nice thing about nano is that it's supported in the GNU toolchain as a build-time option by overriding the GCC driver behavior with a single argument. We could, in theory, support any number of custom libc variants with different features using the same technique, but that seems out of scope for a general purpose toolchain. |
Not sure where the thought that nano was deprecated. I was thinking it might be worth looking at adding support for long long to nano-libc. Not sure what else we get from the "fuller" newlib. Change the build to support both nano-libc & "fuller" is work that would need to be done to crosstool-ng. |
|
OK I think the confusion from my side stems from the fact that perhaps newlib nano used to be a separate project and it is instead now part of GCC? |
|
Yes, newlib-nano was originally a fork of newlib (separate from GCC), but its features were merged upstream as configure options back in 2014. |
|
@pfalcon If a user falls in the "I want code compatibility" use-case, I can see the addition of long long support being very attractive. However, if I was just trying to use tinycbor (which "selects" NEWLIB_LIBC) and I suddenly found out my sample jumped 13k in size, I would be a bit disgruntled. @pabigot This is a great analysis. I agree that an option to build newlib nano might need to be explored. |
|
@pabigot: Just as as everyone, I thank you for detailed analysis. And just as everyone, I think that we should have more, many more configuration options to satisfy everyone's wishes. But that's quite a separate matter, with its own requirements, analysis phase (much more detailed than provided here), implementation effort, then review effort. Here we however have much simpler task at hand - decide whether to apply this patch now, or not. (For reference, this feature implementation cycle is already 4 months, if counting from #62 creation.) |
@mike-scott, I fully agree that it's sad to to get 13K bump in size. But thinking more about it, the root of sadness is that "tiny" libraries depend on the whole bloated C library. But let's not indulge in sadness, and look at the bright side of things. My interpretation of why tinycbor depends on Newlib is that "people want to write software which uses good deal of libc functionality". But then it's easy to imagine to that there're even more software which wants to use even more of libc. So, we just need to make a decision about this patch - whether we want to set polar bounds of libc support in Zephyr (full Newlib vs minlibc), and let interested parties to fill choices inbetween (with a usual warning about premature optimization), or stay at the adhoc point we're now. |
nashif
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to look into supporting multiple libc configurations, this patch is going to break many users and applications that are on the limits of flash and ram and is the benefit is minimal (we did not need that the last 5 years, why change now?).
Because nobody used Zephyr, and people only start to use it now, and discover a lot of issues. |
|
Note that one of the usages for %lld formatting specifier is to print unix time, e.g. for various networking protocols. It's well known by now that this time cannot be represented as 32-bit value, as that leads to Year 2038 problem. So, unix time must be at least 64-bit value, and its printing consequently should be done using %lld formatting specifier. So, this is (or borders on) a security issue, so adding @d3zd3z into loop. (Yeah, it's me just thinking what to do if this patch doesn't go thru in regard to GoogleIoT SDK porting. Like, me coming to Google folks and saying "Hey, you recently fixed a security issue in your codebase. Would-be CVE, or maybe actual CVE. But would you be so kind to add an option to revert back to the original insecure behavior, just for the sake of Zephyr RTOS?" Would be a fun conversation.) |
|
@pfalcon CBOR data formatting is now included in the v1.1 LwM2M spec. I'm looking to remove the newlib dependency of tinycbor so that the smaller boards will be able to move forward. I appreciate you're newly found optimism towards better portability in Zephyr, but I'm still not convinced the average board maintainer will be "OK" with this change as it is. That being said, I would be equally frustrated if I was working on the GoogleIoT stuff. :/. Unfortunately, I see this PR as being linked to a "small" newlib vs. "larger" newlib config option. |
Then we need specific review votes (from "board maintainers"). |
|
Confirming that this patch solved the issue was GoogleIoTSDK - where previously it just crashed, now it works as expected. |
|
Adding this as a critical issue for me too. I'm trying to build a firmware based on Lua 5.3 and Zephyr OS, which breaks in many wonderful ways due to the lack of long long support in Newlib. Using the minimal libc is quite difficult with Lua. It's quite funny really: floating point support looks fine, but no long longs. EDIT: I found an workaround by using |
This commit enables 'long long' (64-bit) support in the newlib. Note that 'long long' support is only enabled for the normal newlib (libc.a), without affecting the newlib nano variant (libc_nano.a). Signed-off-by: Stephanos Ioannidis <[email protected]>
4926990 to
d4cd4f7
Compare
|
Force pushing new commit, as #153 is now merged: |
|
Running newlib normal ( newlib nano ( |
Signed-off-by: Kumar Gala [email protected]