This is an archive of the discontinued LLVM Phabricator instance.

Remove m_decl_objects and look up variables for ComiplerDecls directly
Needs ReviewPublic

Authored by spyffe on May 18 2016, 4:00 PM.

Details

Summary

m_decl_objects is not sound when there are multiple instances of an inline function. The reason is that the clang::FunctionDecl is unique to the single abstract origin of all the inlined instances, and has one set of unique variables, but the VariableSP is different for each inlined instance.

The map m_decl_objects was introduced by LLDB r247746 which introduced a test case with some namespace lookups. I am putting this up for review to make sure that this fix (which works fine with that test case) also doesn't break anything else that might have been addressed by m_decl_objects.

Diff Detail

Repository
rL LLVM

Event Timeline

spyffe updated this revision to Diff 57699.May 18 2016, 4:00 PM
spyffe retitled this revision from to Remove m_decl_objects and look up variables for ComiplerDecls directly.
spyffe updated this object.
spyffe added reviewers: paulherman, sivachandra, jingham.
spyffe set the repository for this revision to rL LLVM.
spyffe added a subscriber: lldb-commits.

Added a few inline comments to make the patch's intent more understandable.

include/lldb/Symbol/ClangASTContext.h
1221

We remove this map and all accessors to it because its only purpose was the lookup that's been replaced.

source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
1262

Here, we already have a list of variables in scope with the right name (returned from GetInScopeVariableList()). So we just have to pick the one with the right VarDecl.

If there are any other variables that aren't in that list that we need to consider, this is something we need to know about.

source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
3610

The point of this code is to ensure that the variables are not duplicated. There is only one FunctionDecl, so there needs to be only one VarDecl, the one for the abstract origin. Otherwise the FunctionDecl contains multiple variables with the same name, messing up lookup.

labath added a subscriber: labath.May 19 2016, 5:21 AM

Siva and Paul are not active on LLDB any more. I don't know if they are still keeping an eye out on this, but I think you can go ahead with this if they don't respond. It should be fine as long as the test passes.

OTOH, if it is possible to write a test for the new code introduced here (I don't know enough about this are to be able to tell if that's the case), then I think it would be even better.

spyffe updated this revision to Diff 57879.May 19 2016, 4:37 PM

Added a test case,