I’ve been looking at some of the test failures on Windows and this led me to realize that there are at least several tests that are duplicated between lldb-suite and the lit tests. This appears to have been on purpose circa 2016 as a proof of concept for moving tests from lldb-suite to lit. I think this is confusing and we should pick a set (lit or lldb-suite) and remove the second set. Also, if we decide to stick with the lit tests, I think they will need to be updated as right now they are not all functioning as expected.
For example, the test TestCallStdStringFunction exists both for lit and lldb-suite. It is expected to fail on Windows because windows does not correctly support expressions. However, the test fails in lldb-suite and passes in lit and it passes for the wrong reason (see details below). I suspect there may be other places in the duplicated tests where we think we’ve checked something, but we really haven’t validated it correctly.
My suggestion is that we remove the lit versions of the duplicated tests rather than fixing them as the lldb-suite set appears to be working correctly.
P.S. Here are the details on TestCallStdStringFunction:
Here is what the test attempts to do:
breakpoint set --file call-function.cpp --line 52
In the lldb-suite version these CHECKs would have verified the output of the print immediately, but because of how lit works, these are verified together at the end. Since the executable itself prints “Hello World” a couple of times, even though the print expressions fail, “Hello World” can be found twice in the output, so the test succeeds.
Moreover, the test sets a breakpoint that it expects to hit before calling the two print statements. In the lldb-suite version, the test verifies that the breakpoint was set, this version doesn’t and it happens to fail to set the breakpoint. So when the test is calling “print”, the executable has already run through the end, so even if expressions worked correctly on windows, this would have failed since we would have made the call after the executable finished. At the very least, this test needs an additional CHECK statement to verify that either the breakpoint was set or it was hit.
Looking at the other duplicated tests, we have the potential for similar issues. They also all use CHECK rather then CHECK-DAG, so we should at least update them to use CHECK-DAG.