Page MenuHomePhabricator

[NativePDB] Reconstruct FunctionDecl AST nodes from PDB debug info
ClosedPublic

Authored by zturner on Dec 6 2018, 1:06 PM.

Details

Summary

Previously we would create an lldb::Function object for each function parsed, but we would not add these to the clang AST. This is a first step towards getting local variable support working, as we first need an AST decl so that when we create local variable entries, they have the proper DeclContext.

Diff Detail

Repository
rL LLVM

Event Timeline

zturner created this revision.Dec 6 2018, 1:06 PM
zturner updated this revision to Diff 177039.Dec 6 2018, 1:38 PM

Clang-cl emits S_LOCAL symbols while MSVC emits S_REGREL32 and S_REGISTER symbols. So, to get more test coverage, I added an MSVC test as well. Also a little NFC cleanup in the main source file (basically just making sure these decls get added to the decl map).

aleksandr.urakov accepted this revision.Dec 6 2018, 10:39 PM

LGTM!

lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
577 ↗(On Diff #177039)

May be it would be worth to leave a TODO here (about searching a correct declaration context in the future)?

This revision is now accepted and ready to land.Dec 6 2018, 10:39 PM
labath added a comment.Dec 7 2018, 1:17 AM

Clang-cl emits S_LOCAL symbols while MSVC emits S_REGREL32 and S_REGISTER symbols. So, to get more test coverage, I added an MSVC test as well.

I don't want to block this patch, but my thoughts after reading this is that this is repeating the same mistakes that we did with dwarf. The reason we need so much to run our tests in so many configurations is that we haven't found a way way to handle the fact that some things can be expressed in multiple ways in the debug info other than compiling the source code with the compiler which happens to use that dialect. Then, if a later e.g. clang chooses to do the same thing that msvc does, we lose coverage here without anyone noticing. For low-level details like these, I believe a test where the type of the symbol is explicit would be more appropriate (for dwarf that might be a .s file, I don't know if that would work for pdbs).

zturner updated this revision to Diff 177243.Dec 7 2018, 10:01 AM

Add some comments regarding fixing DeclContext reconstruction.

zturner marked 2 inline comments as done.Dec 7 2018, 10:04 AM

Clang-cl emits S_LOCAL symbols while MSVC emits S_REGREL32 and S_REGISTER symbols. So, to get more test coverage, I added an MSVC test as well.

I don't want to block this patch, but my thoughts after reading this is that this is repeating the same mistakes that we did with dwarf. The reason we need so much to run our tests in so many configurations is that we haven't found a way way to handle the fact that some things can be expressed in multiple ways in the debug info other than compiling the source code with the compiler which happens to use that dialect. Then, if a later e.g. clang chooses to do the same thing that msvc does, we lose coverage here without anyone noticing. For low-level details like these, I believe a test where the type of the symbol is explicit would be more appropriate (for dwarf that might be a .s file, I don't know if that would work for pdbs).

I considered this, and we actually have a test that does (see s_constant.s). The reason I didn't do it here because my experience writing s_constant.s taught me just how difficult it is to write these assembly files.

However, now I have a new idea. We can teach llvm-pdbutil to dump its output exactly in llvm assembly format, so the output can just be copy-pasted into a .s file. Then, we can build something with MSVC, dump it to .s format, and paste individual records into a test and mix and match them as necessary to create all the interesting cases.

For the purposes of this test, we don't even need to run the program, so it actually doesn't even matter if the addresses in the debug info records are correct. All we care about is that the AST is reconstructed correctly. I'll add this to my todo list because I think it would be a really useful feature.

lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
577 ↗(On Diff #177039)

Let me know how the new diff looks.

zturner updated this revision to Diff 177244.Dec 7 2018, 10:07 AM
zturner marked an inline comment as done.

Removed accidentally added function.

aleksandr.urakov accepted this revision.Dec 7 2018, 10:29 AM

Now it looks more clear, thanks!

This revision was automatically updated to reflect the committed changes.

I considered this, and we actually have a test that does (see s_constant.s). The reason I didn't do it here because my experience writing s_constant.s taught me just how difficult it is to write these assembly files.

However, now I have a new idea. We can teach llvm-pdbutil to dump its output exactly in llvm assembly format, so the output can just be copy-pasted into a .s file. Then, we can build something with MSVC, dump it to .s format, and paste individual records into a test and mix and match them as necessary to create all the interesting cases.

For the purposes of this test, we don't even need to run the program, so it actually doesn't even matter if the addresses in the debug info records are correct. All we care about is that the AST is reconstructed correctly. I'll add this to my todo list because I think it would be a really useful feature.

That sounds like a really useful feature.

It looks like this might not be specific to your change as a few other tests are failing with a similar error. I'm looking into it.

I tracked this down - the failures are not due to this change (or any LLDB change). It's this change to the common CMake files in LLVM: https://reviews.llvm.org/D55056