[lldb] Check Decl kind when completing -flimit-debug-info types

[labath]

Summary

The search for the complete class definition can also produce entries
which are not of the expected type. This can happen for instance when
there is a function with the same name as the class we're looking up
(which means that the class needs to be disambiguated with the
struct/class tag in most contexts).

Previously we were just picking the first Decl that the lookup returned,
which later caused crashes or assertion failures if it was not of the
correct type. This patch changes that to search for an entry of the
correct type.

[teemperor]

Is it too late to claim that I did this on purpose to see if anyone noticed the bug? It probably is :/

lldb/test/API/functionalities/limit-debug-info/main.cpp

27

Nit: the public isn't necessary and you also omitted above.

28

I'm probably missing something obvious here, but why give every class in the test a defaulted constexpr constructor?

[shafik]

Minor comment but LGTM, nice fix!

lldb/test/API/functionalities/limit-debug-info/main.cpp

30

Just to clarify we func_shadow::One is being shadowed here. Maybe a comment would help clarify the what is going on.

[labath]

In D85904#2215700, @teemperor wrote:

Is it too late to claim that I did this on purpose to see if anyone noticed the bug? It probably is :/

:P

lldb/test/API/functionalities/limit-debug-info/main.cpp

28

I was trying to guarantee that these variables get constant-initialized because earlier versions of this test just inspected a static binary without running it. But then one of the cases actually required running code so it no longer serves any purpose. I'll just delete them...

lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp

if (td && md && md->IsForcefullyCompleted()) {
     Expected<DeclContext *> dc_or_err = ImportContext(td->getDeclContext());
     if (!dc_or_err)
       return dc_or_err.takeError();
     Expected<DeclarationName> dn_or_err = Import(td->getDeclName());
     if (!dn_or_err)
       return dn_or_err.takeError();
     DeclContext *dc = *dc_or_err;
     DeclContext::lookup_result lr = dc->lookup(*dn_or_err);
-    if (lr.size()) {
-      clang::Decl *lookup_found = lr.front();
-      RegisterImportedDecl(From, lookup_found);
-      m_decls_to_ignore.insert(lookup_found);
-      return lookup_found;
-    } else
-      LLDB_LOG(log, "[ClangASTImporter] Complete definition not found");
+    for (clang::Decl *candidate : lr) {
+      if (candidate->getKind() == From->getKind()) {
+        RegisterImportedDecl(From, candidate);
+        m_decls_to_ignore.insert(candidate);
+        return candidate;
+      }
+    }
   }
 
   return ASTImporter::ImportImpl(From);
 }
 
 void ClangASTImporter::ASTImporterDelegate::ImportDefinitionTo(
     clang::Decl *to, clang::Decl *from) {
   // We might have a forward declaration from a shared library that we

lldb/test/API/functionalities/limit-debug-info/TestLimitDebugInfo.py

def test_one_and_two_debug(self):
         self.expect_expr("array_of_one[2].member", result_value="174")
         self.expect_expr("array_of_two[2].one[2].member", result_value="174")
         self.expect_expr("array_of_two[2].member", result_value="274")
 
         self.expect_expr("get_one().member", result_value="124")
         self.expect_expr("get_two().one().member", result_value="124")
         self.expect_expr("get_two().member", result_value="224")
 
+        self.expect_expr("shadowed_one.member", result_value="47")
+        self.expect_expr("shadowed_one.one", result_value="142")
+
     @skipIf(bugnumber="pr46284", debug_info="gmodules")
     @skipIfWindows # Clang emits type info even with -flimit-debug-info
     def test_two_debug(self):
         self.build(dictionary=dict(STRIP_ONE="1"))
         target = self.dbg.CreateTarget(self.getBuildArtifact("a.out"))
 
         self._check_debug_info_is_limited(target)
 

lldb/test/API/functionalities/limit-debug-info/main.cpp

 #include "onetwo.h"
 
 struct InheritsFromOne : One {
-  constexpr InheritsFromOne() = default;
   int member = 47;
 } inherits_from_one;
 
 struct InheritsFromTwo : Two {
-  constexpr InheritsFromTwo() = default;
   int member = 47;
 } inherits_from_two;
 
 struct OneAsMember {
-  constexpr OneAsMember() = default;
   member::One one;
   int member = 47;
 } one_as_member;
 
 struct TwoAsMember {
-  constexpr TwoAsMember() = default;
   member::Two two;
   int member = 47;
 } two_as_member;
 
 array::One array_of_one[3];
 array::Two array_of_two[3];
 
 result::One get_one() { return result::One(124); }
 result::Two get_two() { return result::Two(224); }
 
+// Note that there's also a function with the name func_shadow::One.
+struct ShadowedOne : func_shadow::One {
+  int member = 47;
+} shadowed_one;
+
 int main() { return get_one().member; }

lldb/test/API/functionalities/limit-debug-info/one.cpp

 #include "onetwo.h"
 
 One::~One() = default;
 member::One::~One() = default;
 array::One::~One() = default;
 
 result::One::One(int member) : member(member) {}
 result::One::~One() = default;
+
+void func_shadow::One(int) {}
+func_shadow::One::~One() = default;
+void func_shadow::One(float) {}

lldb/test/API/functionalities/limit-debug-info/onetwo.h

 
 struct Two {
   int member;
   Two(int member);
   One one() const;
   virtual ~Two();
 };
 } // namespace result
+
+namespace func_shadow {
+void One(int);
+struct One {
+  int one = 142;
+  constexpr One() = default;
+  virtual ~One();
+};
+void One(float);
+} // namespace func_shadow