Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

[libc++] Add check for building with picolibc
Needs RevisionPublic

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

Details

Reviewers
DavidSpickett
ldionne
simon_tatham
philnik
Group Reviewers
Restricted Project
Restricted Project
Restricted Project
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
DavidSpickett added a comment.EditedJul 5 2023, 9:17 AM

I got this to build on Linaro's container.

clang 15.0.0 (our current compiler) fails with errors about not finding types that should be in locale.h. The files are all in the right place, and moving to clang 16 fixes this. So I assume some driver changes did that.

clang 16.0.4 failed with this on one part of the build:

clang-16: error: argument unused during compilation: '-G R-' [-Werror,-Wunused-command-line-argument]

Turns out that -GR- is the equivalent of /GR- which is the msvc option to disable RTTI (https://learn.microsoft.com/en-us/cpp/build/reference/gr-enable-run-time-type-information?view=msvc-170).

My guess is that some part of clang checked for this -GR- or -fno-rtti and forgot to claim the other one. This was fixed in https://github.com/llvm/llvm-project/commit/cd18efb61d759405956dbd30e4b5f2720d8e1783 but we will have to wait until 17.0.0 to have that, and I don't want to be building a custom clang for Linaro's bots.

So I think the sensible thing to do right now is add -Wno-unused-command-line-argument to the build flags and I will update the container's clang to 16.0.4. Which we would have to do eventually anyway.

I'll let you know once that's done so you can update this diff and see a green CI build before landing.

libcxx/utils/ci/run-buildbot
640

flags="--sysroot=${INSTALL_DIR} -Wno-unused-command-line-argument"

For reasons explained in the top level comment.

clang-16: error: argument unused during compilation: '-G R-' [-Werror,-Wunused-command-line-argument]

Turns out that -GR- is the equivalent of /GR- which is the msvc option to disable RTTI

That's familiar – I ran into that in another context last month. libc++ contains this in its cmake files:

function(cxx_add_rtti_flags target)
  if (NOT LIBCXX_ENABLE_RTTI)
    target_add_compile_flags_if_supported(${target} PUBLIC -GR-)
    target_add_compile_flags_if_supported(${target} PUBLIC -fno-rtti)
  endif()
endfunction()

and that "add_compile_flags_if_supported" is somehow misidentifying whether it's supported. Possibly because -GR- is possible to re-parse in a non-msvc-style command line as the completely different option -G with argument word R-. The fix might be to explicitly query the command-line flavour, via if (CMAKE_C_COMPILER_ID MATCHES "MSVC" OR CMAKE_C_COMPILER_FRONTEND_VARIANT MATCHES "MSVC") or something along those lines, and use that to condition the use of that MSVC option.

Aha, even better, check LIBCXX_TARGETING_MSVC which the same CMakeLists has already defined.

michaelplatings marked 2 inline comments as done.

clang -> $CC

Production containers are updated.

I've done a build on one, with the cc setting fixed, and it works. So in theory, update this one more time and we'll get a green build.

libcxx/utils/ci/build-picolibc.sh
70

Needs to literally be cc. As /usr/local/bin/cc is linked to clang-16.0.4 in our images, we don't set any env vars.

I know it's not great for anyone reproducing, and an env var would probably be better but cross that road when it's needed.

Default to compiling picolibc with cc

michaelplatings marked an inline comment as done.Jul 6 2023, 10:38 AM
michaelplatings added inline comments.
libcxx/utils/ci/build-picolibc.sh
70

Thanks for explaining. Now if CC isn't set then cc will be used.

michaelplatings marked an inline comment as done.Jul 6 2023, 10:39 AM

We got a green build - https://buildkite.com/llvm-project/libcxx-ci/builds/27739#_

All the infrastructure changes are done, so please libcxx folks proceed with the review as usual.

libcxx/utils/ci/build-picolibc.sh
70

Nice, that's much more convenient.

Mordante added inline comments.Jul 10 2023, 8:34 AM
libcxx/docs/index.rst
135

The CI is green. Is this work-in-progress or working?

libcxx/utils/ci/build-picolibc.sh
10

Can you add a short comment what this file does?

michaelplatings marked 2 inline comments as done.

Add summary comment to build-picolibc.sh

libcxx/docs/index.rst
135

At present all this job will do is check that libc++ can build against picolibc. It doesn't yet run any tests so there's not yet any guarantee that the build output actually works. Therefore work-in-progress. Once tests are running and green this note should be removed.

ldionne added inline comments.Jul 10 2023, 1:25 PM
libcxx/utils/ci/run-buildbot
649

For now I just want to set a baseline of keeping libcxx+picolibc building. Once that's in place we can start ratcheting up the quality bar.

Ratcheting the quality bar is usually done by first setting up the test suite and adding a bunch of XFAIL and UNSUPPORTED markup that is then gradually removed. In this case, we're not running the test suite at all, and that's insufficient to claim support for it. I hate to be that person, but this patch needs to include whatever's necessary to build and run the test suite before it is merged.

One thing we could discuss is actually only compiling and linking all the tests, but not running them right away since that might require more complicated setup. I'm not sure we want to set that precedent, but I'd definitely be open to the discussion if we had everything compiling and linking. If we were to do that, we'd replace the %{exec} substitution by something like true and that would cause the tests not to be executed for real.

Mordante added inline comments.Jul 14 2023, 9:56 AM
libcxx/utils/ci/run-buildbot
649

I too would like to see XFAIL and UNSUPPORTED. That gives us more insights in which parts work and which parts don't work. This also helps with future patches to decide how much effort we should spend on getting it to work with picolib. For example getting locales tested properly may take quite a bit of effort; at least that has been my experience. When adding a new locale test it's easier to justify to add a new XFAIL/UNSUPPORTED.

Compile and link tests

Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 14 2023, 10:49 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
michaelplatings marked 3 inline comments as done.Jul 14 2023, 10:52 AM
michaelplatings added inline comments.
libcxx/utils/ci/run-buildbot
649

OK I've updated it to compile & link all the tests, using true for %{exec} as suggested.

philnik added inline comments.Jul 14 2023, 11:00 AM
libcxx/test/std/atomics/atomics.types.generic/integral.pass.cpp
9

I guess this is caused by a missing libatomic?

libcxx/utils/ci/run-buildbot
632

Personally I'm not super concerned about building picolibc every time. I don't expect it to be a long build (probably similar to libc++), and testing always the latest version seems like a nice thing to me.

arichardson added inline comments.Jul 14 2023, 1:40 PM
libcxx/utils/libcxx/test/features.py
214

Why pipe? Is that function actually called inside libc++?

libunwind/test/configs/armv7m-libunwind.cfg.in
21

There should probably be a comment here such as todo: run using qemu-system-arm?

michaelplatings marked 3 inline comments as done.Jul 14 2023, 2:26 PM
michaelplatings added inline comments.
libcxx/test/std/atomics/atomics.types.generic/integral.pass.cpp
9

Yes, there's no libatomic or equivalent. Perhaps something like REQUIRES: atomic would be better here.

libcxx/utils/ci/run-buildbot
632

Yes, the time to build picolibc is seconds - negligible compared to the testing time. The revision of picolibc is pinned, but doing it this way does make it trivial to update to the latest version.

libcxx/utils/libcxx/test/features.py
214

check_assertion.h uses pipe, fork, exec etc. Picolibc declares these in unistd.h but doesn't implement them. Therefore this feature check was previously passing with picolibc, but now fails at link time.

libunwind/test/configs/armv7m-libunwind.cfg.in
21

Agreed

michaelplatings marked 5 inline comments as done.Jul 14 2023, 3:11 PM
michaelplatings added inline comments.
libcxx/utils/libcxx/test/features.py
214

(Despite the name "has-unix-headers", the check is mostly (always?) used to xfail tests that use check_assertion.h)

I want to have a closer look when the CI is green. But it seems the number of disabled tests is quite small :-)

libcxx/test/std/atomics/atomics.types.generic/integral.pass.cpp
9

When there are no plans to add that to picolib soon I think we indeed should add something like //UNSUPPRTED: no-libatomic (maybe needs a better name). The LIBCXX-PICOLIBC-FIXME should be used for things that can be fixed.

michaelplatings marked 2 inline comments as done.

Add "atomics" feature, run git-clang-format, and other minor tweaks

libcxx/test/std/atomics/atomics.types.generic/integral.pass.cpp
9

For consistency with the existing non-lockfree-atomics feature I've gone with the name atomics.

philnik accepted this revision as: philnik.Jul 17 2023, 5:12 PM

LGTM. Leaving final approval to @ldionne, since he knows the CI best.

libcxx/test/std/atomics/atomics.types.generic/integral.pass.cpp
9

I would just go with // REQUIRES: atomics. That's a lot less confusing that the double negation.

michaelplatings marked an inline comment as done.

XFAIL: !atomics -> REQUIRES: atomics

libcxx/test/std/atomics/atomics.types.generic/integral.pass.cpp
9

I went with XFAIL for consistency with non-lockfree-atomics. REQUIRES means the tests won't be run at all, but I'm OK with that.

libcxx/test/std/atomics/atomics.types.generic/integral.pass.cpp
9

The CI results with XFAIL: !atomics suggest that the code to detect atomics support was incorrect. REQUIRES wouldn't have picked that up so I feel XFAIL is the better choice.

Change how atomics feature is detected and change atomic REQUIRES back to negated XFAIL

Use 16MB area of RAM to enable running bigger tests

arichardson added inline comments.Jul 18 2023, 11:13 PM
libcxx/utils/libcxx/test/features.py
98

This is effectively checking for availability of 64-bit atomics (since uintmax_t essentially has to be the same as uint64_t for ABI reasons). Maybe something like has-64-bit-atomics is more accurate? I believe this will fail for 32 bit arm picolibc since that needs a libatomic runtime function. The other larger libatomic requirements should already be handled by non-lockfree-atomics.

michaelplatings marked an inline comment as done.

atomics -> has-64-bit-atomics

Rebase after -Wcast-qual error was fixed

Is this ready for review? I see that the CI is failing. This would need to be rebased and the CI to be green.

Once that's done, please ping me on Discord. We're trying to drain the review queue here on Phabricator as part of our transition to Github PRs.

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.Thu, Sep 28, 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
318–323

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
1–2

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

12

Why is that include needed?

libcxx/test/libcxx/system_reserved_names.gen.py
105

What is the relationship between picolibc and _NEWLIB_VERSION?

libcxx/utils/ci/run-buildbot
639

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
104

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

214

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

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