Details
- Reviewers
Mordante DavidSpickett ldionne michaelplatings - Group Reviewers
Restricted Project Restricted Project Restricted Project - Commits
- rGbe811d161765: [libc++] Run picolibc tests with qemu
Diff Detail
Unit Tests
Event Timeline
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.
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. |
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.
Handle command line arguments
libcxx/utils/run-qemu.sh | ||
---|---|---|
16 ↗ | (On Diff #541466) | Thanks, I didn't know qemu had that option. |
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.
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=... |
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. |
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. |
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. |
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)
Re-add xfail for libunwind_02.pass.cpp tests.
It somehow got missed in the last update.
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 "") |
Instead, please do this in your CMake cache: