[lldb/DWARF] Fix linking direction in CopyUniqueClassMethodTypes

[labath]

Summary

IIUC, the purpose of CopyUniqueClassMethodTypes is to link together
class definitions in two compile units so that we only have a single
definition of a class. It does this by adding entries to the die_to_type
and die_to_decl_ctx maps.

However, the direction of the linking seems to be reversed. It is taking
entries from the class that has not yet been parsed, and copying them to
the class which has been parsed already -- i.e., it is a very
complicated no-op.

Changing the linking order allows us to revert the changes in D13224
(while keeping the associated test case passing), and is sufficient to
fix PR54761, which was caused by an undesired interaction with that
patch.

[shafik]

This mostly makes sense, the purpose of the || alternate_defn was not clear to me either.

[clayborg]

So I believe the reason we need to be able to add methods to a class is for templates. Templates in DWARF are really poorly emitted all in the name of saving space. There is no full definition of template classes that get emitted, just a class definition with _only_ the methods that the user used in the current compile unit. DWARF doesn't really support emitting a templatized definition of a class in terms of a type T, it will always emit instantiations of the type T directly. So in LLDB we must create a template class like "std::vector<int>" and say it has no member functions, and then add each member function as a type specific specialization due to how the types must be created in the clang::ASTContext.

in one DWARF compile unit you end up having say "std::vector<int>::clear()" and in another you would have "std::vector<int>::erase()". In order to make the most complete definition of template types we need to find all "std::vector<int>" definitions and manually add any method that we haven't already seen, otherwise we end up not being able to call methods that might exist. Of course calling template functions is already fraught with danger since most are inlined if they come from the STL, but if the user asks for the class definition for "std::vector<int>", we should show all that we can. Can you make sure this patch doesn't hurt that functionality? We must have a test for it.

So we can't just say we will accept just one class definition if we have template types. Given this information, let me know what you think of your current patch

[dblaikie]

I take it no tests fail upon removing this condition? Any hints in version history about its purpose?

[labath]

In D124370#3472394, @clayborg wrote:

So I believe the reason we need to be able to add methods to a class is for templates. Templates in DWARF are really poorly emitted all in the name of saving space. There is no full definition of template classes that get emitted, just a class definition with _only_ the methods that the user used in the current compile unit. DWARF doesn't really support emitting a templatized definition of a class in terms of a type T, it will always emit instantiations of the type T directly. So in LLDB we must create a template class like "std::vector<int>" and say it has no member functions, and then add each member function as a type specific specialization due to how the types must be created in the clang::ASTContext.

in one DWARF compile unit you end up having say "std::vector<int>::clear()" and in another you would have "std::vector<int>::erase()". In order to make the most complete definition of template types we need to find all "std::vector<int>" definitions and manually add any method that we haven't already seen, otherwise we end up not being able to call methods that might exist. Of course calling template functions is already fraught with danger since most are inlined if they come from the STL, but if the user asks for the class definition for "std::vector<int>", we should show all that we can. Can you make sure this patch doesn't hurt that functionality? We must have a test for it.

So we can't just say we will accept just one class definition if we have template types. Given this information, let me know what you think of your current patch

I think I'm confused. I know we're doing some funny stuff with class templates, and I'm certain I don't fully understand how it works. However, I am also pretty sure your description is not correct either. A simple snippet like std::vector<int> v; will produce a definition for the vector<int> class, and that definition will include the declarations for erase, clear, and any other non-template member function, even though the code definitely does not call them. And this makes perfect sense to me given how DWARF (and clang) describes only template instantiations -- so the template instantiation vector<int> is treated the same way as a class vector_int with the same interface.

What you're describing resembles the treatment of template member functions -- those are only emitted in the compile units that they are used. This is very unfortunate for us, though I think it still makes sense from the DWARF perspective. However:
a) In line with the previous paragraph -- this behavior is the same for template class instantiations and regular classes. IOW, the important question is whether the method is a template, not whether its enclosing class is
b) (precisely for this reason) we currently completely ignore member function templates when parsing classes
Given all of that, I don't see how templates are relevant for this patch. *However*, please read on.

In D124370#3472555, @dblaikie wrote:

I take it no tests fail upon removing this condition? Any hints in version history about its purpose?

No tests broke with that. My first archaeology expedition did not find anything, but now I've tried it once more, and I found D13224. That diff is adding this condition (to support -flimit-debug-info, no less) and it even comes with a test. That test still exists, but it was broken over time (in several ways). Once I fix it so that it tests what it was supposed to test, I see that my patch breaks it.

I'm now going to try to figure out how does that code actually work...

[labath]

Here's the updated version. I'll place my new findings into the patch
description.

[dblaikie]

In D124370#3474824, @labath wrote:

In D124370#3472394, @clayborg wrote:

So I believe the reason we need to be able to add methods to a class is for templates. Templates in DWARF are really poorly emitted all in the name of saving space. There is no full definition of template classes that get emitted, just a class definition with _only_ the methods that the user used in the current compile unit. DWARF doesn't really support emitting a templatized definition of a class in terms of a type T, it will always emit instantiations of the type T directly. So in LLDB we must create a template class like "std::vector<int>" and say it has no member functions, and then add each member function as a type specific specialization due to how the types must be created in the clang::ASTContext.

in one DWARF compile unit you end up having say "std::vector<int>::clear()" and in another you would have "std::vector<int>::erase()". In order to make the most complete definition of template types we need to find all "std::vector<int>" definitions and manually add any method that we haven't already seen, otherwise we end up not being able to call methods that might exist. Of course calling template functions is already fraught with danger since most are inlined if they come from the STL, but if the user asks for the class definition for "std::vector<int>", we should show all that we can. Can you make sure this patch doesn't hurt that functionality? We must have a test for it.

So we can't just say we will accept just one class definition if we have template types. Given this information, let me know what you think of your current patch

I think I'm confused. I know we're doing some funny stuff with class templates, and I'm certain I don't fully understand how it works. However, I am also pretty sure your description is not correct either. A simple snippet like std::vector<int> v; will produce a definition for the vector<int> class, and that definition will include the declarations for erase, clear, and any other non-template member function, even though the code definitely does not call them. And this makes perfect sense to me given how DWARF (and clang) describes only template instantiations -- so the template instantiation vector<int> is treated the same way as a class vector_int with the same interface.

What you're describing resembles the treatment of template member functions -- those are only emitted in the compile units that they are used. This is very unfortunate for us, though I think it still makes sense from the DWARF perspective. However:
a) In line with the previous paragraph -- this behavior is the same for template class instantiations and regular classes. IOW, the important question is whether the method is a template, not whether its enclosing class is
b) (precisely for this reason) we currently completely ignore member function templates when parsing classes
Given all of that, I don't see how templates are relevant for this patch. *However*, please read on.

Yep, that mostly lines up with my understand.

Re: member function templates: These aren't the only things that vary between copies of a type in the DWARF. The other things that vary are implicit special member functions ({default,move,copy} ctor, dtor, {copy,move} assignment operator}) - these will only be present when instantiated - and nested types (though that's perhaps less of an issue for LLDB, not sure). I have made an argument that functions with "auto" return type should be treated this way (omitted unless the definition is instantiated and the return type is deduced/resolved) too - but for now they are not (they are always included, albeit with the "auto" return type on the declaration, and then the definition DIE has the real/deduced return type).

In D124370#3472555, @dblaikie wrote:

I take it no tests fail upon removing this condition? Any hints in version history about its purpose?

No tests broke with that. My first archaeology expedition did not find anything, but now I've tried it once more, and I found D13224. That diff is adding this condition (to support -flimit-debug-info, no less) and it even comes with a test. That test still exists, but it was broken over time (in several ways). Once I fix it so that it tests what it was supposed to test, I see that my patch breaks it.

I'm now going to try to figure out how does that code actually work...

Should the cleanups you found for the previous test case be included in this patch (or a separate pre/post-commit?)? The description in the updated patch sounds plausible - though delves into parts of lldb I'm not especially qualified to make a firm assessment on.

[labath]

@clayborg, @shafik, what do you make of this version?

In D124370#3481338, @dblaikie wrote:

Should the cleanups you found for the previous test case be included in this patch (or a separate pre/post-commit?)? The description in the updated patch sounds plausible - though delves into parts of lldb I'm not especially qualified to make a firm assessment on.

I already checked those in --> rG089a1d9deba5

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

if (tag == DW_TAG_subprogram || tag == DW_TAG_inlined_subroutine) {
                 die.GetOffset(), tag, DW_TAG_value_to_name(tag));
           }
         }
       } else if (is_cxx_method) {
         // Look at the parent of this DIE and see if is is a class or
         // struct and see if this is actually a C++ method
         Type *class_type = dwarf->ResolveType(decl_ctx_die);
         if (class_type) {
-          bool alternate_defn = false;
           if (class_type->GetID() != decl_ctx_die.GetID() ||
               IsClangModuleFwdDecl(decl_ctx_die)) {
-            alternate_defn = true;
 
             // We uniqued the parent class of this function to another
             // class so we now need to associate all dies under
             // "decl_ctx_die" to DIEs in the DIE for "class_type"...
             DWARFDIE class_type_die = dwarf->GetDIE(class_type->GetID());
 
             if (class_type_die) {
               std::vector<DWARFDIE> failures;
if (tag == DW_TAG_subprogram || tag == DW_TAG_inlined_subroutine) {
                   ") has no decl\n",
                   die.GetID(), abs_die.GetOffset());
             }
             type_handled = true;
           } else {
             CompilerType class_opaque_type =
                 class_type->GetForwardCompilerType();
             if (TypeSystemClang::IsCXXClassType(class_opaque_type)) {
-              if (class_opaque_type.IsBeingDefined() || alternate_defn) {
+              if (class_opaque_type.IsBeingDefined()) {
                 if (!is_static && !die.HasChildren()) {
                   // We have a C++ member function with no children (this
                   // pointer!) and clang will get mad if we try and make
                   // a function that isn't well formed in the DWARF, so
                   // we will just skip it...
                   type_handled = true;
                 } else {
-                  bool add_method = true;
-                  if (alternate_defn) {
-                    // If an alternate definition for the class exists,
-                    // then add the method only if an equivalent is not
-                    // already present.
-                    clang::CXXRecordDecl *record_decl =
-                        m_ast.GetAsCXXRecordDecl(
-                            class_opaque_type.GetOpaqueQualType());
-                    if (record_decl) {
-                      for (auto method_iter = record_decl->method_begin();
-                           method_iter != record_decl->method_end();
-                           method_iter++) {
-                        clang::CXXMethodDecl *method_decl = *method_iter;
-                        if (method_decl->getNameInfo().getAsString() ==
-                            attrs.name.GetStringRef()) {
-                          if (method_decl->getType() ==
-                              ClangUtil::GetQualType(clang_type)) {
-                            add_method = false;
-                            LinkDeclContextToDIE(method_decl, die);
-                            type_handled = true;
-
-                            break;
-                          }
-                        }
-                      }
-                    }
-                  }
-
-                  if (add_method) {
-                    llvm::PrettyStackTraceFormat stack_trace(
-                        "SymbolFileDWARF::ParseType() is adding a method "
-                        "%s to class %s in DIE 0x%8.8" PRIx64 " from %s",
-                        attrs.name.GetCString(),
-                        class_type->GetName().GetCString(), die.GetID(),
-                        dwarf->GetObjectFile()
-                            ->GetFileSpec()
-                            .GetPath()
-                            .c_str());
+                      dwarf->GetObjectFile()->GetFileSpec().GetPath().c_str());
 
-                    const bool is_attr_used = false;
-                    // Neither GCC 4.2 nor clang++ currently set a valid
-                    // accessibility in the DWARF for C++ methods...
-                    // Default to public for now...
-                    if (attrs.accessibility == eAccessNone)
-                      attrs.accessibility = eAccessPublic;
 
-                    clang::CXXMethodDecl *cxx_method_decl =
-                        m_ast.AddMethodToCXXRecordType(
-                            class_opaque_type.GetOpaqueQualType(),
-                            attrs.name.GetCString(), attrs.mangled_name,
-                            clang_type, attrs.accessibility, attrs.is_virtual,
-                            is_static, attrs.is_inline, attrs.is_explicit,
-                            is_attr_used, attrs.is_artificial);
 
-                    type_handled = cxx_method_decl != nullptr;
-                    // Artificial methods are always handled even when we
-                    // don't create a new declaration for them.
-                    type_handled |= attrs.is_artificial;
 
-                    if (cxx_method_decl) {
-                      LinkDeclContextToDIE(cxx_method_decl, die);
 
-                      ClangASTMetadata metadata;
-                      metadata.SetUserID(die.GetID());
 
-                      if (!object_pointer_name.empty()) {
-                        metadata.SetObjectPtrName(
-                            object_pointer_name.c_str());
-                        LLDB_LOGF(log,
-                                  "Setting object pointer name: %s on method "
-                                  "object %p.\n",
-                                  object_pointer_name.c_str(),
-                                  static_cast<void *>(cxx_method_decl));
-                      }
-                      m_ast.SetMetadata(cxx_method_decl, metadata);
-                    } else {
-                      ignore_containing_context = true;
-                    }
-                  }
-                }
+                      metadata.SetObjectPtrName(object_pointer_name.c_str());
               } else {
                 // We were asked to parse the type for a method in a
                 // class, yet the class hasn't been asked to complete
                 // itself through the clang::ExternalASTSource protocol,
                 // so we need to just have the class complete itself and
                 // do things the right way, then our
                 // DIE should then have an entry in the
                 // dwarf->GetDIEToType() map. First
DWARFASTParserClang *src_dwarf_ast_parser =
       static_cast<DWARFASTParserClang *>(
           SymbolFileDWARF::GetDWARFParser(*src_class_die.GetCU()));
   DWARFASTParserClang *dst_dwarf_ast_parser =
       static_cast<DWARFASTParserClang *>(
           SymbolFileDWARF::GetDWARFParser(*dst_class_die.GetCU()));
   auto link = [&](DWARFDIE src, DWARFDIE dst) {
     SymbolFileDWARF::DIEToTypePtr &die_to_type =
         dst_class_die.GetDWARF()->GetDIEToType();
-    clang::DeclContext *src_decl_ctx =
-        src_dwarf_ast_parser->m_die_to_decl_ctx[src.GetDIE()];
-    if (src_decl_ctx)
-      dst_dwarf_ast_parser->LinkDeclContextToDIE(src_decl_ctx, dst);
+    clang::DeclContext *dst_decl_ctx =
+        dst_dwarf_ast_parser->m_die_to_decl_ctx[dst.GetDIE()];
+    if (dst_decl_ctx)
+      src_dwarf_ast_parser->LinkDeclContextToDIE(dst_decl_ctx, src);
 
     if (Type *src_child_type = die_to_type[src.GetDIE()])
       die_to_type[dst.GetDIE()] = src_child_type;
   };
 
   // Now do the work of linking the DeclContexts and Types.
   if (fast_path) {
     // We can do this quickly.  Just run across the tables index-for-index

lldb/test/API/lang/cpp/incomplete-types/members/Makefile

+CXX_SOURCES := main.cpp f.cpp g.cpp
+
+include Makefile.rules
+
+# Force main.cpp to be built with no debug information
+main.o: CFLAGS = $(CFLAGS_NO_DEBUG)
+
+# And force -flimit-debug-info on the rest.
+f.o: CFLAGS_EXTRAS += $(LIMIT_DEBUG_INFO_FLAGS)
+g.o: CFLAGS_EXTRAS += $(LIMIT_DEBUG_INFO_FLAGS)

lldb/test/API/lang/cpp/incomplete-types/members/TestCppIncompleteTypeMembers.py

+"""
+Test situations where we don't have a definition for a type, but we have (some)
+of its member functions.
+"""
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class TestCppIncompleteTypeMembers(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    def test(self):
+        self.build()
+        lldbutil.run_to_source_breakpoint(self, "// break here",
+                lldb.SBFileSpec("f.cpp"))
+
+        # Sanity check that we really have to debug info for this type.
+        this = self.expect_var_path("this", type="A *")
+        self.assertEquals(this.GetType().GetPointeeType().GetNumberOfFields(),
+                0, str(this))
+
+        self.expect_var_path("af.x", value='42')
+
+        lldbutil.run_break_set_by_source_regexp(self, "// break here",
+                extra_options="-f g.cpp")
+        self.runCmd("continue")
+
+        self.expect_var_path("ag.a", value='47')

lldb/test/API/lang/cpp/incomplete-types/members/a.h

+#ifndef A_H
+#define A_H
+
+class A {
+public:
+  A();
+  virtual void anchor();
+  int f();
+  int g();
+
+  int member = 47;
+};
+
+#endif

lldb/test/API/lang/cpp/incomplete-types/members/f.cpp

+#include "a.h"
+
+int A::f() {
+  struct Af {
+    int x, y;
+  } af{42, 47};
+  return af.x + af.y; // break here
+}

lldb/test/API/lang/cpp/incomplete-types/members/g.cpp

+#include "a.h"
+
+int A::g() {
+  struct Ag {
+    int a, b;
+  } ag{47, 42};
+  return ag.a + ag.b; // break here
+}

lldb/test/API/lang/cpp/incomplete-types/members/main.cpp

+#include "a.h"
+
+A::A() = default;
+void A::anchor() {}
+
+int main() {
+  A().f();
+  A().g();
+}