[Lldb-commits] [PATCH] D68961: Add support for DW_AT_export_symbols for anonymous structs

Mon Oct 21 12:59:45 PDT 2019

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