[libc++] Add a utility for checking the output of commands

[philnik777]

This allows us to check the output of any tools.
For example, we can

• check for codegen properties - e.g. make sure std::copy with trivial types always calls __builtin_memmove
• we can ensure that clang-tidy checks actually diagnose the things they should

These kinds of tests are more likely to break because of external tool changes, so they shouldn't be used if there is a different way to test things.

This tool is a strict subset of FileCheckin terms of it's functionality. I didn't just make that a dependency because it has quite heavy dependencies itself, which would make it by far the most costly part of building libc++. If we find that we extend this tool a lot (or FileCheck becomes less heavy) we should reconsider whether we should take the dependency hit.

[EricWF]

Just use filecheck.

[EricWF]

I'm not OK importing our own version of an LLVM tool that we should just use if we need it.
Also, reading the tests you added with the change look like they won't last more than one version before they start failing.

Further, these tests are a second order measure of the thing we're actually trying to measure, which is "what is the performance of this code".

Lets say that the code that used to be lowered to __memmove start being lowered to some instruction that's faster. The tests start failing.

Tests should only fail when the property under test is no longer true.

For example, microbenchmarks seem like the more appropriate way to test performance. Sure they're noisy, but they're a 1st order derivative of what we're trying to test.

[ldionne]

I'm not OK importing our own version of an LLVM tool that we should just use if we need it. Also, reading the tests you added with the change look like they won't last more than one version before they start failing.

One challenge with using FileCheck is that it depends on the rest of LLVM, so we need to basically build it first. This has been a major blocker for us to do this in the past. Perhaps we could investigate shipping FileCheck with LLVM such that we can use the apt-get installed version in the Docker image.

[EricWF]

I'm not OK importing our own version of an LLVM tool that we should just use if we need it. Also, reading the tests you added with the change look like they won't last more than one version before they start failing.

One challenge with using FileCheck is that it depends on the rest of LLVM, so we need to basically build it first. This has been a major blocker for us to do this in the past. Perhaps we could investigate shipping FileCheck with LLVM such that we can use the apt-get installed version in the Docker image.

Or if we just want to run this on the bots, we can install it as part of the base setup.

However, I don't know that the tests in this PR are tests that provide value to the project. They're going to fail on almost every platform or different compiler version. They don't really test anything that's our responsibility to test. Any added assurance is outweighed by their brittleness.

Tests should fail only when the behavior under test changes.

I've given a lot of thought to adding these sorts of tests before, but the inherent non-portability and limited utility have always kept me from doing it.

Could you more thoroughly explain the value added by these tests and why they can't be established in other ways?

[ldionne]

I've given a lot of thought to adding these sorts of tests before, but the inherent non-portability and limited utility have always kept me from doing it.

Could you more thoroughly explain the value added by these tests and why they can't be established in other ways?

We've wished that we would have access to FileCheck in our tests several times in the past, usually when we implemented optimizations. I don't understand the purpose of the cmp_equal tests & friends, but IMO the test for libcxx/test/libcxx/algorithms/alg.modifying.operations/copy_move_codegen.sh.cpp is really great. It makes sure that we lower to memmove under some circumstances, which we don't have any way to test otherwise. We have similar optimizations in std::equal with memcmp IIRC, and those are basically impossible to test without looking at the assembly.

I see a lot of value in being able to perform these checks in specific (rather unusual) circumstances. I also agree that it's not ideal to write the tool ourselves when we have FileCheck. I see two options for moving forward with this:

1. Either make FileCheck independent of the rest of LLVM so we can build it from scratch every time easily
2. Or use a pre-installed version of FileCheck in the Docker image. This will require making FileCheck support optional in the test suite, because we can't guarantee that folks will have installed it.

IMO we should look into what's required to do (1).

[EricWF]

I've given a lot of thought to adding these sorts of tests before, but the inherent non-portability and limited utility have always kept me from doing it.
Could you more thoroughly explain the value added by these tests and why they can't be established in other ways?

We've wished that we would have access to FileCheck in our tests several times in the past, usually when we implemented optimizations. I don't understand the purpose of the cmp_equal tests & friends, but IMO the test for libcxx/test/libcxx/algorithms/alg.modifying.operations/copy_move_codegen.sh.cpp is really great. It makes sure that we lower to memmove under some circumstances, which we don't have any way to test otherwise.

What does it mean if that test starts failing? I would argue "not much".

We don't care that the functions call memmov, we care about the performance. If the compiler stops lowering to memmov, and instead lowers to faster instructions, do we care? No. Should we have a test that fails if such a change occurs? No.

What we're really trying to test is that we don't regress a benchmark. And that's tricky to do, so I understand the motivation to find another way to test that.

I see a lot of value in being able to perform these checks in specific (rather unusual) circumstances. I also agree that it's not ideal to write the tool ourselves when we have FileCheck. I see two options for moving forward with this:

1. Either make FileCheck independent of the rest of LLVM so we can build it from scratch every time easily
2. Or use a pre-installed version of FileCheck in the Docker image. This will require making FileCheck support optional in the test suite, because we can't guarantee that folks will have installed it.

Cool, let's look into #1.

But I'm pretty opposed to making the build+test iteration speed any slower by default. If we actually come up with a substantial amount of tests that need FileCheck, I would be happy to revisit it's necessity, but we're not there yet.

[ldionne]

In the interest of keeping the review queue small and relevant, I then suggest we close this PR as option (1) is investigated.

@philnik777 WDYT?

[philnik777]

In the interest of keeping the review queue small and relevant, I then suggest we close this PR as option (1) is investigated.

@philnik777 WDYT?

I've converted it to a draft for now to clear up the queue.

[ldionne]

Closing per discussion with Nikolas since we will most likely pursue installing FileCheck in the test suite instead. This can always be reopened if needed.

[ldionne]

Since using the real FileCheck still has not happened and there are many use cases for this patch, I think it would be reasonable to pursue this again. @philnik777 If you want to rebase this patch on top of main, I'll be happy to review it (likely when I get back from the holidays).

[ldionne]

Thanks a lot for reviving this! I really like this in general, but I do wonder about the value of some of the cmp_less.codegen tests you added.

[ldionne]

Can you rebase and apply this patch on the PR? That should fix CI and my review comments:

diff --git a/libcxx/test/CMakeLists.txt b/libcxx/test/CMakeLists.txt
index 5ab843e8c131..f4e577aed57d 100644
--- a/libcxx/test/CMakeLists.txt
+++ b/libcxx/test/CMakeLists.txt
@@ -42,7 +42,6 @@ add_custom_target(install-cxx-test-suite-prefix
                               cxx
                               cxx_experimental
                               cxx-modules
-                              check_output
                       COMMAND ${CMAKE_COMMAND} -E make_directory "${LIBCXX_TESTING_INSTALL_PREFIX}"
                       COMMAND "${CMAKE_COMMAND}"
                               -DCMAKE_INSTALL_COMPONENT=cxx-headers
diff --git a/libcxx/test/configs/cmake-bridge.cfg.in b/libcxx/test/configs/cmake-bridge.cfg.in
index 31ebcfe98215..3bbd1356ad20 100644
--- a/libcxx/test/configs/cmake-bridge.cfg.in
+++ b/libcxx/test/configs/cmake-bridge.cfg.in
@@ -31,4 +31,4 @@ config.substitutions.append(('%{lib-dir}', '@LIBCXX_TESTING_INSTALL_PREFIX@/@LIB
 config.substitutions.append(('%{module-dir}', '@LIBCXX_TESTING_INSTALL_PREFIX@/@LIBCXX_INSTALL_MODULES_DIR@'))
 config.substitutions.append(('%{test-tools-dir}', '@LIBCXX_TEST_TOOLS_PATH@'))
 config.substitutions.append(('%{benchmark_flags}', '-I @LIBCXX_BINARY_DIR@/test/benchmarks/google-benchmark/include -L @LIBCXX_BINARY_DIR@/test/benchmarks/google-benchmark/lib -L @LIBCXX_BINARY_DIR@/test/benchmarks/google-benchmark/lib64 -l benchmark'))
-config.substitutions.append(('%{check-output}', os.path.join('@CMAKE_BINARY_DIR@', 'bin/check_output') + " %s"))
+config.substitutions.append(('%{check-output}', os.path.join('@LIBCXX_BINARY_DIR@', 'bin/check_output') + " %s"))
diff --git a/libcxx/test/tools/check_output/CMakeLists.txt b/libcxx/test/tools/check_output/CMakeLists.txt
index e1fe1a018e00..aa33b2b1992f 100644
--- a/libcxx/test/tools/check_output/CMakeLists.txt
+++ b/libcxx/test/tools/check_output/CMakeLists.txt
@@ -1,15 +1,6 @@
 
 add_executable(check_output check_output.cpp)
 
-# Link against libc++
-if (TARGET cxx_shared)
-  target_link_libraries(check_output PRIVATE cxx_shared)
-elseif (TARGET cxx_static)
-  target_link_libraries(check_output PRIVATE cxx_static)
-else()
-  message(FATAL "Neither cxx_shared nor cxx_static are available to be linked against")
-endif()
-
 # put the binary into <build>/bin
 set_target_properties(check_output PROPERTIES RUNTIME_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/bin")
 
@@ -20,3 +11,5 @@ set_target_properties(check_output PROPERTIES
                       CXX_EXTENSIONS NO)
 
 target_compile_options(check_output PRIVATE -Werror=missing-prototypes)
+
+add_dependencies(cxx-test-depends check_output)

[ldionne]

Looks like you can't use <print> from the implementation of check_output itself.