This is an archive of the discontinued LLVM Phabricator instance.

[ASTImporter] Set MustBuildLookupTable on PrimaryContext
ClosedPublic

Authored by martong on Nov 23 2018, 7:21 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

martong created this revision.Nov 23 2018, 7:21 AM

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.

martong set the repository for this revision to rLLDB LLDB.Nov 25 2018, 8:58 AM
a_sidorin accepted this revision.Nov 25 2018, 11:06 AM
a_sidorin added a subscriber: a_sidorin.

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

This revision is now accepted and ready to land.Nov 25 2018, 11:06 AM
labath accepted this revision.Nov 26 2018, 2:07 AM
labath added a reviewer: clayborg.

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

davide added a subscriber: davide.Nov 26 2018, 6:39 AM

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 accepted this revision.Nov 26 2018, 7:08 AM

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

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.

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!

This revision was automatically updated to reflect the committed changes.

Thank you all for the review! :)

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