LGTM

This is a good change, we do see bugs that are sometimes not totally deterministic so this should hopefully reduce those.

LGTM

Eric, this is breaking the green dragon build bots for LLDB, see the first log here: http://lab.llvm.org:8080/green/view/LLDB/job/lldb-cmake/5581/

In D72391#1809751, @teemperor wrote:

I don't know if this needs a unit test where we call the constructor and explicitly check the name is the one we passed in. Let me know if you think this would make sense.

In D72391#1811589, @teemperor wrote:

@clayborg We can easily append the ptr value to the display name. All names should always be unique as long as there is one target, but in the off-chance that one isn't unique it might be useful.

Perhaps it makes sense to modify the dump() method to also display the new name?

LGTM

In D70846#1809837, @labath wrote:

This looks fine to me. @shafik, @teemperor, do you have any more comments?

I am curious what prompted this set of changes, they look sensible but I don't see how we can do anything useful if we end up in this state.

This is a great change! Moving to stricter casts clarifies intent and reduces the chance that later changes can introduce bugs accidentally.

• Fix typo
• Adjust error handing
• clang-format test

In D71378#1781616, @teemperor wrote:

I wonder if we have a way to fix this from with LLDB. Having Clang code that is only tested in LLDB is always a bit weird.

Otherwise the idea itself LGTM, thanks for working on this (and reducing the test case to that!)

It is not clear to me we could use these in a test because I don't see a reason for the numbers to stay stable and therefore a useful measure.

In D71306#1779379, @labath wrote:

Besides lldb developers, another category we should consider are llvm (or clang or lld, ...) developers who know nothing about lldb, but they ended up building it because they specified LLVM_ENABLE_PROJECTS=all. These people probably don't want to figure out how to install libedit, or how to disable it, but if the default config "just works" for them, they'd be happy to build lldb and make sure their patch doesn't break it. Optimizing for this experience may be even more important than core lldb developers, since there's fewer of the latter and they usually just configure lldb once.

Besides my comments about different scenarios this LGTM

Apologies for wacky C++ code that follows but will this also work for the following cases:

LGTM

In D70524#1767775, @aprantl wrote:

@shafik Can you speak about whether this feature ("auto" return type in DWARF v5 section 5.2) would be useful for LLDB?

LGTM

Changing -static_cast<uint64_t>(1) to std::numeric_limits<uint64_t>::max()

In D70846#1767455, @labath wrote:

I guess you were building darwin binaries, right? If that's the case, then you weren't using this code at all, as apple uses AppleIndex by default. The test in question uses all three indexes (it forces their generation by altering compile flags) and the failures you were seeing were most likely coming the "manual" case...

Yeah, there was indeed some ugly code there, nice work!

In D70846#1766598, @labath wrote:

In D70846#1766204, @shafik wrote:

In D70846#1763731, @labath wrote:

There's lldb-shell :: SymbolFile/DWARF/find-basic-function.cpp, which probably didn't get run for you as you didn't have lld enabled (cmake -DLLVM_ENABLE_PROJECTS=...;lld). You'll need to update that test to match the new behavior, but other than that, I think this is a good change.

So with this change for the find-basic-function.cpp test I no longer see any results for the full case so we should at least generate a case that has results for the full case.

There's an additional check in that test which does a search by a mangled name (search for FULL-MANGLED), and this one does return some result. If this patch lands, I'm not sure if there's any other kind of a "full" lookup that we could perform. eFunctionNameTypeFull is documented as: ... For C this is the same as just the name of the function For C++ this is the mangled or demangled version of the mangled name..., which appears like we should support searching by *de*mangled names. However, I'm not sure if that is actually a good idea. Implementing that for the manual index would be simple enough, but that is something that the apple index could never support (in fact, I think I remember that the manual index once supported searching by demangled names, but then I removed this ability for consistency when adding debug_names support).

That said, I think it may be interesting to add a test searching for an extern "C" symbol (which has no "mangled" name), as right now it's not clear if it will show up because of function_fullnames.Find or function_basenames.Find...

In D70846#1763731, @labath wrote:

There's lldb-shell :: SymbolFile/DWARF/find-basic-function.cpp, which probably didn't get run for you as you didn't have lld enabled (cmake -DLLVM_ENABLE_PROJECTS=...;lld). You'll need to update that test to match the new behavior, but other than that, I think this is a good change.

In D70846#1763802, @teemperor wrote:

This LGTM, but the TODO directly above the change in ClangExpressionDeclMap.cpp worries me a bit. I am not sure how good our test coverage is for calling functions when there are instance methods in scope (e.g., we are in instance method and try to call another instance method).

Applying clang-format

• Adjust comment
• matching_lambda argument is now FunctionSP to match ForeachFunction signature.

In D69913#1739579, @jingham wrote:

Why not have the FindFunctions lambda take a FunctionSP? It would be easy to get the Function name out of the function in the lambda, and that would make the function more general (and also match what the Foreach does...

@aprantl you will want to take a second look at this, I did some major refactoring.

After updating branch the stepping test started failing. It was failing because the __invoke case was subtly broken in this patch. Fixing that case opened up a lot of refactoring opportunities. So this patch ends up being a large refactor and in many ways a simplification now.

Updating based on comments

• Adding documentation
• using startswith(...)
• Updating initialization to use = instead of {}

As far as I can tell we don't have coverage for GetBaseClassPath it would be good to add it while we are refactoring this code.

Using llvm-dwarfdump instead of dwarfdump

• Updated test to add dwarfdump test

-Add test to clarify what we depending wrt to debug info

In D68961#1714241, @labath wrote:

In D68961#1713566, @shafik wrote:

We also have the 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> 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> and dsymutil <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.

Minor fixes.

In D68961#1711537, @labath wrote:

In D68961#1711407, @shafik wrote:

In D68961#1709708, @clayborg wrote:

Have many compilers supported DW_AT_export_symbols for a while now? If not, are there any serious issues introduced here that would change debugger behavior if this attribute is not emitted by a compiler? Or is this a new fix in clang that was recently introduced in order to fix an issue when debugging in lldb?

We don't except any regressions for code compiled with older compilers. We are fixing the case that unnamed classes are identified as anonymous. The anonymous classes cases should be caught in older revisions in ClangASTContext::AddFieldToRecordType which does a Rec->setAnonymousStructOrUnion(true) for those cases.

Would it make sense to write a test in asm then? That way it would be obvious exactly what kind of debug info is being tested, and you could ensure both old&new compiler behavior is covered.

In D68961#1709708, @clayborg wrote:

Have many compilers supported DW_AT_export_symbols for a while now? If not, are there any serious issues introduced here that would change debugger behavior if this attribute is not emitted by a compiler? Or is this a new fix in clang that was recently introduced in order to fix an issue when debugging in lldb?

• Simplified DumpFromSymbolFile() based on comments
• Reused -name option in lldb-test and to replace -symbol-name

This change broek the`TestDataFormatterInvalidStdUniquePtr.py` test, see: http://lab.llvm.org:8080/green/view/LLDB/job/lldb-cmake/2411/testReport/

SymbolFile/Breakpad.symtab.test is failing on green dragon cmake build see: http://lab.llvm.org:8080/green/view/LLDB/job/lldb-cmake/2378/

In D67994#1697981, @labath wrote:

In D67994#1695172, @shafik wrote:

In D67994#1692645, @labath wrote:

Maybe this is my fault since I'm the one who introduced the first bunch of arguments here IIRC, but anyway, I have a feeling all of these dumping options are getting out of hand. Looking at the argument list, you'd expect that -dump-ast, and -dump-clang-ast do something completely different, but they actually print the exact same AST, only one allows you to print a subset of it, while the other one doesn't.

Given that the ast dumping is currently incompatible with all/most of the other dumping options anyway, maybe we should turn over a clean slate, and implement a new subcommand specifically dedicated to dumping clang ast (as opposed to the internal lldb representations of types/functions/..., which is what the "symbols" subcommand started out as, and which is what most of its options are dedicated to).

OR, and this would-be super cool, if it was actually possible, we could try to make ast-dumping compatible with the existing searching options. Currently, if you type lldb-test symbols -find=type -name=foo it will search for types named "foo", and print some summary of the lldb object representing that type. What if we made it so that adding -dump-ast to that command caused the tool to dump the *AST* of the types it has found (e.g., in addition to the previous summary)? I think that would make the behavior of the tool most natural to a new user, but I am not sure whether that would actually fit in with your goals here...

The output is actually quite different for example given:

struct A {
  struct {
      int x;
  };
} a;

the output of lldb-test symbols -dump-ast anon.o is:

Module: anon.o
struct A;

while the output of lldb-test symbols -dump-clang-ast anon.o is:

Module: anon.o
A
CXXRecordDecl 0x7ff3698262c8 <<invalid sloc>> <invalid sloc> struct A definition
|-DefinitionData pass_in_registers aggregate standard_layout trivially_copyable pod trivial literal
...

Given they are both working with the Symbol File I believe they both belong under the symbol command. I feel like -dump-ast is a little misleading but -dump-clang-ast feels good.

You're right about. I'm sorry, I should have checked what -dump-ast does. It definitely is confusing that it does something completely different than the "dump ast" lldb command.

But what about the second part of my comment? I think it would still be nice if the lldb-test symbols options could be somehow factored into two groups:

• a group which selects *what* to dump: this would be -find, -name, etc.
• a group which selects *how* to dump it: -dump-clang-ast, -dump-ast :/, the default mode
  
  would it be somehow possible to hook in this logic into the findTypes function in lldb-test, so that instead of calling TypeMap::Dump, it does something else if -dump-clang-ast flag is present (this "else" resuling in the printing of clang ast)?

LGTM

See the same break on green dragon as well: http://lab.llvm.org:8080/green/view/LLDB/job/lldb-cmake/2360/testReport/LLDB/SymbolFile_NativePDB/s_constant_cpp/

I reverted this change b/c the green dragon bots have been red for too long.

@wallace it looks like TestPlatformClient.py is failing on osx see green dragon log here: http://lab.llvm.org:8080/green/view/LLDB/job/lldb-cmake/2317/testReport/junit/lldb-Suite/functionalities_gdb_remote_client/TestPlatformClient_py/

In D67994#1692645, @labath wrote:

Maybe this is my fault since I'm the one who introduced the first bunch of arguments here IIRC, but anyway, I have a feeling all of these dumping options are getting out of hand. Looking at the argument list, you'd expect that -dump-ast, and -dump-clang-ast do something completely different, but they actually print the exact same AST, only one allows you to print a subset of it, while the other one doesn't.

Given that the ast dumping is currently incompatible with all/most of the other dumping options anyway, maybe we should turn over a clean slate, and implement a new subcommand specifically dedicated to dumping clang ast (as opposed to the internal lldb representations of types/functions/..., which is what the "symbols" subcommand started out as, and which is what most of its options are dedicated to).

OR, and this would-be super cool, if it was actually possible, we could try to make ast-dumping compatible with the existing searching options. Currently, if you type lldb-test symbols -find=type -name=foo it will search for types named "foo", and print some summary of the lldb object representing that type. What if we made it so that adding -dump-ast to that command caused the tool to dump the *AST* of the types it has found (e.g., in addition to the previous summary)? I think that would make the behavior of the tool most natural to a new user, but I am not sure whether that would actually fit in with your goals here...

Thank you for doing this, this looks like an excellent start from a quick review.