This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Add check for building with picolibc
ClosedPublic

Authored by domin144 on Jun 30 2023, 11:42 AM.

Details

Summary

This is intended to identify changes that would fail to build on
embedded platforms e.g. D152382

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Is this ready for review?

Yes this is rebased, ready for review and CI is looking good now. The same goes for D155521. I've pinged you on Discord as requested.

ldionne requested changes to this revision.Sep 28 2023, 11:41 AM

This looks pretty clean, a lot cleaner than I would have expected for an embedded c library. I have a few questions and comments but I don't see major blockers at this point.

libcxx/include/__atomic/cxx_atomic_impl.h
320–325

I am not certain we want to disable this warning here. At the end of the day, it seems like the compiler is warning us about something legitimate, no? We could perhaps silence that warning in the armv7m testing configuration instead?

libcxx/test/configs/armv7m-libc++.cfg.in
2–3

This comment seems copy-pasted (same for the libc++abi one and probably the libunwind one too).

13

Why is that include needed?

libcxx/test/libcxx/system_reserved_names.gen.py
105 ↗(On Diff #557024)

What is the relationship between picolibc and _NEWLIB_VERSION?

libcxx/utils/ci/run-buildbot
622

It's weird that you're passing this here but also hardcoding it in your Armv7M-picolibc.cmake cache file. IMO it would make more sense *not* to hardcode it in the cache.

libcxx/utils/libcxx/test/features.py
108

Otherwise this test will fail in -ffreestanding mode, where main() isn't special.

218

Yeah it's pretty lame, we need to change that.

This revision now requires changes to proceed.Sep 28 2023, 11:41 AM

Make changes requested by @ldionne and rebase.

michaelplatings marked 5 inline comments as done.Sep 29 2023, 4:22 AM
michaelplatings added inline comments.
libcxx/test/configs/armv7m-libc++.cfg.in
13

Added comment explaining it.

libcxx/test/libcxx/system_reserved_names.gen.py
105 ↗(On Diff #557024)

Picolibc is derived from Newlib, so defines _NEWLIB_VERSION. I've updated the comment to make it clear that it applies to both libraries.

libcxx/utils/ci/run-buildbot
622

Yes I'd prefer not to put the target in the CMake cache file, but that I think that would cause confusion because it would be inconsistent with all the other CMake cache files.

Would very much like to see this land soon. Two small comments

libcxx/test/configs/armv7m-libc++.cfg.in
18

Doesn't this mean the library can't be used without adding this include to any build system that is trying to use libc++ with picolibc? I think it would be a lot cleaner if we could just add the required includes to the libc++ source code?

As far as I recall from the last time I tried to use libc++ with picolibc this was not needed but I may have been building a slightly different configuration

39

Could this be autodetected instead by checking for the presence of the picolibc header? Then it would also work out of the box with the default config file.

michaelplatings marked 2 inline comments as done.Oct 2 2023, 2:37 AM
michaelplatings added inline comments.
libcxx/test/configs/armv7m-libc++.cfg.in
18

Doesn't this mean the library can't be used without adding this include to any build system that is trying to use libc++ with picolibc?

Most libc++ headers already indirectly include picolibc.h so most of the time this isn't a problem in practice. It's a minority of tests that fail without this include.

I think it would be a lot cleaner if we could just add the required includes to the libc++ source code?

I agree with you that it would be cleaner to remove this line and instead change the libc++ headers. However I can imagine that adding extra includes could cause undesirable knock-on effects. I think it's best to limit the scope of this change to only adding the test setup and not touching libc++ itself.

39

I don't think we can infer that we're targeting picolibc from the presence of the header. It might also happen to be on an include path in a scenario in which another platform is targeted. I think it's best to be explicit here.

arichardson added inline comments.Oct 30 2023, 10:28 PM
libcxx/test/configs/armv7m-libc++.cfg.in
18

Do you have a list of tests that fail? If it's just a few maybe the better solution is to add the fixme to REQUIRES?

39

I meant trying to compile something like the following:

#include <stdio.h>
#ifndef __PICOLIBC__
#error not picolibc
#endif
domin144 commandeered this revision.Nov 2 2023, 1:57 AM
domin144 added a reviewer: michaelplatings.
domin144 updated this revision to Diff 557982.Nov 2 2023, 2:05 AM
This comment was removed by domin144.
domin144 updated this revision to Diff 557983.Nov 2 2023, 2:08 AM

rebase (minor conflict in doc formatting)
move LIBCXX-PICOLIBC-FIXME derivation to features.py (as suggested in comment)

domin144 added inline comments.Nov 2 2023, 2:21 AM
libcxx/test/configs/armv7m-libc++.cfg.in
18

There are 138 tests that fail without the ' -include picolibc.h'.
I haven't reviewed them all, but most of them fall into two categories:

  • use of __input reserved name in stdlib.h (e.g. algorithm.compile.pass.cpp)
  • ld.lld: error: undefined symbol: std::__1::basic_streambuf<char, std::__1::char_traits<char>>::seekoff(long long, std::__1::ios_base::seekdir, unsigned int) (e.g. eof.pass.cpp)

I would prefer to solve it in a separate patch as this might need modification to libc++ headers.

39

Thanks! Applied in the new diff.

One suggestion otherwise looking good to me

libcxx/test/configs/armv7m-libc++.cfg.in
18

If it's so many tests then yes that sounds good to me.

libcxx/test/libcxx/system_reserved_names.gen.py
105 ↗(On Diff #557024)

So it seems this header needs the picolibc.h include? Could we just add something like #include <stdlib.h> to the top of this file?

libcxxabi/test/configs/armv7m-libc++abi.cfg.in
30

I believe this is not needed anymore since it's in features.oy now?

domin144 updated this revision to Diff 558024.Nov 6 2023, 4:40 AM

Applied suggestions from review:

  • remove redundant LIBCXX-PICOLIBC-FIXME definition
  • add "#include <string.h>" to make sure the _NEWLIB_VERSION is available
domin144 marked 3 inline comments as done.Nov 6 2023, 4:45 AM
domin144 added inline comments.
libcxx/test/libcxx/system_reserved_names.gen.py
105 ↗(On Diff #557024)

Done.

libcxxabi/test/configs/armv7m-libc++abi.cfg.in
30

Good spot, I removed this part in the libc++ version and missed it here. Thanks.

Not quite sure what is going on with the CI here, it sounds like GCC is happy with those tests even though the has-64-bit-atomics feature check fails. Should be good to land once that has been fixed and once @ldionne has approved this review.

libcxx/test/libcxx/selftest/dsl/dsl.sh.py
10

To be pedantic, this is not really a problem with picolibc but rather with using an executor stub. Maybe XFAIL: libcxx-fake-executor and defining that feature in the .cfg?

libcxx/test/libcxx/selftest/pass.cpp/run-error.pass.cpp
9

Same here

libcxx/test/libcxx/selftest/pass.mm/run-error.pass.mm
10

And here

domin144 updated this revision to Diff 558036.Nov 7 2023, 6:52 AM
domin144 marked 2 inline comments as done.

Applied comments and fixed gcc-13 failure:

  • add libcxx-fake-executor feature,
  • move "has-64-bit-atomics" after "gcc" feature
domin144 marked 3 inline comments as done.Nov 7 2023, 6:56 AM

The gcc-13 error in "has-64-bit-atomics" detection is:

/home/domwoj01/sources/llvm-project/builds/libcxx-gcc-13-test/build/include/c++/v1/limits:529:69: error: ‘float_denorm_style’ is deprecated [-Werror=deprecated-declarations]
  529 |     _LIBCPP_CONSTEXPR const float_denorm_style numeric_limits<_Tp>::has_denorm;
      |                                                                     ^~~~~~~~~~

The tests are run with the flag below, while the "has-64-bit-atomics" are not.

AddCompileFlag("-D_LIBCPP_DISABLE_DEPRECATION_WARNINGS"),

I moved the "has-64-bit-atomics" check to the end, so it also gets the flag.

domin144 updated this revision to Diff 558105.Nov 15 2023, 2:55 AM

Rebase to resolve conflict
Update picolibc, so XFAIL for stdio.h is no longer needed.

domin144 updated this revision to Diff 558141.Nov 20 2023, 11:42 PM

resolve conflict

domin144 updated this revision to Diff 558169.Nov 24 2023, 12:33 PM

Rebase, add XFAIL for new tests of floating point atomics as they need support for 64-bit atomic types.

ldionne requested changes to this revision.Nov 27 2023, 8:51 AM
ldionne added inline comments.
libcxx/test/configs/armv7m-libc++.cfg.in
4

This doesn't have to be a Lit substitution. You could just do this instead:

libc_linker_script = '@CMAKE_INSTALL_PREFIX@/lib/picolibcpp.ld'

# and then below:

' -T {}'.format(libc_linker_script)
18

Let's name the configuration files armv6m-picolibc-libc++.cfg.in since this is not a generic armv7m configuration, it's tied to picolibc.

libcxx/test/libcxx/system_reserved_names.gen.py
23–24 ↗(On Diff #558169)

We shouldn't be including a file here at all, since this breaks the intent of the test (which is to check that every individual header doesn't use poisoned identifiers).

For example if <string.h> has #undef __allocator, we'd now fail to have coverage for e.g. vector using __allocator as a name, since <string.h> would always undef it.

I think we should rely on the -include picolibc.h part for now. It seems better to only affect the picolibc configuration with this "workaround" than affect the correctness of this test for everyone else.

libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp
17

I'm pretty sure this is a real bug in our test suite. Fine with this being a FIXME for now, but we should really investigate and fix it for everyone.

libcxx/utils/libcxx/test/features.py
291–292

This comment becomes unnecessary once I move the compiler detection features above. Still not great to rely on implicit ordering but 🤷🏼‍♂️

294

Just to record what we're going to do: we're going to leave this feature as-is and then change non-lockfree-atomics to be something like

Feature(
    name="has-128-bits-atomics",
    when=lambda cfg: sourceBuilds(
        cfg,
        """
        #include <atomic>
        struct Large { __int128_t storage; };
        std::atomic<Large> x;
        int main(int, char**) { (void)x.load(); return 0; }
      """,
    ),
),
This revision now requires changes to proceed.Nov 27 2023, 8:51 AM
domin144 updated this revision to Diff 558176.Nov 28 2023, 4:51 AM

Applied changes from comments.

ldionne accepted this revision.Nov 29 2023, 6:55 AM

LGTM, thanks! It's nice to have official support for a more embedded libc.

This revision is now accepted and ready to land.Nov 29 2023, 6:55 AM
This revision was landed with ongoing or failed builds.Nov 29 2023, 7:43 AM
This revision was automatically updated to reflect the committed changes.