Fix resolution of certain recursive types.

[sas]

Summary

If a struct type S has a member T that has a member that is a function that
returns a typedef of S* the respective field would be duplicated, which caused
an assert down the line in RecordLayoutBuilder. This patch adds a check that
removes the possibility of trying to resolve the same type twice within the
same callstack.

This commit also adds unit tests for these failures.

Fixes https://llvm.org/bugs/show_bug.cgi?id=20486.

Patch by Cristian Hancila.

[sas]

I'd like to add some unit tests for this patch but I don't know how to create one that has two separate source files (which is required to repro this bug). Does the unit test framework support that?

[clayborg]

See inline comments.

You can easily make a test case with two files, there are many that do it.

source/Symbol/ClangASTImporter.cpp

289–295

Do we always want to set this to true? If we really just have a forward declaration in the DWARF "result" might be false. If we set this to true, it means anytime we have a forward declaration to a class, we might start and complete the class definition and say that is is a complete class that contains nothing. Then in another ClangASTContext we might have a full definition of the type, then we try to import both types into an expression ClangASTContext and it will fail saying there are two definitions. You should add any needed smarts to ComplateTagDecl() and not do anything inline like this.

[spyffe]

Stéphane,

please add me as a reviewer for this patch. I have an account, spyffe.

Sean

[sas]

source/Symbol/ClangASTImporter.cpp

289–295

This code follows what we have at lines 202-207 so I'm not sure if we're doing something wrong.

I'll factor the code in CompleteTagDecl as you suggested and let Sean comment on what's the right thing to do here.

[sas]

Add unit tests and address review comments.

[spyffe]

This looks good, just make the RAII change so that everybody editing the code doesn't have to remember this.

source/Expression/ClangASTSource.cpp

505

This should be done in a RAII way, with some object that deregisters context_decl when the function returns. That way we don't have to remember to explicitly deregister it wherever we return.

[sas]

Use a scoped class to do removals.

[clayborg]

I am ok as long as Sean Callanan is ok with this.

[spyffe]

Looks good! Thanks for the patch.

[sas]

Will submit this to trunk. Spoke with Sean about porting to release_36 as well after letting it sit for a couple of weeks on trunk, he's fine with it. Will do that too.

include/lldb/Expression/ClangASTSource.h

public:
     /// @param[in] declMap
     ///     A reference to the LLDB object that handles entity lookup.
     //------------------------------------------------------------------
     ClangASTSource (const lldb::TargetSP &target) :
         m_import_in_progress (false),
         m_lookups_enabled (false),
         m_target (target),
         m_ast_context (NULL),
+        m_active_lexical_decls (),
         m_active_lookups ()
     {
         m_ast_importer = m_target->GetClangASTImporter();
     }
   
     //------------------------------------------------------------------
     /// Destructor
     //------------------------------------------------------------------
protected:
     friend struct NameSearchContext;
     
     bool                    m_import_in_progress;
     bool                    m_lookups_enabled;
 
     const lldb::TargetSP                m_target;           ///< The target to use in finding variables and types.
     clang::ASTContext                  *m_ast_context;      ///< The AST context requests are coming in for.
     ClangASTImporter                   *m_ast_importer;     ///< The target's AST importer.
+    std::set<const clang::Decl *>       m_active_lexical_decls;
     std::set<const char *>              m_active_lookups;
 };
 
 //----------------------------------------------------------------------
 /// @class NameSearchContext ClangASTSource.h "lldb/Expression/ClangASTSource.h"
 /// @brief Container for all objects relevant to a single name lookup
 ///     
 /// LLDB needs to create Decls for entities it finds.  This class communicates

source/Expression/ClangASTSource.cpp

 #include "lldb/Target/ObjCLanguageRuntime.h"
 #include "lldb/Target/Target.h"
 
 #include <vector>
 
 using namespace clang;
 using namespace lldb_private;
 
+//------------------------------------------------------------------
+// Scoped class that will remove an active lexical decl from the set
+// when it goes out of scope.
+//------------------------------------------------------------------
+namespace {
+    class ScopedLexicalDeclEraser
+    {
+    public:
+        ScopedLexicalDeclEraser(std::set<const clang::Decl *> &decls,
+                                const clang::Decl *decl)
+            : m_active_lexical_decls(decls), m_decl(decl)
+        {
+        }
+
+        ~ScopedLexicalDeclEraser()
+        {
+            m_active_lexical_decls.erase(m_decl);
+        }
+
+    private:
+        std::set<const clang::Decl *> &m_active_lexical_decls;
+        const clang::Decl *m_decl;
+    };
+}
+
 ClangASTSource::~ClangASTSource()
 {
     m_ast_importer->ForgetDestination(m_ast_context);
 
     // We are in the process of destruction, don't create clang ast context on demand
     // by passing false to Target::GetScratchClangASTContext(create_on_demand).
     ClangASTContext *scratch_clang_ast_context = m_target->GetScratchClangASTContext(false);
 
if (log)
                     static_cast<void*>(tag_decl),
                     tag_decl->getName().str().c_str());
 
         log->Printf("      CTD[%u] Before:", current_id);
         ASTDumper dumper((Decl*)tag_decl);
         dumper.ToLog(log, "      [CTD] ");
     }
 
+    auto iter = m_active_lexical_decls.find(tag_decl);
+    if (iter != m_active_lexical_decls.end())
+        return;
+    m_active_lexical_decls.insert(tag_decl);
+    ScopedLexicalDeclEraser eraser(m_active_lexical_decls, tag_decl);
+
     if (!m_ast_importer->CompleteTagDecl (tag_decl))
     {
         // We couldn't complete the type.  Maybe there's a definition
         // somewhere else that can be completed.
 
         if (log)
             log->Printf("      CTD[%u] Type could not be completed in the module in which it was first found.", current_id);
 
ClangASTSource::FindExternalLexicalDecls (const DeclContext *decl_context,
 
     Log *log(lldb_private::GetLogIfAllCategoriesSet (LIBLLDB_LOG_EXPRESSIONS));
 
     const Decl *context_decl = dyn_cast<Decl>(decl_context);
 
     if (!context_decl)
         return ELR_Failure;
 
+    auto iter = m_active_lexical_decls.find(context_decl);
+    if (iter != m_active_lexical_decls.end())
+        return ELR_Failure;
+    m_active_lexical_decls.insert(context_decl);
+    ScopedLexicalDeclEraser eraser(m_active_lexical_decls, context_decl);
+
     static unsigned int invocation_id = 0;
     unsigned int current_id = invocation_id++;
 
     if (log)
     {
         if (const NamedDecl *context_named_decl = dyn_cast<NamedDecl>(context_decl))
             log->Printf("FindExternalLexicalDecls[%u] on (ASTContext*)%p in '%s' (%sDecl*)%p with %s predicate",
                         current_id, static_cast<void*>(m_ast_context),
if (TagDecl *original_tag_decl = dyn_cast<TagDecl>(original_decl))
         if (external_source)
             external_source->CompleteType (original_tag_decl);
     }
 
     const DeclContext *original_decl_context = dyn_cast<DeclContext>(original_decl);
 
     if (!original_decl_context)
         return ELR_Failure;
 
     for (TagDecl::decl_iterator iter = original_decl_context->decls_begin();
          iter != original_decl_context->decls_end();
          ++iter)
     {
         Decl *decl = *iter;
 
         if (!predicate || predicate(decl->getKind()))
         {

source/Symbol/ClangASTImporter.cpp

 bool
 ClangASTImporter::RequireCompleteType (clang::QualType type)
 {
     if (type.isNull())
         return false;
     
     if (const TagType *tag_type = type->getAs<TagType>())
     {
-        return CompleteTagDecl(tag_type->getDecl());
+        TagDecl *tag_decl = tag_type->getDecl();
+
+        if (tag_decl->getDefinition() || tag_decl->isBeingDefined())
+            return true;
+
+        return CompleteTagDecl(tag_decl);
     }
     if (const ObjCObjectType *objc_object_type = type->getAs<ObjCObjectType>())
     {
         if (ObjCInterfaceDecl *objc_interface_decl = objc_object_type->getInterface())
             return CompleteObjCInterfaceDecl(objc_interface_decl);
         else
             return false;
     }
     if (const ArrayType *array_type = type->getAsArrayTypeUnsafe())

test/types/TestRecursiveTypes.py

+"""
+Test that recursive types are handled correctly.
+"""
+
+import lldb
+import lldbutil
+import sys
+import unittest2
+from lldbtest import *
+
+class RecursiveTypesTestCase(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    def setUp(self):
+        # Call super's setUp().
+        TestBase.setUp(self)
+        # disable "There is a running process, kill it and restart?" prompt
+        self.runCmd("settings set auto-confirm true")
+        self.addTearDownHook(lambda: self.runCmd("settings clear auto-confirm"))
+        # Find the line number to break for main.c.
+        self.line = line_number('recursive_type_main.cpp',
+                                '// Test at this line.')
+
+        self.d1 = {'CXX_SOURCES': 'recursive_type_main.cpp recursive_type_1.cpp'}
+        self.d2 = {'CXX_SOURCES': 'recursive_type_main.cpp recursive_type_2.cpp'}
+
+    @unittest2.skipUnless(sys.platform.startswith("darwin"), "requires Darwin")
+    @dsym_test
+    def test_recursive_dsym_type_1(self):
+        """Test that recursive structs are displayed correctly."""
+        self.buildDsym(dictionary=self.d1)
+        self.print_struct()
+
+    @dwarf_test
+    def test_recursive_dwarf_type_1(self):
+        """Test that recursive structs are displayed correctly."""
+        self.buildDwarf(dictionary=self.d1)
+        self.print_struct()
+
+    @unittest2.skipUnless(sys.platform.startswith("darwin"), "requires Darwin")
+    @dsym_test
+    def test_recursive_dsym_type_2(self):
+        """Test that recursive structs are displayed correctly."""
+        self.buildDsym(dictionary=self.d2)
+        self.print_struct()
+
+    @dwarf_test
+    def test_recursive_dwarf_type_2(self):
+        """Test that recursive structs are displayed correctly."""
+        self.buildDwarf(dictionary=self.d2)
+        self.print_struct()
+
+    def print_struct(self):
+        self.runCmd("file a.out", CURRENT_EXECUTABLE_SET)
+
+        lldbutil.run_break_set_by_file_and_line (self, "recursive_type_main.cpp", self.line, num_expected_locations=-1, loc_exact=True)
+
+        self.runCmd("run", RUN_SUCCEEDED)
+
+        self.expect("print tpi", RUN_SUCCEEDED)
+        self.expect("print *tpi", RUN_SUCCEEDED)
+
+if __name__ == '__main__':
+    import atexit
+    lldb.SBDebugger.Initialize()
+    atexit.register(lambda: lldb.SBDebugger.Terminate())
+    unittest2.main()

test/types/recursive_type_1.cpp

+typedef struct t *tp;
+typedef tp (*get_tp)();
+
+struct s {
+    get_tp get_tp_p;
+};
+
+struct t {
+    struct s *s;
+};
+
+struct t t;

test/types/recursive_type_2.cpp

+typedef struct t *tp;
+typedef tp (*get_tp)();
+
+struct t {
+    struct {
+      get_tp get_tp_p;
+    };
+};
+
+struct t t;

test/types/recursive_type_main.cpp

+typedef struct t *tp;
+extern struct t t;
+
+int main() {
+    tp tpi = &t;
+    // Test at this line.
+    return 0;
+}