[Lldb-commits] [PATCH] D56418: Change lldb-test to use ParseAllDebugSymbols instead of ParseDeclsForContext

[Zachary Turner via Phabricator via lldb-commits]

zturner created this revision.
zturner added reviewers: labath, clayborg, aleksandr.urakov, asmith, amccarth.
Herald added subscribers: JDevlieghere, aprantl.

`ParseDeclsForContext` was originally created to serve the very specific case where the context is a function block.  It was never intended to be used for arbitrary DeclContexts, however due to the generic name, the DWARF and PDB plugins implemented it in this way "just in case".  Then, `lldb-test` came along and decided to use it in that way.

Related to this, there are a set of functions in the `SymbolFile` class interface whose requirements and expectations are not documented.  For example, if you call `ParseCompileUnitFunctions`, there's an inherent requirement that you create entries in the underlying clang AST for these functions as well as their signature types, because in order to create an `lldb_private::Function` object, you have to pass it a `CompilerType` for the parameter representing the signature.

On the other hand, there is no similar requirement (either inherent or documented) if one were to call `ParseDeclsForContext`.  Specifically, if one calls `ParseDeclsForContext`, and some variable declarations, types, and other things are added to the clang AST, is it necessary to create `lldb::Variable`, `lldb::Type, etc objects representing them?  Nobody knows.  There is, however, an //accidental// requirement, because since all of the plugins implemented this just in case, `lldb-test` came along and used `ParsedDeclsForContext`, and then wrote check lines that depended on this.

When I went to try and implemented the NativePDB reader, I did not adhere to this (in fact, from a layering perspective I went out of my way to avoid it), and as a result the existing DIA PDB tests don't work when the native PDB reader is enabled, because they expect that calling ParseDeclsForContext will modify the *module's* view of symbols, and not just the internal AST.

All of this confusion, however, can be avoided if we simply stick to using `ParseDeclsForContext` for its original intended use case (blocks), and use a different function (`ParseAllDebugSymbols`) for its intended use case which is, unsuprisingly, to parse all the debug symbols (which is all `lldb-test` really wanted to do anyway).

In the future, I would like to change `ParseDeclsForContext` to `ParseDeclsForFunctionBlock`, then delete all of the dead code inside that handles other types of DeclContexts (and probably even assert if the DeclContext is anything other than a block).

A few PDB tests needed to be fixed up as a result of this, and this also exposed a couple of bugs in the DIA PDB reader (doesn't matter much since it should be going away soon, but worth mentioning) where the appropriate AST entries weren't being created always.


https://reviews.llvm.org/D56418

Files:
  lldb/lit/SymbolFile/PDB/enums-layout.test
  lldb/lit/SymbolFile/PDB/type-quals.test
  lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
  lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.h
  lldb/tools/lldb-test/lldb-test.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D56418.180576.patch
Type: text/x-patch
Size: 11295 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20190107/55841924/attachment-0001.bin>