[libcxx-commits] [libcxx] [libcxx] Test `await_suspend` control flow & `coro_await_suspend_destroy` attr (PR #152820)

[Nikolas Klauser via libcxx-commits]

philnik777 wrote:

> @philnik777, thanks for looking -- a few responses, please be understanding since this is my first time contributing to LLVM.
> 
>     1. This is an end-to-end test for the control flow of C++ coroutines. It does have a natural affinity for the `<coroutine>` header, but I will be the first to admit that the dependency on `libcxx` can be stubbed out with some effort.
> 
>     2. I would have preferred to put this in `clang/`, closer to the actual coroutine mechanics, but as far as I can tell, there is no place in `clang/` that can run "end-to-end" tests of this sort. The specific things that the `libcxx` testing framework gives is -- it compiles AND executes the code with a large matrix of flags & architectures, including `-fno-exceptions`. I don't see any examples of this being doable in any `clang/` tests.

It is quite funny to see that we have such good coverage that we're asked to run tests for other sub-projects. That may be an indication that other sub-projects need some more robust CI.

The main problem here is that the tests under `libcxx/` are supposed to test libc++. We have to maintain these tests, but they're not actually testing anything we care directly about. It's also very different from other tests, so most contributors (including me currently) have no idea what they're even looking at.

>     3. To your question about "golden", these traces are recording the expected sequencing of the various coroutine hooks. All this behavior is theoretically derivable from the standard, but it's not reasonable to manually test all the permutations. Recording & comparing traces is much more scalable.
> 
>     4. As it turns out the underlying implementation is complex enough that the code-local, "unit-style" tests of LLVM IR don't come close to catching all control flow bugs. Comparing traces caught some subtle and very problematic bugs during my work.
> 
>     5. If "golden" tests trigger an allergy, a watered-down version of this test is possible, where I just compare two trunk behaviors (`await_suspend` vs `await_suspend_destroy`), and drop the "change detector" part. But first, I would suggest that the coroutine stack maintainers weigh in on this, because I think they might see value in the "change detector" testing strategy for the core control flow.

I'm sure there is value. However, the libc++ test suite is usually not run against the rest of the compiler. So any breaks in LLVM would break our CI. This is just not an acceptable way to handle this, especially since these are (as far as I understand currently) golden tests - i.e. they may be broken without the changes that broke them being incorrect.

> I hope this gives some additional useful context! Please guide me in regards to these concrete decisions:
> 
>     * If this can't go in `libcxx/`, where in `llvm-project/` can I get a test to **run** on a reasonably broad test matrix of flags and runtimes, to make sure that the underlying coroutine compiler logic is robust?
> 
>     * While I agree that "change detector" tests in general are smelly, in this case, we're detecting changes from standard-mandated behavior, so I think it's actually rather good. How strongly do you insist on removing the "gold" part of the test flow? Are you open to a conversation with the coroutine folks first?

I'm happy to talk to people who work on coroutines. How happy I am about golden tests depend very much on how golden they actually are. If they're actually testing standards-mandated behaviour I wouldn't consider them golden - simply conformance tests. Though that still doesn't mean they should live inside `libcxx/`.


https://github.com/llvm/llvm-project/pull/152820