Page MenuHomePhabricator

[libcxx] Port to NuttX(https://nuttx.apache.org/) RTOS
ClosedPublic

Authored by xiaoxiang781216 on Oct 1 2020, 10:32 PM.

Details

Reviewers
ldionne
Group Reviewers
Restricted Project
Commits
rGf0785c1f7acd: [libcxx] Port to NuttX (https://nuttx.apache.org) RTOS
Summary

Since NuttX conform to POSIX standard, the code need to add is very simple

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptOct 1 2020, 10:33 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
xiaoxiang781216 requested review of this revision.Oct 1 2020, 10:33 PM

Updating D88718: [libcxx] Port to NuttX(https://nuttx.apache.org/) RTOS

mclow.lists added inline comments.
libcxx/include/__config
1124

How does __NuttX__ get defined?

libcxx/include/support/nuttx/xlocale.h
18

This change probably belongs in __config

25

Why all the #undefs?

A general comment - is someone going to run a libc++ test bot for NuttX? Otherwise, how will we know when changes that we make to libc++ break NuttX support?

ldionne requested changes to this revision.Oct 2 2020, 9:19 AM
ldionne added a subscriber: ldionne.

Thanks for the contribution. It's nice to see a new platform that's as easy to support as this.

However, I'll echo Marshall's comment here. If we are to add support for a new platform, we need CI for it. Are you willing and able to provide that? We use BuildKite for CI and it's fairly simple to setup.

This revision now requires changes to proceed.Oct 2 2020, 9:19 AM
libcxx/include/__config
1124

It get defined in two ways:
1.Build toolchain target NuttX OS from here:

https://github.com/NuttX/buildroot
The result binary define __NuttX__ automatically just like how Linux toolchain define __linux__

2.Define NuttX from the board/toolchain specific Makefile, like this:

https://github.com/apache/incubator-nuttx/blob/master/boards/arm/imxrt/imxrt1050-evk/configs/libcxxtest/Make.defs#L77
libcxx/include/support/nuttx/xlocale.h
18

Yes, could be, but I want to minize the change as small as possible. But if you want, I will move the change to __config.

25

NuttX support a special target platform named sim which is basically a normal process running on Linux/macOS/Cygwin, but simulate all OS API inside it. Since it is built by the host toolchain, I have to undef these macros to avoid libcxx detect the wrong target. Here is more info about simulator:
https://cwiki.apache.org/confluence/display/NUTTX/NuttX+Simulation

A general comment - is someone going to run a libc++ test bot for NuttX? Otherwise, how will we know when changes that we make to libc++ break NuttX support?

NuttX is RTOS, so the setup for both build and test is much hard than normal development environment(e.g. Linux, macOS or Windows):
1.We need prepare the cross build toolchain
2.We need build libcxx with NuttX OS together
3.We need run the test with the real hardware or simulator
It isn't suitable to incorporate this type of change into libcxx because it's too specific to NuttX environment. On the other hand, we plan to integrate libc++ test into NuttX daliy testing infrastructure, like what we have already done for Linux Test Project here:
https://github.com/apache/incubator-nuttx-apps/pull/410

(Also from the Apache NuttX team)
If what we set up on our end in CI is not sufficient enough, I would be willing to look at what needs to be done to provide that here via what you have in BuildKite. Supporting the simulator target probably would not be too difficult.

(Also from the Apache NuttX team)
If what we set up on our end in CI is not sufficient enough, I would be willing to look at what needs to be done to provide that here via what you have in BuildKite. Supporting the simulator target probably would not be too difficult.

That would be great.

Otherwise, have you considered building with -DLIBCXX_ENABLE_LOCALIZATION=OFF? I added this "flavor" of libc++ specifically to cater to the needs of smaller OSes that don't provide localization support. The best case scenario would be if libc++ could work on your platform out-of-the-box without support for localization: that way, we wouldn't even have to add another configuration for libc++.

LIBCXX_ENABLE_LOCALIZATION is a great feature what we find it consume a large code size for small OS. Could you point me the change? I can't find LIBCXX_ENABLE_LOCALIZATION in libc++ 11.0.0 release.

@ldionne could you give more info about LIBCXX_ENABLE_LOCALIZATION?

Just find it here: https://reviews.llvm.org/D90072
I will change the porting to adapter the new flag, which is very important for the small embeded system.

Updating D88718: [libcxx] Port to NuttX(https://nuttx.apache.org/) RTOS

@ldionne and @mclow.lists, the update should address your comment except the test and local. both of them could be addressed in the new patch.

Updating D88718: [libcxx] Port to NuttX(https://nuttx.apache.org/) RTOS

ldionne requested changes to this revision.Mon, Nov 9, 8:06 AM
ldionne added inline comments.
libcxx/include/__config
27

I'm not comfortable with undefining these macros here in libc++. Those are compiler-defined macros, and it's not libc++'s job to undefine them.

It seems to me that you either need a compiler target triple that understands NuttX, or to do whatever macro magic you want to do lower in the stack.

libcxx/include/__locale
26

Do you need this anymore? You shouldn't need it if you're using LIBCXX_ENABLE_LOCALIZATION=OFF.

This revision now requires changes to proceed.Mon, Nov 9, 8:06 AM
libcxx/include/__config
27

Simulator is a special case here: It's a normal program run on Ubunutu or macOS and then should be compiled by host gcc toolchain, but all functions(POSIX, libc...) are provided by NuttX. If we don't undefine these macros, we have to modify every location where reference them. The later approach is very fragile and hard to maintain. Do you have other suggestion?

libcxx/include/__locale
26

Since stream and regex isn't supported when LIBCXX_ENABLE_LOCALIZATION=OFF, nuttx/xlocale.h is still needed to let the user turn on/off LIBCXX_ENABLE_LOCALIZATION as needed.

Updating D88718: [libcxx] Port to NuttX(https://nuttx.apache.org/) RTOS

@ldionne I move the toolchain macro undefinition to NuttX's Makefile, please review again.

@ldionne do you have time to review the update again?

ldionne accepted this revision.Wed, Nov 18, 1:14 PM

Do you have commit privilege? If so, please go ahead. Otherwise, please provide Author Name <email@domain> I should use when committing this for you.

libcxx/include/__config
27

Do you have other suggestion?

Yes, a compiler target triple that actually understands your platform.

This revision is now accepted and ready to land.Wed, Nov 18, 1:14 PM

Do you have commit privilege? If so, please go ahead. Otherwise, please provide Author Name <email@domain> I should use when committing this for you.

I have the memory of a goldfish. I asked you the same in D91074. I'll commit this for you, thanks!

Just FYI, you also forgot to add support/nuttx/xlocale.h to libcxx/include/CMakeLists.txt, but I realized it when applying the patch locally and added it myself.

This revision was automatically updated to reflect the committed changes.

Just FYI, you also forgot to add support/nuttx/xlocale.h to libcxx/include/CMakeLists.txt, but I realized it when applying the patch locally and added it myself.

Thanks for helping to add the new header file to CMakeLists.txt.