[PATCH] D54187: Add debuginfo-tests that use cdb on Windows

Wed Nov 7 08:54:23 PST 2018

zturner added a comment.

In https://reviews.llvm.org/D54187#1290247, @aprantl wrote:

> > I think the only way to realistically make this work for all platforms would be to separate the source file from the input/output. The source file would be the test case, and if you wanted to support a different debugger you would need to supply a different input/output file..
>
> I don't necessarily agree with that statement. The debuginfo-tests use only a very small subset of debugger functionality that includes
>
> - setting breakpoints
> - printing the value of integer variables
> - continuing to the next breakpoint
>
>   I'm genuinely curious what is so different about the Windows debugger that it couldn't be wrapped in a translation layer like `llgdb.py` that abstracts these three operations. This would cover a large set of the existing tests. I'm fine with having a few extra tests that test something that only works on a specific platform here and there, but I really don't want us to grow separate platform-specific testsuites. Inevitably, someone working on platform A will fix a general bug in LLVM and then only add a test for platform A and that's bad for everyone. What I'm trying to avoid is a situation like the debug info tests in LLVM that are almost entirely x86-specific, even if they are testing stuff that happens at the IR level.

I don't think we want tests that are limited to that amount of simplicity.  We don't just want to print integers, we'd like to also print callstacks.  And objects.  And other things that aren't integers.  A cdb call stack looks like this:

000000c4`aa35f730 00007ffc`8944bc72 : 00000294`3b6e0ae8 00000294`3b6e0a98 cccccccc`cccccccc cccccccc`cccccccc : MSVCP140D!_Cnd_wait+0x20 [f:\dd\vctools\crt\crtw32\stdcpp\thr\cond.c @ 106] 
08 000000c4`aa35f760 00007ffc`89450a54 : 00000294`3b6e0ae8 00000294`3b6e0a98 cccccccc`cccccccc cccccccc`cccccccc : liblldb!std::_Cnd_waitX+0x32 [c:\program files (x86)\microsoft visual studio\2017\professional\vc\tools\msvc\14.15.26726\include\thr\xthread @ 97] 
09 000000c4`aa35f790 00007ffc`8944122d : 00000294`3b6e0ae8 000000c4`aa35f828 00000000`00000000 00000000`00000000 : liblldb!std::condition_variable::wait+0x54 [c:\program files (x86)\microsoft visual studio\2017\professional\vc\tools\msvc\14.15.26726\include\mutex @ 714] 
0a 000000c4`aa35f7d0 00007ffc`89440897 : 00000294`3b6e09e0 000000c4`aa35fb78 00000000`00000000 00000000`00000000 : liblldb!lldb_private::Listener::GetEventInternal+0x20d [d:\src\llvm-mono\lldb\source\core\listener.cpp @ 367] 
0b 000000c4`aa35f8e0 00007ffc`8939b70e : 00000294`3b6e09e0 000000c4`aa35fa78 000000c4`aa35fb78 cccccccc`cccccccc : liblldb!lldb_private::Listener::GetEvent+0x57 [d:\src\llvm-mono\lldb\source\core\listener.cpp @ 404] 
0c 000000c4`aa35f930 00007ffc`8939b118 : 00000294`3b6de700 cccccccc`cccccccc cccccccc`cccccccc cccccccc`cccccccc : liblldb!lldb_private::Debugger::DefaultEventHandler+0x27e [d:\src\llvm-mono\lldb\source\core\debugger.cpp @ 1546] 
0d 000000c4`aa35fc30 00007ffc`8960cf62 : 00000294`3b6de700 00000294`00000001 cccccccc`cccccccc cccccccc`cccccccc : liblldb!lldb_private::Debugger::EventHandlerThread+0x28 [d:\src\llvm-mono\lldb\source\core\debugger.cpp @ 1600]

Then you need a way to abstract over the command lines needed to generate the executables (`clang-cl` and `lld-link` use different flag syntax than `clang++` etc).  Then there's the issue of when a test is using Microsoft specific extensions.  At the end of all of this, it's going to take a lot of effort to implement this layer of abstraction that is ultimately going to be subverted for a large number of tests anyway when something doesn't fit cleanly into the abstraction.  I think there is also the issue of how much actual overlap there is between the set of interesting test cases for DWARF / Itanium ABI and PDB / Microsoft ABI.  Many issues that we would want to add tests for would be surrounding fixes specific to the way we emit the PDB or that are constructed due to knowledge of how we emit CodeView in certain situations.  And none of those test cases will be interesting to abstract over.

Finally, by strictly limiting the amount of output we're checking against, we can be too permissive.  For example, we have a command up there that checks against the line `g`.  But this will match any line that has the letter `g` in it, which is extremely permissive.  It would be more useful if the line said `DEBUGGER: 0:000> g`

The line that says `CHECK: a = 0n2` will also match if the variable happens to be 23 or any number of other values.  So we'd really like to be able to be more precise here.

So I'm not convinced there is a lot of added value, or at the very least, I don't believe it to be worth the cost.  Especially since as far as I can tell, nobody has even run debuginfo-tests since late August, because it was actually broken by r341135 on August 30 (fixed in r346060 yesterday)

https://reviews.llvm.org/D54187