This is an archive of the discontinued LLVM Phabricator instance.

Change lldb-test to use ParseAllDebugSymbols instead of ParseDeclsForContext
ClosedPublic

Authored by zturner on Jan 7 2019, 3:30 PM.

Details

Summary

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.

Diff Detail

Repository
rLLDB LLDB

Event Timeline

zturner created this revision.Jan 7 2019, 3:30 PM
zturner edited the summary of this revision. (Show Details)Jan 7 2019, 3:41 PM
labath accepted this revision.Jan 8 2019, 2:38 AM

The lldb-test change itself looks fine. I don't feel like I know the full impact of the proposed follow-up changes (but they sounds reasonable).

lldb/lit/SymbolFile/PDB/enums-layout.test
3 ↗(On Diff #180576)

I guess this is because the order in the two plugins ends up being different?

This revision is now accepted and ready to land.Jan 8 2019, 2:38 AM
amccarth added inline comments.Jan 8 2019, 2:32 PM
lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
318 ↗(On Diff #180576)

Is it appropriate to hard code that language type here? I realize we don't have good PDB support of many other languages, but what about C? Or various versions of C++?

zturner marked an inline comment as done.Jan 8 2019, 2:56 PM
zturner added inline comments.
lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
318 ↗(On Diff #180576)

Well, the question is really whether any of those other languages would ever use a different type system. IMHO, the Language enumeration specifies too many unnecessary values (YAGNI), and it really should just be "some flavor of C" and "Swift", since no other type systems are even supported (and even Swift is downstream, so probably shouldn't even be in the upstream repo). The entire plugin (and indeed, pretty much all of LLDB) is already coupled very tightly with clang, and what flavor of C++ we're using won't matter for which TypeSystem we need (which is going to be ClangASTContext every single time, no exceptions).

This revision was automatically updated to reflect the committed changes.