This is an archive of the discontinued LLVM Phabricator instance.

[NativePDB] Support type lookup by name
ClosedPublic

Authored by zturner on Oct 22 2018, 10:08 AM.

Details

Summary

This is the minimal set of functionality necessary to support type lookup by name in the native PDB plugin.

For the purposes of testing I decided to bypass lldb-test and go with a scripted lldbinit file. I'm sticking with this approach wherever possible to prove that this is "true" cross-platform debugger functionality. However, there are definite limitations. For example, the output format of type lookup has some limitations. It doesn't print the layout of the type in any kind of detail (e.g. field offsets), it doesn't support lookup by regex, it doesn't print the underlying type of an enumeration, it doesn't support limiting the scope of the search to specific kinds of types, so there is definitely room for lldb-test to come into the picture here to expand the coverage since we have full control over the output format.

I tried to think of some interesting test cases to exercise some edge cases here, but I welcome some ideas for other interesting cases. I consciously avoided things like bit fields, member functions, pointers to members, and virtual bases because there was a balancing act between implementing a useful piece of functionality and keeping the patch small enough that it can be understandable enough to review meaningfully.

Welcome any feedback. Hopefully this is getting to the point that maybe it's hackable by others as well, although I'm definitely going to continue working on it.

Diff Detail

Repository
rLLDB LLDB

Event Timeline

zturner created this revision.Oct 22 2018, 10:08 AM
zturner edited the summary of this revision. (Show Details)Oct 22 2018, 10:08 AM
lemo added a comment.Oct 22 2018, 2:01 PM

Nice :)

lldb/lit/SymbolFile/NativePDB/tag-types.cpp
87 ↗(On Diff #170447)
  • at least one virtual function + override?
  • at least one method returning void?
94 ↗(On Diff #170447)

pointer to itself too?

110 ↗(On Diff #170447)

just an idea - add a virtual inheritance variation?

131 ↗(On Diff #170447)

a few suggestions for additional things to cover:

  • local classes
  • lambdas
  • instantiating class and function templates
    • explicit specializations
    • for classes, partial specializations
  • namespaces
  • anonymous namespace
zturner added inline comments.Oct 22 2018, 2:30 PM
lldb/lit/SymbolFile/NativePDB/tag-types.cpp
87 ↗(On Diff #170447)

The reason I didn't add any tests for methods is because I didn't add anything in the implementation to parse method records either. As I said in the patch description, it was a balance between providing some piece of useful functionality while keeping the patch size down. So I tried to limit the scope to just fields minus bitfields, since there's some extra complexity there that I wanted to punt on for now. The reason I have the constructor is because it was required in order to initialize the reference member, otherwise it wouldn't compile.

110 ↗(On Diff #170447)

Virtual inheritance is a good followup, but if you look at UdtRecordCompleter.cpp and Ctrl+F for VirtualBaseClassRecord, I basically treat them as regular base classes and I put a fixme to handle the virtual aspects of the base. So that's a known problem right now.

131 ↗(On Diff #170447)

Some of these I could probably handle right now. I tried to keep the scope of the patch to "types which would be named", because it makes it easy to write tests (the test can just do lookup by name, basically a WinDbg dt test.). That makes local classes, lambdas, anonymous namespace, and anything to do with functions out of scope.

lemo accepted this revision.Oct 22 2018, 3:42 PM

Looks good to me, minor notes inline.

lldb/lit/SymbolFile/NativePDB/tag-types.cpp
131 ↗(On Diff #170447)

k. how about more basic stuff like?

  • pointer to pointer
  • const/volatile
lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
313 ↗(On Diff #170447)

char

335 ↗(On Diff #170447)

int64_t ?

344 ↗(On Diff #170447)

unsigned int64_t ?

660 ↗(On Diff #170447)

minor: = 0; ?
(to make any potential mistakes more obvious)

687 ↗(On Diff #170447)

assert size > 0 ?

lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp
189 ↗(On Diff #170447)

nit: add empty line at the end of file

lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.h
26 ↗(On Diff #170447)

nit: vertical space between { and the next line

This revision is now accepted and ready to land.Oct 22 2018, 3:42 PM
aleksandr.urakov accepted this revision.Oct 23 2018, 8:11 AM

Looks good to me, thank you!

This revision was automatically updated to reflect the committed changes.

For the purposes of testing I decided to bypass lldb-test and go with a scripted lldbinit file. I'm sticking with this approach wherever possible to prove that this is "true" cross-platform debugger functionality. However, there are definite limitations. For example, the output format of type lookup has some limitations. It doesn't print the layout of the type in any kind of detail (e.g. field offsets), it doesn't support lookup by regex, it doesn't print the underlying type of an enumeration, it doesn't support limiting the scope of the search to specific kinds of types, so there is definitely room for lldb-test to come into the picture here to expand the coverage since we have full control over the output format.

When implementing the debug_names accelerator tables, I also needed to test the lookup funtionality. That's how lldb-test symbols -find=(function|namespace|...) came to be. However, there I needed to just test that a type is found and not any of it's details, so I just used whatever dumping method we had implemented already. It would definitely be possible to change this dumping format to print more detailed (and FileCheckable) information.

BTW, it might be interesting to take some of the tests in lit/SymbolFile/DWARF/find-*** and add a RUN: clang-cl ... line to them, to verify that the same types/namespaces/variables are found regardless of whether we use DWARF or PDB (right now they test that .debug_names and manual dwarf indexes return the same results).