[DWARFASTParserClang] Strengthen incomplete type handling.

[sivachandra]

Summary

This change fixes pr24916. As associated test has been added.

[zturner]

Can you change the name of the folder to something more descripive than pr24916. For example, you could put pr24916 in a comment in the test, but the test itself could have a descriptive name. Enrico at Apple just went and did a sweep removing rdar's from bug trackers. The public tracker is not exactly the same since it's, well, public. But still it requires more mental (and physical) cycles to parse than it does to parse something like limit-debug-info.

[sivachandra]

Rename test directory to limit-debug-info.

[zturner]

Makefile stuff looks good. Is the plan to do the same thing for the previous patch you submitted last week? What's the status of that?

[sivachandra]

About Makefile changes in other tests, I will one day clean all places where no-limit-debug-info shows up in the Makefile.
Just in case, zturner's comment about Makefiles on the other change came in a day after I landed the change. So, I will have to do it in another change.

[clayborg]

So this fix is really to take care of the case where a class methods might be defined on only some class instances? Can you explain more of what this fixes?

[sivachandra]

I will use an example to explain the problem that this patch is trying to solve.

class.h:

class C
{
public:
    C () {
        c = 0;
    }

    virtual int method();

private:
    int c;
};

class.cpp:

#include "class.h"

int
C::method ()
{
    return c;
}

main.cpp:

#include "class.h"

int
main ()
{
  C c;

  return c.method();
}

Compile as follows:

g++|clang++ -g class.cpp main.cpp # on mac you will require -flimit-debug-info

The interesting DWARF for the CU main.cpp is here:

< 1><0x00000029>    DW_TAG_class_type
                    DW_AT_name                  "C"
                    DW_AT_declaration           yes(1)
                    DW_AT_sibling               <0x00000041>
< 2><0x00000030>      DW_TAG_subprogram
                      DW_AT_external              yes(1)
                      DW_AT_name                  "C"
                      DW_AT_decl_file             0x00000001 class.h
                      DW_AT_decl_line             0x00000005
                      DW_AT_accessibility         DW_ACCESS_public
                      DW_AT_declaration           yes(1)
                      DW_AT_object_pointer        <0x0000003a>
< 3><0x0000003a>        DW_TAG_formal_parameter
                        DW_AT_type                  <0x00000041>
                        DW_AT_artificial            yes(1)

As you can see, there is a method declaration under a class declaration (not definition). The same CU also has subprogram DIEs as follows:

< 1><0x00000047>    DW_TAG_subprogram
                    DW_AT_specification         <0x00000030>
                    DW_AT_inline                DW_INL_declared_not_inlined
                    DW_AT_object_pointer        <0x00000055>
                    DW_AT_sibling               <0x0000005f>
< 2><0x00000055>      DW_TAG_formal_parameter
                      DW_AT_name                  "this"
                      DW_AT_type                  <0x0000005f>
                      DW_AT_artificial            yes(1)
< 1><0x0000005f>    DW_TAG_const_type
                    DW_AT_type                  <0x00000041>
< 1><0x00000064>    DW_TAG_subprogram
                    DW_AT_abstract_origin       <0x00000047>
                    DW_AT_linkage_name          "_ZN1CC2Ev"
                    DW_AT_low_pc                0x00400640
                    DW_AT_high_pc               <offset-from-lowpc>32
                    DW_AT_frame_base            len 0x0001: 9c: DW_OP_call_frame_cfa
                    DW_AT_object_pointer        <0x00000087>
                    DW_AT_GNU_all_call_sites    yes(1)
                    DW_AT_sibling               <0x00000090>
< 2><0x00000087>      DW_TAG_formal_parameter
                      DW_AT_abstract_origin       <0x00000055>
                      DW_AT_location              len 0x0002: 9168: DW_OP_fbreg -24

The DWARF for the CU class.cpp has the full definition of the class (with all its methods), but does not have subprogram DIEs which refer to the methods in the class definition. So, when stopped in C::C, the context resolves to an incomplete class for which a full definition also exists. This patch is essentially saying, "use the complete definition of the class when you find its incomplete definition."

[clayborg]

Thanks for the info, now the patch makes more sense. That is some really lame DWARF I must say, but alas we must deal with it. Looks good.

[sivachandra]

Rebase.

source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

if (die)
                                                                                  type_name_cstr,
                                                                                  type_sp->GetID());
                             }
 
                             // We found a real definition for this type elsewhere
                             // so lets use it and cache the fact that we found
                             // a complete type for this die
                             dwarf->GetDIEToType()[die.GetDIE()] = type_sp.get();
+                            clang::DeclContext *defn_decl_ctx = GetCachedClangDeclContextForDIE(
+                                dwarf->DebugInfo()->GetDIE(DIERef(type_sp->GetID())));
+                            if (defn_decl_ctx)
+                                LinkDeclContextToDIE(defn_decl_ctx, die);
                             return type_sp;
                         }
                     }
                     assert (tag_decl_kind != -1);
                     bool clang_type_was_created = false;
                     clang_type.SetCompilerType(&m_ast, dwarf->GetForwardDeclDieToClangType().lookup (die.GetDIE()));
                     if (!clang_type)
                     {
if (die)
                             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())
                                     {
+                                        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"...
                                         SymbolFileDWARF *class_symfile = NULL;
                                         DWARFDIE class_type_die;
 
                                         SymbolFileDWARFDebugMap *debug_map_symfile = dwarf->GetDebugMapSymfile();
                                         if (debug_map_symfile)

                                         }
                                         type_handled = true;
                                     }
                                     else
                                     {
                                         CompilerType class_opaque_type = class_type->GetForwardCompilerType ();
                                         if (ClangASTContext::IsCXXClassType(class_opaque_type))
                                         {
-                                            if (class_opaque_type.IsBeingDefined ())
+                                            if (class_opaque_type.IsBeingDefined () || alternate_defn)
                                             {
-                                                // Neither GCC 4.2 nor clang++ currently set a valid accessibility
-                                                // in the DWARF for C++ methods... Default to public for now...
-                                                if (accessibility == eAccessNone)
-                                                    accessibility = eAccessPublic;
-
                                                 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
                                                 {
-                                                    clang::CXXMethodDecl *cxx_method_decl;
+                                                    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() == std::string(type_name_cstr))
+                                                                {
+                                                                    if (method_decl->getType() == ClangASTContext::GetQualType(clang_type))
+                                                                    {
+                                                                        add_method = false;
+                                                                        LinkDeclContextToDIE(ClangASTContext::GetAsDeclContext(method_decl), die);
+                                                                        type_handled = true;
+
+                                                                        break;
+                                                                    }
+                                                                }
+                                                            }
+                                                        }
+                                                    }
+
+                                                    if (add_method)
+                                                    {
                                                         // REMOVE THE CRASH DESCRIPTION BELOW
                                                         Host::SetCrashDescriptionWithFormat ("SymbolFileDWARF::ParseType() is adding a method %s to class %s in DIE 0x%8.8" PRIx64 " from %s",
                                                                                              type_name_cstr,
                                                                                              class_type->GetName().GetCString(),
                                                                                              die.GetID(),
                                                                                              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 (accessibility == eAccessNone)
+                                                            accessibility = eAccessPublic;
     
+                                                        clang::CXXMethodDecl *cxx_method_decl;
                                                         cxx_method_decl = m_ast.AddMethodToCXXRecordType (class_opaque_type.GetOpaqueQualType(),
                                                                                                           type_name_cstr,
                                                                                                           clang_type,
                                                                                                           accessibility,
                                                                                                           is_virtual,
                                                                                                           is_static,
                                                                                                           is_inline,
                                                                                                           is_explicit,
                                                                                                           is_attr_used,
                                                                                                           is_artificial);
     
                                                         type_handled = cxx_method_decl != NULL;
     
                                                         if (type_handled)
                                                         {
                                                             LinkDeclContextToDIE(ClangASTContext::GetAsDeclContext(cxx_method_decl), die);
     
                                                             Host::SetCrashDescription (NULL);
     
     
                                                             ClangASTMetadata metadata;
                                                             metadata.SetUserID(die.GetID());
     
                                                             if (!object_pointer_name.empty())
                                                             {
                                                                 metadata.SetObjectPtrName(object_pointer_name.c_str());
                                                                 if (log)
                                                                     log->Printf ("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;
                                                         }
                                                     }
                                                 }
+                                            }
                                             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
                                                 // we need to modify the dwarf->GetDIEToType() so it doesn't think we are

test/lang/cpp/limit-debug-info/Makefile

+LEVEL = ../../../make
+
+CXX_SOURCES = main.cpp derived.cpp base.cpp
+
+CFLAGS_EXTRAS += $(LIMIT_DEBUG_INFO_FLAGS)
+
+include $(LEVEL)/Makefile.rules

test/lang/cpp/limit-debug-info/TestWithLimitDebugInfo.py

+import lldb
+from lldbtest import *
+import lldbutil
+
+class TestWithLimitDebugInfo(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    @dwarf_test
+    def test_with_dwarf(self):
+        self.buildDwarf()
+
+        cwd = os.getcwd()
+
+        src_file = os.path.join(cwd, "main.cpp")
+        src_file_spec = lldb.SBFileSpec(src_file)
+        self.assertTrue(src_file_spec.IsValid(), "breakpoint file")
+
+        # Get the path of the executable
+        exe_path  = os.path.join(cwd, 'a.out')
+
+        # Load the executable
+        target = self.dbg.CreateTarget(exe_path)
+        self.assertTrue(target.IsValid(), VALID_TARGET)
+
+        # Break on main function
+        breakpoint = target.BreakpointCreateBySourceRegex("break here", src_file_spec)
+        self.assertTrue(breakpoint.IsValid() and breakpoint.GetNumLocations() >= 1, VALID_BREAKPOINT)
+
+        # Launch the process
+        process = target.LaunchSimple(None, None, self.get_process_working_directory())
+        self.assertTrue(process.IsValid(), PROCESS_IS_VALID)
+
+        # Get the thread of the process
+        self.assertTrue(process.GetState() == lldb.eStateStopped, PROCESS_STOPPED)
+        thread = lldbutil.get_stopped_thread(process, lldb.eStopReasonBreakpoint)
+        thread.StepInto()
+
+        # Get frame for current thread
+        frame = thread.GetSelectedFrame()
+
+        v1 = frame.EvaluateExpression("1")
+        self.assertTrue(v1.IsValid(), "'expr 1' results in a valid SBValue object")
+        self.assertTrue(v1.GetError().Success(), "'expr 1' succeeds without an error.")
+
+        v2 = frame.EvaluateExpression("this")
+        self.assertTrue(v2.IsValid(), "'expr this' results in a valid SBValue object")
+        self.assertTrue(v2.GetError().Success(), "'expr this' succeeds without an error.")

test/lang/cpp/limit-debug-info/base.h

+class FooNS
+{
+public:
+    virtual void bar();
+    virtual char baz() = 0;
+
+protected:
+    int x;
+};
+

test/lang/cpp/limit-debug-info/base.cpp

+#include "base.h"
+
+void FooNS::bar() {
+    x = 54321;
+}
+

test/lang/cpp/limit-debug-info/derived.h

+#include "base.h"
+
+class Foo : public FooNS
+{
+public:
+    Foo() {
+        a = 12345;
+    }
+
+    char baz() override;
+    int a;
+};
+

test/lang/cpp/limit-debug-info/derived.cpp

+#include "derived.h"
+
+char Foo::baz() {
+    return (char)(x&0xff);
+}
+

test/lang/cpp/limit-debug-info/main.cpp

+#include "derived.h"
+
+int main() {
+    Foo f; // break here
+    f.bar();
+    return f.baz();
+}

test/make/Makefile.rules

ifeq "$(ARCH)" "aarch64"
     override ARCHFLAG :=
   endif
 
   ifeq "$(SPLIT_DEBUG_SYMBOLS)" "YES"
     DSYM = $(EXE).debug
   endif
 endif
 
+LIMIT_DEBUG_INFO_FLAGS =
+ifneq (,$(findstring clang,$(CC)))
+   LIMIT_DEBUG_INFO_FLAGS += -flimit-debug-info
+endif
+
 CFLAGS ?= -g -O0 -fno-builtin
 ifeq "$(OS)" "Darwin"
   CFLAGS += $(ARCHFLAG) $(ARCH) $(FRAMEWORK_INCLUDES) $(CFLAGS_EXTRAS) -I$(LLDB_BASE_DIR)include
 else
   CFLAGS += $(ARCHFLAG)$(ARCH) $(FRAMEWORK_INCLUDES) $(CFLAGS_EXTRAS) -I$(LLDB_BASE_DIR)include
 endif
 CFLAGS += -include $(THIS_FILE_DIR)test_common.h $(TRIPLE_CFLAGS)