This is an archive of the discontinued LLVM Phabricator instance.

[NativePDB] Support local variables
ClosedPublic

Authored by zturner on Dec 11 2018, 1:23 PM.

Details

Summary

This patch adds support for parsing and evaluating local variables. using the native pdb plugin.

While implementing this, I came up with several new ideas for increasing test coverage throughout LLDB (not limited to this plugin), including:

  • We should start extending AST tests to other types of targets and debug info, not just windows / native pdb. This uncovered a few bugs for me here that allowed me to proceed.
  • We should have comprehensive test coverage of the DWARFExpression evaluator in lldb/Expression/DWARFExpression.cpp. This would allow us to improve test coverage of scenarios that require compilers to emit specific debug info, and allow us to do it in a way that doesn't require that debug info to have been emitted by some compiler.
  • We should add options to lldb-test that will allow it to call into SymbolFile::ResolveSymbolContext and dump the output in some format that is interesting for file checking. This function is at the heart of a lot of logic, and if it doesn't work, many things will fail. Being able to test all of the edge cases of this function would weed out a lot of problems.

Anyway, for now these are all just ideas. For this patch, I've just implemented support for local variables and a test to make sure it works. One interesting thing: With the old PDB plugin, this test takes over 1 minute to run. With the new PDB plugin, it takes about 2 seconds.

Diff Detail

Repository
rLLDB LLDB

Event Timeline

zturner created this revision.Dec 11 2018, 1:23 PM
labath added inline comments.Dec 12 2018, 1:59 AM
lldb/lit/SymbolFile/NativePDB/local-variables.cpp
31–37 ↗(On Diff #177768)

I'm not thrilled by all of the hard-coded information (line numbers, the default format of stop-information printing) here, which is irrelevant for the tested functionality here. This will make it very hard to update this test if there is ever a need for (perhaps one would like to remove the line break in the RUN: command once the env LLDB_USE_NATIVE_PDB_READER=1 part disappears), or the default way of printing stop information in lldb changes. It doesn't seem like a maintainable long term strategy.

lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
1929 ↗(On Diff #177768)

Huh?

llvm/include/llvm/Support/BinaryStreamArray.h
142 ↗(On Diff #177768)

It looks like this could be done (and tested, in llvm) separately.

zturner marked an inline comment as done.Dec 12 2018, 7:34 AM
zturner added inline comments.
lldb/lit/SymbolFile/NativePDB/local-variables.cpp
31–37 ↗(On Diff #177768)

I felt the same way TBH, but wanted to see what the feedback would be like. What do you think about just removing all the backtrace lines from the check output and only checking the print commands and their output?

zturner marked an inline comment as done.Dec 12 2018, 7:35 AM
zturner added inline comments.
lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
1929 ↗(On Diff #177768)

Oops :-/

labath added inline comments.Dec 12 2018, 7:42 AM
lldb/lit/SymbolFile/NativePDB/local-variables.cpp
31–37 ↗(On Diff #177768)

I think that would be much better.

zturner updated this revision to Diff 177877.Dec 12 2018, 10:13 AM

Updated test to be less dependent on the exact format of the source.

The test looks really good now.

This looks fine to me, though I don't know much about PBDs. I still think the seemingly random change in BinaryStreamArray would be better served as a standalone patch.

lldb/lit/SymbolFile/NativePDB/Inputs/local-variables.lldbinit
19–22 ↗(On Diff #177877)

I am not sure if you care about the distinction between "expr" and "frame variable" here, but if you don't, these could be displayed in one shot with frame variable Param1 Param2 Local1 Local2 (or even just frame variable which would display all locals).

lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
1929 ↗(On Diff #177768)

It looks like you still haven't removed this.

This revision was not accepted when it landed; it landed in state Needs Review.Dec 13 2018, 10:21 AM
This revision was automatically updated to reflect the committed changes.
$ ":" "RUN: at line 5"
$ "E:\build_slave\lldb-x64-windows-ninja\build\bin\lldb.EXE" "-S" "E:/build_slave/lldb-x64-windows-ninja/llvm/tools/lldb/lit\lit-lldb-init" "-f" "E:\build_slave\lldb-x64-windows-ninja\build\tools\lldb\lit\SymbolFile\NativePDB\Output\local-variables.cpp.tmp.exe" "-s" "E:\build_slave\lldb-x64-windows-ninja\llvm\tools\lldb\lit\SymbolFile\NativePDB/Inputs/local-variables.lldbinit"
$ "E:\build_slave\lldb-x64-windows-ninja\build\bin\FileCheck.EXE" "E:\build_slave\lldb-x64-windows-ninja\llvm\tools\lldb\lit\SymbolFile\NativePDB\local-variables.cpp"
# command stderr:
E:\build_slave\lldb-x64-windows-ninja\llvm\tools\lldb\lit\SymbolFile\NativePDB\local-variables.cpp:154:16: error: CHECK-NEXT: expected string not found in input

// CHECK-NEXT: Dumping clang ast for 7 modules.

               ^

<stdin>:140:1: note: scanning from here

Dumping clang ast for 8 modules.

^


error: command failed with exit status: 1