[cpullvm] - Prepare for LIBCXX tests enablement ARM/RISC-V. #323
[cpullvm] - Prepare for LIBCXX tests enablement ARM/RISC-V. #323Pawan Nirpal (pawan-nirpal-031) wants to merge 1 commit into
Conversation
|
Jonathon Penix (@jonathonpenix) Could you please review and add other reviewers as you see fit. |
|
At a glance, it looks like this is headed in the right direction, thanks Pawan! Few misc. thoughts: The libc++abi/libunwind tests are also included under the broader "enable libc++ tests". Right now they're running as we just run all enabled tests: cpullvm-toolchain/qualcomm-software/scripts/test.sh Lines 20 to 24 in 8f6aeba I'm fine if we enable this stuff incrementally, but I think we have a few options to do so. Things that make the most sense to me:
Pawan Nirpal (@pawan-nirpal-031) do you know/remember how long the libc++ tests were taking to run on your local machine? Arm has these split out as they apparently take a long time to run, so we might want to do # 2 above even if we resolve build/test failures. |
|
I am not 100% sure how this ends up working, but are we correctly setting These prevent some of the really slow tests from running. |
|
They are already turned off in Pawan Nirpal (@pawan-nirpal-031), let's focus on fixing the issues first. We can merge the fixes without enabling the tests. Before activating the tests, we may need to split them into separate runs, as they seem to still take considerable time even with long/large tests disabled. |
|
Ana Pazos (@apazos) Yes I think that would be preferable. I've fixed the tests, some of them have to be unsupported due to memory limitations set to 2MB IIRC, those tests run into memory issues and I suppose aren't meant for embedded. Long double tests have to be xfailed through and through. But I will update this PR and we can discuss on the approach more. |
yet to get a hold on this one : libcxx/test/std/strings/c.strings/no_c8rtomb_mbrtoc8.verify.cpp |
|
Hi Pawan Nirpal (@pawan-nirpal-031), Besides tests failing, there are several ways to disable tests to control the time it takes to run the tests. Bringing them up here so we select the most appropriate one depending on the tests running time and the tests failing, so we can progressively enable them as we fix the issues:
We can consider enabling tests for libc++ in parts, as done in ATfe, they skip ninja check-cxx which takes long, see ATfE /embedded/scripts/test.sh
For example, if you only enable ninja check-cxxabi or ninja check-unwind or both, are there failures? |
|
Ana Pazos (@apazos) Thanks for the detailed description, Yes I feel incrementally enabling would be the best option, ninja check-cxx are the ones that I am focusing on this PR. I imagine we don't want any source changes apart from changes in qualcomm-sotfware/* is that right? If yes I don't think there's much we can do to disable failing tests that need source changes, I'm wrestling with some riscv32* tests that are need support from upstream, Do you think we can push some of these libcxx source changes upstream first? Or we can disable the tests now and then start upstreaming fixes into libcxx? |
|
We have no custom LLVM changes in qualcomm-software and it keeps up with latest LLVM tip. |
|
Ana Pazos (@apazos) Jonathon Penix (@jonathonpenix) Sam Elliott (@lenary) I have worked libcxx into a different test_libcxx.sh test script file similar to ATFE, All the tests locally pass, however I am omitting check unwind, abi and picolibc tests. The ETA for the libcxx tests locally is around 6-6:30 hours. The failure now seems to be purely CI timeout. |
|
Pawan Nirpal (@pawan-nirpal-031) this patch will have to be further cleaned up as some of these changes you are pushing in a separate PR. Also, might make sense to combine the xfailed test patch with the patch that enables the tests. |
Hi Ana Pazos (@apazos) I'm sorry I didn't quite understand, you said we were only going to push the fixes and defer the enablement to a later PR. I will clean up the infra changes alright. Then as per me this PR should have xfails.py (fixes) and the separation of test_libcxx.sh. So if we also push the xfails to the enablement patch, this is just a CI change patch which separates the ninja check, is that what we intend to achieve? Apologies for the confusion. |
e936aa0 to
dbe1f95
Compare
98e9fe4 to
2dec827
Compare
|
Ana Pazos (@apazos) Jonathon Penix (@jonathonpenix) could you review it now. Also this seems to be failing the license check what would be the appropriate one? |
Jonathon Penix (jonathonpenix)
left a comment
There was a problem hiding this comment.
This looks pretty close to me, thanks a lot Pawan Nirpal (@pawan-nirpal-031) for working on this.
Also this seems to be failing the license check what would be the appropriate one?
Apache 2 + LLVM is right, AFAIK that check assumes BSD 3 clause by default which is where our errors generally come from--it's something we have to sort out with the folks who maintain that. So, there's nothing you need to change here AFAIK
| ], | ||
| description="stable_sort.pass.cpp runs exhaustive permutation tests up to N=8 " | ||
| "and large-N sorts up to N=2053. Under QEMU emulation this exceeds " | ||
| "the test timeout. ATfE runs on FVP which is faster; we exclude " |
There was a problem hiding this comment.
Is this test not running for RISC-V or is it actually passing?
Seems surprising to me that we wouldn't have to handle RISC-V variants similarly. (I assume RISC-V QEMU isn't that much faster than Arm/AArch64?)
There was a problem hiding this comment.
Pawan Nirpal (@pawan-nirpal-031) it seems these xfails were removed entirely in the latest patch? Does that mean all targets are passing without these xfails?
| # or libc++ testing enabled) we can run everything. | ||
| cd "${REPO_ROOT}"/build | ||
| ninja check-all-llvm-toolchain | ||
| ninja check-all |
There was a problem hiding this comment.
Do you have a rough idea how long the libc++ tests for an individual variant take to run? Can you check on this?
Needs input from others, but it might be worthwhile including at least a couple "core" variants to be tested during premerge (and normal nightly?) checks and we can run the full suite (like what you did for test_libcxx.sh) separately. See ex: https://github.com/arm/arm-toolchain/blob/arm-software/arm-software/embedded/scripts/test_pre_merge.sh
There was a problem hiding this comment.
Ping
| # This can be enabled with the --xunit-xml-output option. The file | ||
| # written will be relative to the individual suite's build directly, | ||
| # so the same name can be used for all files for consistency. | ||
| export LIT_OPTS="--ignore-fail --xunit-xml-output=lit_results.junit.xml" |
There was a problem hiding this comment.
We might want to omit this part for now.
I think eventually we want something like this (we've discussed re-adding this into test.sh a few times as we've already been bitten a few times by the "exit at first fail" hiding other issues) but we'd need people to be aware to look at this (or, do some more handling of it to make the failures more obvious).
Ana Pazos (@apazos) do you have thoughts/preferences here?
(I guess this is also a bit moot though until we actually schedule something to run these tests)
838483e to
f8cf672
Compare
c6826a8 to
d9c35e6
Compare
Jonathon Penix (@jonathonpenix) these tests take a long time to run, I wanted to xfail them to investigate later, they take long enough that they don't report a pass or a fail. But I'm skeptical if CI will actually allow them to run that long. So I've removed them for now. |
7ec01ac to
d82c91a
Compare
1d4ce44 to
28cfad2
Compare
| # On bare-metal static executables the local-exec TLS model is always | ||
| # correct. Without this, -fPIC variants use general-dynamic TLS which | ||
| # emits __tls_get_addr calls that are not available on bare-metal. | ||
| string(APPEND compile_arch_flags " -ftls-model=local-exec") |
There was a problem hiding this comment.
This isn't right--there are variants that are specifically/intentionally built under initial-exec to support non-static executables. We can't just not do that anymore.
If you're seeing an issue, please provide more information.
The __tls_get_addr issue described in the comment should already have been addressed (that's what the lines that were deleted are there for).
60a2dc8 to
47fb2fa
Compare
|
There are 7 variants for which libc testing is enabled but not libcxx tests.
Pls enable libcxx tests for the above variants as well. |
a20e2c3 to
5beb4d8
Compare
FYI: These variants also has |
520c2a8 to
338d200
Compare
Noted, added the two armv7s, will add riscv in the following patches due to CI timeout all versions cannot be accommodated. |
Signed-off-by: Pawan Nirpal <pnirpal@qti.qualcomm.com>
338d200 to
baa538f
Compare
Manage fixes and split testing script to run check-cxx separately to reduce the testing time for libcxx tests.