[ASTImporter] Set MustBuildLookupTable on PrimaryContext

[martong]

Summary

SetMustBuildLookupTable() must always be called on a primary context.

[martong]

We have a change in Clang/ASTImporter which causes an LLDB assertion, unless this patch is applied.
The related change is https://reviews.llvm.org/D53655

In that patch we change the importer to properly import the redecl chains of RecordDecls. Consequently the to_tag_decl here in LLDB may not always be the PrimaryContext anymore, thus we have to explicitly use the primary context.

[a_sidorin]

Hi Gabor,
This change looks fine to me but please wait for LLDB reviewer's decision.

[labath]

The change looks pretty safe to me. Adding Greg in case he has any concerns.

[davide]

In D54863#1306947, @martong wrote:

We have a change in Clang/ASTImporter which causes an LLDB assertion, unless this patch is applied.
The related change is https://reviews.llvm.org/D53655

In that patch we change the importer to properly import the redecl chains of RecordDecls. Consequently the to_tag_decl here in LLDB may not always be the PrimaryContext anymore, thus we have to explicitly use the primary context.

Can you write an lldb test for this?

[clayborg]

Looks good to me as long as test suite is happy.

[martong]

Can you write an lldb test for this?

There is already an existing test for that:

2: test_expr_dwarf (TestSharedLib.SharedLibTestCase)
   Test that types work when defined in a shared library and forward-declared in the main executable ... python: ../../git/llvm/tools/clang/include/clang/AST/DeclBase.h:2298: void clang::DeclContext::setMustBuildLookupTable(): Assertion `this == getPrimaryContext() && "should only be called on primary context"' failed.

[davide]

In D54863#1308176, @martong wrote:

Can you write an lldb test for this?

There is already an existing test for that:

2: test_expr_dwarf (TestSharedLib.SharedLibTestCase)
   Test that types work when defined in a shared library and forward-declared in the main executable ... python: ../../git/llvm/tools/clang/include/clang/AST/DeclBase.h:2298: void clang::DeclContext::setMustBuildLookupTable(): Assertion `this == getPrimaryContext() && "should only be called on primary context"' failed.

Cool, thanks. Is this failing only on linux? Do you happen to have a buildbot link that shows the failure?

Thanks!

[martong]

Thank you all for the review! :)

[martong]

Is this failing only on linux? Do you happen to have a buildbot link that shows the failure?

This LLDB patch is to prevent the buildbot failure when we commit this Clang/ASTImporter patch : https://reviews.llvm.org/D53655

lldb/trunk/source/Symbol/ClangASTImporter.cpp

if (log)
                   static_cast<void *>(from), static_cast<void *>(m_source_ctx),
                   static_cast<void *>(&to->getASTContext()));
   }
 
   if (TagDecl *from_tag_decl = dyn_cast<TagDecl>(from)) {
     TagDecl *to_tag_decl = dyn_cast<TagDecl>(to);
 
     to_tag_decl->setHasExternalLexicalStorage();
-    to_tag_decl->setMustBuildLookupTable();
+    to_tag_decl->getPrimaryContext()->setMustBuildLookupTable();
 
     if (log)
       log->Printf(
           "    [ClangASTImporter] To is a TagDecl - attributes %s%s [%s->%s]",
           (to_tag_decl->hasExternalLexicalStorage() ? " Lexical" : ""),
           (to_tag_decl->hasExternalVisibleStorage() ? " Visible" : ""),
           (from_tag_decl->isCompleteDefinition() ? "complete" : "incomplete"),
           (to_tag_decl->isCompleteDefinition() ? "complete" : "incomplete"));