shafik added a comment. In D68961#1714241 <https://reviews.llvm.org/D68961#1714241>, @labath wrote:
> In D68961#1713566 <https://reviews.llvm.org/D68961#1713566>, @shafik wrote: > > > We also have the lldb-cmake-matrix > > <http://lab.llvm.org:8080/green/view/LLDB/job/lldb-cmake-matrix/> bot which > > runs the test suite using older versions of clang which is there for > > exactly this purpose to catch regressions due to features not supported by > > older compiler versions. Which would catch any regressions here. > > > I'm afraid I have to disagree with that. Like I said the last time <D66370 > <https://reviews.llvm.org/D66370>> when this topic came up, I don't think > that this "matrix" testing is the way to test dwarf features. The "matrix" > bot is very useful to catch regressions in parsing old debug info (because > there will always be some quirk that you forget to think of), but I don't > think there should be any piece of code that is _only_ tested this way, > particularly when it is very easy to do so (like it is here). > > As you're probably aware, there's an "end-to-end testing" discussion going on > on the mailing lists right now. One of the main questions being debated is > whether it is ok to have a test which checks the assembly generated from some > c(++) source code. And there's a lot of pushback saying that this is testing > too much. However, even such a test is peanuts compared to > clang-ast-from-dwarf-unamed-and-anon-structs.cpp. Even though it's one of the > simplest lldb test we have, it runs the compiler front end, back end *and* > assembler, and then reads the output hoping that the pipeline has generated > the thing it wants to test. (And btw, it will fail on windows.) It doesn't > even check that the compiler has generated the thing it wants to check, so if > the compiler output later changes to not include DW_AT_export_symbols, it > will start to test something completely different, leaving the code added > here uncovered, unless someone remembers to add a new row to the matrix bot. > > Not that this is very likely to happen in this particular case, but these > kinds of changes happen all the time. Just last week, llvm started generating > a very different style of location lists. Both lldb <rL374769 > <https://reviews.llvm.org/rL374769>> and dsymutil <D69005 > <https://reviews.llvm.org/D69005>> had to be fixed to handle that. Both > patches had tests, which were not based on running clang to generate the > debug info (the dsymutil test had a checked in binary, which I am not a fan > of, but that's a different story). Even so, it's very likely that this has > regressed our coverage of the previous forms of location lists because we had > no tests (except some basic ones I've added two or three months ago) which > tested this code directly. And we only know about this because the initial > implementation was wrong/incomplete, so we noticed the breakage. > > So, overall, I still think we should have an even more targeted test here. > Maybe it's not fair to ask you to add this kind of a test for the "old" style > structs because that is past and not really "on you". However, I think we > should be trying to add these kinds of tests for new features whenever is > possible (another thing -- I imagine debugging failures from the matrix bot > can be hard, though I've fortunately had not had to do that yet). I've been > trying to do that for the past year, and while it takes a while to get used > to, after some time I've gotten fairly efficient at generating/reducing "raw" > dwarf. Besides being able to generate "stable" tests, this also enables one > to generate tests for "incorrect" dwarf, which is one of the things which is > impossible with the "clang -g" approach. When the I added the feature to the front end tests were added to verify that `DW_AT_export_symbols` is being generated for anonymous structs in D66605 <https://reviews.llvm.org/D66605> and D66667 <https://reviews.llvm.org/D66667> so if this regresses in the front-end we will catch it vis these tests. So as far I can tell we have tests at every point it can regress. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68961/new/ https://reviews.llvm.org/D68961 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
