This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Run picolibc tests with qemu
ClosedPublic

Authored by domin144 on Jul 17 2023, 3:34 PM.

Details

Reviewers
Mordante
DavidSpickett
ldionne
michaelplatings
Group Reviewers
Restricted Project
Restricted Project
Restricted Project
Commits
rGbe811d161765: [libc++] Run picolibc tests with qemu

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptJul 17 2023, 3:34 PM
michaelplatings requested review of this revision.Jul 17 2023, 3:34 PM
Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald TranscriptJul 17 2023, 3:34 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
philnik added inline comments.
libcxx/CMakeLists.txt
480 ↗(On Diff #541257)

I'm not super happy with this. Could this instead be set through an environment variable when running the tests with qemu-system-arm as the default?

Surprised to not see bootcode here, but it is qemu and one can get away without it a lot of the time. Is that the case?

I will add qemu-system-arm to our containers. Focal's apt version is 4.2.1 and I checked it has the board and cpu options needed, so I expect it'll work fine.

michaelplatings marked an inline comment as done.

Move code to find qemu-system-arm program

Surprised to not see bootcode here, but it is qemu and one can get away without it a lot of the time. Is that the case?

I will add qemu-system-arm to our containers. Focal's apt version is 4.2.1 and I checked it has the board and cpu options needed, so I expect it'll work fine.

Thanks very much David.

I think bootcode is coming from the "crt0-semihost" library which is part of picolibc. Source is here.

libcxx/CMakeLists.txt
480 ↗(On Diff #541257)

I agree its not ideal, it has the inappropriate intimacy code smell.

I want to keep find_program because it's good for developer experience. With environment variables you have to remember to keep them set in your environment whereas with CMake cache variables you can set them once at configure time. There's precedent for using find_program in a cache file in clang/cmake/caches/3-stage-base.cmake so I've moved the find_program to Armv7M-picolibc.cmake where it sits more comfortably.

Use 16MB area of RAM to enable running bigger tests

QEMU has been installed.

buildkite-linaro-aarch64-libcxx-02:/# qemu-system-arm --version
QEMU emulator version 4.2.1 (Debian 1:4.2-3ubuntu6.27)
Copyright (c) 2003-2019 Fabrice Bellard and the QEMU Project developers

And it's on PATH, no /usr/local... stuff needed this time.

QEMU has been installed.

buildkite-linaro-aarch64-libcxx-02:/# qemu-system-arm --version
QEMU emulator version 4.2.1 (Debian 1:4.2-3ubuntu6.27)
Copyright (c) 2003-2019 Fabrice Bellard and the QEMU Project developers

And it's on PATH, no /usr/local... stuff needed this time.

Great, thanks David. If it's on PATH then this should Just Work...

arichardson added inline comments.Jul 18 2023, 11:22 PM
libcxx/test/libcxx/selftest/convenience_substitutions/build_run.sh.cpp
9 ↗(On Diff #541466)

Can be handled by the qemu script but that can always be a follow up change.

libcxx/utils/run-qemu.sh
16 ↗(On Diff #541466)

The arg= semihosting flag can be used to create argv.

michaelplatings marked 2 inline comments as done.

Handle command line arguments

libcxx/utils/run-qemu.sh
16 ↗(On Diff #541466)

Thanks, I didn't know qemu had that option.

Rebase after -Wcast-qual error was fixed

While you are iterating on this, please consider removing all the other jobs from the buildkite-pipeline.yml file to avoid spamming the CI. We're currently crunching for the LLVM 17 release and our CI resources are under heavy stress.

BTW, I'll be happy to review this once the dust settles for LLVM 17. I'm silent on this so far for lack of time, not interest.

OK if I get any more review comments I'll do that. The new "Armv7-M picolibc" configuration is passing in BuildKite now and the failures in other configurations look unrelated so I have no changes planned at this point.

arichardson added inline comments.Sep 29 2023, 11:05 PM
libcxx/utils/run-qemu.sh
19 ↗(On Diff #557479)

I wonder if this should instead be a full executor such as qemu_system.py (similar to ssh.py) that accepts all the same flags and just requires something like --qemu=qemu-system-arm --cpu=.. --machine=...

arichardson added inline comments.Oct 9 2023, 4:28 PM
libcxx/utils/run-qemu.sh
19 ↗(On Diff #557479)

Done as part of https://github.com/llvm/llvm-project/pull/68643, let me know if that works for you?

libcxx/utils/run-qemu.sh
19 ↗(On Diff #557479)

I'm not sure whether the Python script is an improvement but I'd go with whatever gets these patches landed sooner. I think that to be used in place of run.py the Python script would need to support the full interface of run.py i.e. --codesign_identity et al.

arichardson added inline comments.Oct 10 2023, 6:07 PM
libcxx/utils/run-qemu.sh
19 ↗(On Diff #557479)

That's a good point, so far I've tested it with libunwind, will try libc++ as well.

arichardson added inline comments.Oct 25 2023, 2:02 PM
libcxx/utils/run-qemu.sh
19 ↗(On Diff #557479)

I was able to use the new script for libc++ since the other arguments do not appear to be used.

domin144 commandeered this revision.Nov 14 2023, 1:24 AM
domin144 added a reviewer: michaelplatings.
domin144 updated this revision to Diff 558106.Nov 15 2023, 2:58 AM

Rebase
Fix paths to qemu_baremetal.py

domin144 updated this revision to Diff 558135.Mon, Nov 20, 3:44 AM

Xfail remaining libunwind test on clang-16

For reviewer context, Linaro's builders are on clang 16.0.5 at the moment and we prefer to keep that the same across the buildbots. An upgrade comes with more risk than this one test, so I advised Dominik to go with xfailing it. Linaro will of course move to 17 eventually and this test will start running.

(plus the xfail is a useful note for users other than the buildbots themselves)

domin144 updated this revision to Diff 558140.Mon, Nov 20, 11:15 PM

reupload to retrigger the buildkite job

ldionne requested changes to this revision.Mon, Nov 27, 8:54 AM
ldionne added inline comments.
libcxx/test/configs/armv7m-libc++.cfg.in
26 ↗(On Diff #558140)

libcxx/test/libcxx/selftest/pass.mm/run-error.pass.mm and probably others are now using the libcxx-fake-executor Lit feature, but it's not defined anywhere after this patch.

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

Remove this XFAIL?

This revision now requires changes to proceed.Mon, Nov 27, 8:54 AM
domin144 updated this revision to Diff 558177.Tue, Nov 28, 4:52 AM

Applied changes from comments.

domin144 updated this revision to Diff 558180.Wed, Nov 29, 2:27 AM
domin144 retitled this revision from Run picolibc tests with qemu to [libc++] Run picolibc tests with qemu.

Re-add xfail for libunwind_02.pass.cpp tests.
It somehow got missed in the last update.

ldionne accepted this revision.Wed, Nov 29, 7:48 AM

Thanks! LGTM assuming the CI passes with my comment applied.

libcxx/test/configs/armv7m-picolibc-libc++.cfg.in
40–41

Instead, please do this in your CMake cache:

set(LIBCXX_TEST_PARAMS "long_tests=False" CACHE STRING "")
This revision is now accepted and ready to land.Wed, Nov 29, 7:48 AM
domin144 updated this revision to Diff 558185.Wed, Nov 29, 8:29 AM

Applied comment:

  • moved "long_tests=False" to the cmake cache file.
ldionne accepted this revision.Wed, Nov 29, 9:21 AM
This revision was landed with ongoing or failed builds.Wed, Nov 29, 2:21 PM
This revision was automatically updated to reflect the committed changes.