Remove m_decl_objects and look up variables for ComiplerDecls directly

[spyffe]

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.

[spyffe]

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]

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]

Added a test case,

include/lldb/Symbol/ClangASTContext.h

     //----------------------------------------------------------------------
     // CompilerDecl override functions
     //----------------------------------------------------------------------
-    lldb::VariableSP
-    DeclGetVariable (void *opaque_decl) override;
-
-    void
-    DeclLinkToObject (void *opaque_decl, std::shared_ptr<void> object) override;
-    
     ConstString
     DeclGetName (void *opaque_decl) override;
 

     uint32_t                                        m_pointer_byte_size;
     bool                                            m_ast_owned;
     bool                                            m_can_evaluate_expressions;
-    std::map<void *, std::shared_ptr<void>>         m_decl_objects;
     // clang-format on
 private:
     //------------------------------------------------------------------

include/lldb/Symbol/CompilerDecl.h

     IsClang () const;
 
     //----------------------------------------------------------------------
-    // Object linked to the decl
-    //----------------------------------------------------------------------
-    lldb::VariableSP
-    GetAsVariable ();
-
-    //----------------------------------------------------------------------
     // Accessors
     //----------------------------------------------------------------------
     

include/lldb/Symbol/GoASTContext.h

         return ConstString();
     }
 
-    lldb::VariableSP
-    DeclGetVariable (void *opaque_decl) override
-    {
-        return lldb::VariableSP();
-    }
-
-    void
-    DeclLinkToObject (void *opaque_decl, std::shared_ptr<void> object) override
-    {
-    }
-
     //----------------------------------------------------------------------
     // CompilerDeclContext functions
     //----------------------------------------------------------------------

include/lldb/Symbol/JavaASTContext.h

     ConstString
     DeclGetName(void *opaque_decl) override;
 
-    lldb::VariableSP
-    DeclGetVariable(void *opaque_decl) override;
-
-    void
-    DeclLinkToObject(void *opaque_decl, std::shared_ptr<void> object) override;
-
     //----------------------------------------------------------------------
     // CompilerDeclContext functions
     //----------------------------------------------------------------------

include/lldb/Symbol/TypeSystem.h

     virtual ConstString
     DeclGetMangledName (void *opaque_decl);
 
-    virtual lldb::VariableSP
-    DeclGetVariable (void *opaque_decl) = 0;
-
-    virtual void
-    DeclLinkToObject (void *opaque_decl, std::shared_ptr<void> object) = 0;
-
     virtual CompilerDeclContext
     DeclGetDeclContext (void *opaque_decl);
 

packages/Python/lldbsuite/test/lang/c/inlines/TestInlines.py

+from lldbsuite.test import lldbinline
+from lldbsuite.test import decorators
+
+lldbinline.MakeInlineTest(__file__, globals(), [])

packages/Python/lldbsuite/test/lang/c/inlines/main.c

+#include <stdio.h>
+
+void test1(int) __attribute__ ((always_inline));
+void test2(int) __attribute__ ((always_inline));
+
+void test2(int b) {
+    printf("test2(%d)\n", b); //% self.expect("expression b", DATA_TYPES_DISPLAYED_CORRECTLY, substrs = ["42"])
+}
+
+void test1(int a) {
+    printf("test1(%d)\n",  a);
+    test2(a+1);//% self.dbg.HandleCommand("step")
+               //% self.expect("expression b", DATA_TYPES_DISPLAYED_CORRECTLY, substrs = ["24"])
+}
+
+int main() {
+    test2(42);
+    test1(23);
+}

source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp

                 bool variable_found = false;
                 for (CompilerDecl decl : found_decls)
                 {
-                    var = decl.GetAsVariable();
+                    for (size_t vi = 0, ve = vars->GetSize(); vi != ve; ++vi)
+                    {
+                        VariableSP candidate_var = vars->GetVariableAtIndex(vi);
+                        if (candidate_var->GetDecl() == decl)
+                        {
+                            var = candidate_var;
+                            break;
+                        }
+                    }
+
                     if (var)
                     {
                         variable_found = true;

source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

         m_decl_to_die[decl].insert(die.GetDIE());
         return decl;
     }
+    
+    if (DWARFDIE abstract_origin_die = die.GetReferencedDIE(DW_AT_abstract_origin))
+    {
+        clang::Decl *decl = GetClangDeclForDIE(abstract_origin_die);
+        m_die_to_decl[die.GetDIE()] = decl;
+        m_decl_to_die[decl].insert(die.GetDIE());
+        return decl;
+    }
 
     clang::Decl *decl = nullptr;
     switch (die.Tag())

source/Symbol/ClangASTContext.cpp

 //----------------------------------------------------------------------
 // CompilerDecl override functions
 //----------------------------------------------------------------------
-lldb::VariableSP
-ClangASTContext::DeclGetVariable (void *opaque_decl)
-{
-    if (llvm::dyn_cast<clang::VarDecl>((clang::Decl *)opaque_decl))
-    {
-        auto decl_search_it = m_decl_objects.find(opaque_decl);
-        if (decl_search_it != m_decl_objects.end())
-            return std::static_pointer_cast<Variable>(decl_search_it->second);
-    }
-    return VariableSP();
-}
 
-void
-ClangASTContext::DeclLinkToObject (void *opaque_decl, std::shared_ptr<void> object)
-{
-    if (m_decl_objects.find(opaque_decl) == m_decl_objects.end())
-        m_decl_objects.insert(std::make_pair(opaque_decl, object));
-}
-
 ConstString
 ClangASTContext::DeclGetName (void *opaque_decl)
 {

source/Symbol/CompilerDecl.cpp

     return m_type_system->DeclGetMangledName(m_opaque_decl);
 }
 
-lldb::VariableSP
-CompilerDecl::GetAsVariable ()
-{
-    return m_type_system->DeclGetVariable(m_opaque_decl);
-}
-
 CompilerDeclContext
 CompilerDecl::GetDeclContext() const
 {

source/Symbol/JavaASTContext.cpp

     return ConstString();
 }
 
-lldb::VariableSP
-JavaASTContext::DeclGetVariable(void *opaque_decl)
-{
-    return lldb::VariableSP();
-}
-
-void
-JavaASTContext::DeclLinkToObject(void *opaque_decl, std::shared_ptr<void> object)
-{
-}
-
 std::vector<CompilerDecl>
 JavaASTContext::DeclContextFindDeclByName(void *opaque_decl_ctx, ConstString name, const bool ignore_imported_decls)
 {

source/Symbol/Variable.cpp

 CompilerDecl
 Variable::GetDecl ()
 {
-    CompilerDecl decl;
     Type *type = GetType();
-    if (type)
-    {
-        decl = type->GetSymbolFile()->GetDeclForUID(GetID());
-        if (decl)
-            decl.GetTypeSystem()->DeclLinkToObject(decl.GetOpaqueDecl(), shared_from_this());
-    }
-    return decl;
+    return type ? type->GetSymbolFile()->GetDeclForUID(GetID()) : CompilerDecl();
 }
 
 void