DWARFASTParserClang::CompleteTypeFromDWARF: Handle incomplete baseclass or child

[sivachandra]

Summary

With this change DWARFASTParserClang::CompleteTypeFromDWARF returns false if
DWARFASTParserClang::ParseChildMembers returns false. Similarly, it returns
false if any base class is of an incomplete type. This helps in cases like
these:

class Foo
{
public:
  std::string str;
};
...
Foo f;

If a file with the above code is compiled with a modern clang but without
the -fno-limit-debug-info (or similar) option, then the DWARF has only
a forward declration for std::string. In which case, the type for
"class Foo" cannot be completed. If LLDB does not detect that a child
member has incomplete type, then it wrongly conveys to clang (the LLDB
compiler) that "class Foo" is complete, and consequently crashes due to
an assertion failure in clang when running commands like "p f" or
"frame var f".

[sivachandra]

Fix few copy-paste typos.

[clayborg]

We have the same problem with base classes where if we have a class that has a DW_TAG_inheritance tag that points to a forward declaration for a class, then we start and end the definition of the class and assume the base class is empty with no methods, ivars or anything.

You will need to fix this test case so that is fails on MacOSX as well. I believe you will need to specify the -flimit-debug-info manually as -fno-limit-debug-info is the default on darwin and possibly other platforms. So this test should probably only run when using a clang that supports -flimit-debug-info/-fno-limit-debug-info.

[clayborg]

Emitting a warning is a good idea as well if the compiler is clang and how to fix it (something like "please specify -fno-limit-debug-info ...")

[clayborg]

We also need to make sure that if the class is specified anywhere else within the current binary (in another .o file), that we find that class definition and use it in place of the forward declaration. So if you can make another test with the same source file, and another one where std::string _is_ defined, and make sure that the above example succeeds in finding the complete type for std::string elsewhere in the binary that would be great. Same applies to DW_TAG_inheritance tags that point to incomplete types. 1 test for this, and one for when DW_TAG_inheritance points to a forward decl, but we have a full decl elsewhere in another compile unit within the same binary.

[sivachandra]

I did not touch the base classes part because there is a comment in DWARFASTParserClang::CompleteTypeFromDWARF as:

// We have no choice other than to pretend that the base class
// is complete. If we don't do this, clang will crash when we
// call setBases() inside of "clang_type.SetBaseClassesForClassType()"
// below. Since we provide layout assistance, all ivars in this
// class and other classes will be fine, this is the best we can do
// short of crashing.

I assumed it was being properly handled and did not try an example with it. I did now and LLDB crashes. Do you suggest that I modify to return false if a base class also has incomplete type?

[sivachandra]

I have read that part of the code more closely now. It does look like it might need some fixing. Will address all your comments and send a newer version shortly.

[sivachandra]

Address comments.

[sivachandra]

I am not sure how portable my Makefile is. I have no idea if it will work on Windows for example.

[clayborg]

Looks good as long as the test suite if happy.

[dblaikie]

test/lang/cpp/incomplete-types/Makefile

8

In case it's interesting, you can get similarly problematic DWARF by using a dynamic class (one with virtual functions) with a key function that is not defined in the current translation unit.

GCC implements this behavior as well (whereas GCC doesn't implement the behavior that is triggering on basic_string involving explicit instantiation declarations/definitions) and also has a flag for it: -femit-class-debug-always (I think last I recall, Eric Christopher mentioned he'd looked at the GCC implementation of this flag and it differed in some ways from Clang's, so he was reticent to add a compatibility flag for this in Clang... but we could discuss/revisit that, perhaps (though I suppose it wouldn't allow you to /enable/ this optimization, only disable it - not sure if there's a way to opt-in in GCC))

Though it's easy to "disable" the feature by simply not triggering it, rather than using a flag to turn it off - eg: providing no key function for a type (if you're triggering the dynamic class case, not the template case), or removing an explicit instantiation declaration/definition (if you're triggering the template case)

test/lang/cpp/incomplete-types/TestCppIncompleteTypes.py

5

You don't seem to have a test case for the shared library case in the bug report? (decl in one obj, def in the other, shared library/object/whatever boundary between the two)

(& not sure what the behavior is in the case where the def is provided in one obj and they are statically linked together - bug report mentions the failure doesn't occur, but I don't know if it does the right thing (actually finds the definition) - I know lldb used to not do the right thing there, and had some error message about the compiler being wrong)

test/lang/cpp/incomplete-types/length.h

4

Having a test case depend on the entire header of <string> seems a bit brittle - and means this test may or may not test the behavior you're interested in, depending on how the standard library is defined (depending on whether it actually has an explicit instantiation decl/def for basic_string<char> or not)

[sivachandra]

test/lang/cpp/incomplete-types/Makefile

8

Thanks for this info. I can remove the use of std::string and make use of a dynamic class to test this behavior. And, if it can be tested with GCC as well, then @skipIfGcc can be removed.

test/lang/cpp/incomplete-types/TestCppIncompleteTypes.py

5

Per my understanding, LLDB does not search the world to complete or lookup a type. So, shared library case will not still work. If the objects are statically linked, it will however.

Making it work in the shared library case is the next step. I treated pr24812 as unrelated to this. I can open it back if you feel it should be kept open until the shared library case is fixed.

[clayborg]

Each shared library is an object that can be shared between multiple targets. We do not want to injecting types from another shared library into the static notion of what a type is within a shared library. Why? What if one target in lldb loads liba.so which has a forward declaration to "class Bar;", and this same target loads libb.so which contains a full definition for "Bar". Another target also loads liba.so, but it loads a different version of "libb.so" which has a newer more up to date version of "Bar" because you are doing a edit/compile/debug cycle. We can't take a full definition of "Bar" and place it into the debug info for liba.so because it can be wrong. So each shared library always just parses the types _as_they_see_them_ in their debug info.

We do allow variable introspection to always grab the complete type from another shared library when displaying the type, we just don't allow the static notion of a type that comes from a specific shared library to be augmented within that shared library.

So lets say you have liba.so that contains Foo class which has a "Bar *m_bar;" member variable. liba.so doesn't have a full definition for "Bar" which is OK. "Bar", in liba.so, is known to be "class Bar;", nothing else. When we are debugging a a.out binary later that loads both liba.so and libb.so (which contains the full definition for "Bar"), in the variable display code we see that we want to expand "Bar" in the variables view and we note that we have a full definition of "Bar" in libb.so, so we end up using the full definition from libb.so in place of the forward declaration. Again, we just switch the types. We can do this because have have a type that comes from a specific target, and within that target (a collection of shared libraries) we can come up with the right definition for "Bar" without modifying the type itself from the debug info within liba.so.

So there should be not problems when viewing variables because we have that covered.

If we have any problems in the expression parser, we will need to solve them in the say kind of way: given a target context, determine the right version of the type to use and copy into the ASTContext for the expression. We currently import types from multiple different ASTContexts into an expression ASTContext for each expression that we run, so it would be possible to identify any classes that were forward declarations and try to complete them during the ASTContext -> ASTContext copy cycle. We would need to somehow mark these incomplete types to allow them to be completed later. Like for example the forward declaration to a base class. We used to just complete the definition for the base class and say it had no member and no methods. We have hooks in to allow the DWARF to assist in laying out a class so that we can correct any alignment issues we run into (since DWARF doesn't always capture any #pragma pack and other compiler directives that can affect how a class is laid out), but we could allow these base classes to be created and mark them somehow in the ClangASTContext so we can identify them later during any expression use. We could also use this for the Variable display code and find the full definition of the forward declaration base class...

So, any changes you make in future fixes must adhere to:

• a type is parsed exactly as it is represented within the object file itself, no outside definitions from other shared libraries can be merged into these types
• it is fine to switch over to using a different version of a type (the full definition of "Bar" from libb.so when you have a forward declaration for "Bar" in liba.so) when displaying variables or evaluating expressions that have a Target in their execution context since the target has a list of all the shared libraries that are currently loaded. This allows process a.out to load one version of "Bar" from libb.so, and process b.out to load another version of "Bar" from libc.so
• We can modify the ClangASTContext ASTImporter to do the same kind of type switching when copying types into the expression ASTContext

[zturner]

I don't want compiler options in the test Makefiles. Please move the logic
to Makefile.rules, and create a variable and set that instead.

[zturner]

I know, I've seen them in a few places myself. But I think we should move
away from that, because it's a large source of portability problems

[zturner]

To be clear, I would like this Makefile to turn into the following:

LEVEL = ../../../make

CXX_SOURCES = main.cpp length.cpp

DEBUG_INFO_FULL = True
DEBUG_INFO_LIMITED = True

And that's it. You shouldn't need anything else. Whatever needs to happen
in Makefile.rules to make this work should be done.

By default, DEBUG_INFO_FULL should be True and DEBUG_INFO_LIMITED should be
false. This will allow all other makefiles that don't care about the debug
info to work as they normally do. If neither variable is True, it won't
use -g. If only DEBUG_INFO_FULL is True, it will build one target and use
-fno-limit-debug-info. If only DEBUG_INFO_LIMITED is true, it will build
one target and use -flimit-debug-info. If both are true, it will build two
targets, one for each case.

Does this seem reasonable?

[zturner]

Also,when you say the next iteration, is this something that is going to
happen at an unknown time in the future whenever you happen to touch it
again, or are you working on another iteration now?

source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h

protected:
 
     bool
     ParseTemplateDIE (const DWARFDIE &die,
                       lldb_private::ClangASTContext::TemplateParameterInfos &template_param_infos);
     bool
     ParseTemplateParameterInfos (const DWARFDIE &parent_die,
                                  lldb_private::ClangASTContext::TemplateParameterInfos &template_param_infos);
 
-    size_t
+    bool
     ParseChildMembers (const lldb_private::SymbolContext& sc,
                        const DWARFDIE &die,
                        lldb_private::CompilerType &class_clang_type,
                        const lldb::LanguageType class_language,
                        std::vector<clang::CXXBaseSpecifier *>& base_classes,
                        std::vector<int>& member_accessibilities,
                        DWARFDIECollection& member_function_dies,
                        DelayedPropertyList& delayed_properties,

source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

DWARFASTParserClang::CompleteTypeFromDWARF (const DWARFDIE &die,
     if (log)
         dwarf->GetObjectFile()->GetModule()->LogMessageVerboseBacktrace (log,
                                                                          "0x%8.8" PRIx64 ": %s '%s' resolving forward declaration...",
                                                                          die.GetID(),
                                                                          die.GetTagAsCString(),
                                                                          type->GetName().AsCString());
     assert (clang_type);
     DWARFAttributes attributes;
-
     switch (tag)
     {
         case DW_TAG_structure_type:
         case DW_TAG_union_type:
         case DW_TAG_class_type:
         {
             LayoutInfo layout_info;
 
switch (tag)
                     SymbolContext sc(die.GetLLDBCompileUnit());
                     std::vector<clang::CXXBaseSpecifier *> base_classes;
                     std::vector<int> member_accessibilities;
                     bool is_a_class = false;
                     // Parse members and base classes first
                     DWARFDIECollection member_function_dies;
 
                     DelayedPropertyList delayed_properties;
-                    ParseChildMembers (sc,
+                    if (!ParseChildMembers (sc,
                                             die,
                                             clang_type,
                                             class_language,
                                             base_classes,
                                             member_accessibilities,
                                             member_function_dies,
                                             delayed_properties,
                                             default_accessibility,
                                             is_a_class,
-                                       layout_info);
+                                            layout_info))
+                    {
+                        auto module = dwarf->GetObjectFile()->GetModule();
+                        module->ReportError (":: Class %s has members with incomplete type.", die.GetName());
+                        if (die.GetCU()->GetProducer() == DWARFCompileUnit::eProducerClang)
+                            module->ReportError(":: Try compiling the source file with -fno-limit-debug-info.");
+
+                        return false;
+                    }
 
                     // Now parse any methods if there were any...
                     size_t num_functions = member_function_dies.Size();
                     if (num_functions > 0)
                     {
                         for (size_t i=0; i<num_functions; ++i)
                         {
                             dwarf->ResolveType(member_function_dies.GetDIEAtIndex(i));
switch (tag)
                                                                member_accessibilities.size());
                     }
 
                     if (!base_classes.empty())
                     {
                         // Make sure all base classes refer to complete types and not
                         // forward declarations. If we don't do this, clang will crash
                         // with an assertion in the call to clang_type.SetBaseClassesForClassType()
-                        bool base_class_error = false;
                         for (auto &base_class : base_classes)
                         {
                             clang::TypeSourceInfo *type_source_info = base_class->getTypeSourceInfo();
                             if (type_source_info)
                             {
                                 CompilerType base_class_type (&m_ast, type_source_info->getType().getAsOpaquePtr());
                                 if (base_class_type.GetCompleteType() == false)
                                 {
-                                    if (!base_class_error)
-                                    {
-                                        dwarf->GetObjectFile()->GetModule()->ReportError ("DWARF DIE at 0x%8.8x for class '%s' has a base class '%s' that is a forward declaration, not a complete definition.\nPlease file a bug against the compiler and include the preprocessed output for %s",
-                                                                                          die.GetOffset(),
-                                                                                          die.GetName(),
-                                                                                          base_class_type.GetTypeName().GetCString(),
-                                                                                          sc.comp_unit ? sc.comp_unit->GetPath().c_str() : "the source file");
-                                    }
-                                    // We have no choice other than to pretend that the base class
-                                    // is complete. If we don't do this, clang will crash when we
-                                    // call setBases() inside of "clang_type.SetBaseClassesForClassType()"
-                                    // below. Since we provide layout assistance, all ivars in this
-                                    // class and other classes will be fine, this is the best we can do
-                                    // short of crashing.
+                                    auto module = dwarf->GetObjectFile()->GetModule();
+                                    module->ReportError (
+                                        ":: Class '%s' has a base class '%s' which does not have a complete definition.",
+                                        base_class_type.GetTypeName().GetCString());
+                                    if (die.GetCU()->GetProducer() == DWARFCompileUnit::eProducerClang)
+                                         module->ReportError (":: Try compiling the source file with -fno-limit-debug-info.");
 
-                                    ClangASTContext::StartTagDeclarationDefinition (base_class_type);
-                                    ClangASTContext::CompleteTagDeclarationDefinition (base_class_type);
+                                    return false;
                                 }
                             }
                         }
                         m_ast.SetBaseClassesForClassType (clang_type.GetOpaqueQualType(),
                                                           &base_classes.front(),
                                                           base_classes.size());
 
                         // Clang will copy each CXXBaseSpecifier in "base_classes"
if (die.GetDIENamesAndRanges (name,
                 }
             }
         }
     }
     return NULL;
 }
 
 
-size_t
+bool
 DWARFASTParserClang::ParseChildMembers (const SymbolContext& sc,
                                         const DWARFDIE &parent_die,
                                         CompilerType &class_clang_type,
                                         const LanguageType class_language,
                                         std::vector<clang::CXXBaseSpecifier *>& base_classes,
                                         std::vector<int>& member_accessibilities,
                                         DWARFDIECollection& member_function_dies,
                                         DelayedPropertyList& delayed_properties,
                                         AccessType& default_accessibility,
                                         bool &is_a_class,
                                         LayoutInfo &layout_info)
 {
     if (!parent_die)
         return 0;
 
-    size_t count = 0;
+    uint32_t incomplete_member_info_count = 0;
     uint32_t member_idx = 0;
     BitfieldInfo last_field_info;
 
     ModuleSP module_sp = parent_die.GetDWARF()->GetObjectFile()->GetModule();
     ClangASTContext *ast = llvm::dyn_cast_or_null<ClangASTContext>(class_clang_type.GetTypeSystem());
     if (ast == nullptr)
         return 0;
 
for (DWARFDIE die = parent_die.GetFirstChild(); die.IsValid(); die = die.GetSibling())
                                     last_field_info = this_field_info;
                                 }
                                 else
                                 {
                                     last_field_info.Clear();
                                 }
 
                                 CompilerType member_clang_type = member_type->GetLayoutCompilerType ();
+                                if (!member_clang_type.IsCompleteType() && !member_clang_type.GetCompleteType())
+                                    incomplete_member_info_count += 1;
 
                                 {
                                     // Older versions of clang emit array[0] and array[1] in the same way (<rdar://problem/12566646>).
                                     // If the current field is at the end of the structure, then there is definitely no room for extra
                                     // elements and we override the type to array[0].
 
                                     CompilerType member_array_element_type;
                                     uint64_t member_array_size;
for (DWARFDIE die = parent_die.GetFirstChild(); die.IsValid(); die = die.GetSibling())
             }
                 break;
 
             default:
                 break;
         }
     }
 
-    return count;
+    return incomplete_member_info_count == 0;
 }
 
 
 size_t
 DWARFASTParserClang::ParseChildParameters (const SymbolContext& sc,
                                            clang::DeclContext *containing_decl_ctx,
                                            const DWARFDIE &parent_die,
                                            bool skip_artificial,

test/lang/cpp/incomplete-types/Makefile

+LEVEL = ../../../make
+
+CXX_SOURCES = main.cpp length.cpp
+
+CFLAGS_LIMIT = -g -O0 -c
+CFLAGS_NO_LIMIT = -g -O0 -c
+
+ifneq (,$(findstring clang,$(CC)))
+  CFLAGS_LIMIT += -flimit-debug-info
+  CFLAGS_NO_LIMIT += -fno-limit-debug-info
+endif
+
+all: limit nolimit
+
+limit: main.o length_limit.o
+  $(CXX) main.o length_limit.o -o limit
+
+nolimit: main.o length_nolimit.o
+  $(CXX) main.o length_nolimit.o -o nolimit
+
+main.o: main.cpp
+  $(CXX) $(CFLAGS_LIMIT) main.cpp -o main.o
+
+length_limit.o: length.cpp
+  $(CXX) $(CFLAGS_LIMIT) length.cpp -o length_limit.o
+
+length_nolimit.o: length.cpp
+  $(CXX) $(CFLAGS_NO_LIMIT) length.cpp -o length_nolimit.o
+
+clean: OBJECTS += limit nolimit length_limit.o length_nolimit.o
+
+include $(LEVEL)/Makefile.rules

test/lang/cpp/incomplete-types/TestCppIncompleteTypes.py

+import lldb
+from lldbtest import *
+import lldbutil
+
+class TestCppIncompleteTypes(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    @dwarf_test
+    @skipIfGcc
+    def test_with_dwarf_limit_debug_info(self):
+        self.buildDwarf()
+        frame = self.get_test_frame('limit')
+
+        value_f = frame.EvaluateExpression("f")
+        self.assertTrue(value_f.IsValid(), "'expr f' results in a valid SBValue object")
+        self.assertFalse(value_f.GetError().Success(), "'expr f' results in an error, but LLDB does not crash")
+
+        value_s = frame.EvaluateExpression("s")
+        self.assertTrue(value_s.IsValid(), "'expr s' results in a valid SBValue object")
+        self.assertFalse(value_s.GetError().Success(), "'expr s' results in an error, but LLDB does not crash")
+
+    @dwarf_test
+    @skipIfGcc
+    def test_with_dwarf_partial_limit_debug_info(self):
+        self.buildDwarf()
+        frame = self.get_test_frame('nolimit')
+
+        value_f = frame.EvaluateExpression("f")
+        self.assertTrue(value_f.IsValid(), "'expr f' results in a valid SBValue object")
+        self.assertTrue(value_f.GetError().Success(), "'expr f' is successful")
+
+        value_s = frame.EvaluateExpression("s")
+        self.assertTrue(value_s.IsValid(), "'expr s' results in a valid SBValue object")
+        self.assertTrue(value_s.GetError().Success(), "'expr s' is successful")
+
+    def setUp(self):
+        TestBase.setUp(self)
+
+    def get_test_frame(self, exe):
+        # Get main source file
+        src_file = "main.cpp"
+        src_file_spec = lldb.SBFileSpec(src_file)
+        self.assertTrue(src_file_spec.IsValid(), "Main source file")
+
+        # Get the path of the executable
+        cwd = os.getcwd()
+        exe_path  = os.path.join(cwd, exe)
+
+        # Load the executable
+        target = self.dbg.CreateTarget(exe_path)
+        self.assertTrue(target.IsValid(), VALID_TARGET)
+
+        # Break on main function
+        main_breakpoint = target.BreakpointCreateBySourceRegex("break here", src_file_spec)
+        self.assertTrue(main_breakpoint.IsValid() and main_breakpoint.GetNumLocations() >= 1, VALID_BREAKPOINT)
+
+        # Launch the process
+        args = None
+        env = None
+        process = target.LaunchSimple(args, env, 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)
+
+        # Get frame for current thread
+        return thread.GetSelectedFrame()
+
+if __name__ == '__main__':
+    import atexit
+    lldb.SBDebugger.Initialize()
+    atexit.register(lambda: lldb.SBDebugger.Terminate())
+    unittest2.main()

test/lang/cpp/incomplete-types/length.h

+#ifndef __LENGTH_H__
+#define __LENGTH_H__
+
+#include <string>
+
+size_t length (const std::string &str);
+
+#endif

test/lang/cpp/incomplete-types/length.cpp

+
+#include "length.h"
+
+size_t
+length (const std::string &str)
+{
+  return str.length();
+}

test/lang/cpp/incomplete-types/main.cpp

+
+#include "length.h"
+
+class Foo {
+public:
+    Foo(std::string x) : s(x) {}
+
+private:
+    std::string s;
+};
+
+class MyString : public std::string {
+public:
+    MyString(std::string x) : std::string(x) {}
+};
+
+int main()
+{
+    Foo f("qwerty");
+    MyString s("qwerty");
+
+    return length(s); // break here
+}