This is an archive of the discontinued LLVM Phabricator instance.

Differential D94067

[clang][ASTImporter] Fix a possible assertion failure `NeedsInjectedClassNameType(Decl)'.
ClosedPublic

Authored by balazske on Jan 5 2021, 1:01 AM.

Details

Reviewers
a.sidorin
shafik
martong
Commits
rG0877b963ef2d: [clang][ASTImporter] Fix a possible assertion failure…
Summary

The assertion can happen if ASTImporter imports a CXXRecordDecl in a template
and then imports another redeclaration of this declaration, while the first import is in progress.
The process of first import did not set the "described template" yet
and the second import finds the first declaration at setting the injected types.
Setting the injected type requires in the assertion that the described template is set.
The exact assertion was:
clang/lib/AST/ASTContext.cpp:4411:
clang::QualType clang::ASTContext::getInjectedClassNameType(clang::CXXRecordDecl*, clang::QualType) const:
Assertion `NeedsInjectedClassNameType(Decl)' failed.

Diff Detail

Event Timeline

balazske created this revision.Jan 5 2021, 1:01 AM
Herald added a reviewer: a.sidorin. · View Herald TranscriptJan 5 2021, 1:01 AM
Herald added a reviewer: shafik. · View Herald Transcript
Herald added subscribers: martong, teemperor, gamesh411 and 2 others. · View Herald Transcript
balazske requested review of this revision.Jan 5 2021, 1:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 5 2021, 1:01 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
balazske added a comment.Jan 5 2021, 1:06 AM

The exact call chain, if the added test is called without the fix:

$ ./tools/clang/unittests/AST/ASTTests --gtest_filter='*HasNoDescribedTemplateSet*'
Note: Google Test filter = *HasNoDescribedTem*
[==========] Running 4 tests from 1 test case.
[----------] Global test environment set-up.
[----------] 4 tests from ParameterizedTests/ASTImporterOptionSpecificTestBase
[ RUN      ] ParameterizedTests/ASTImporterOptionSpecificTestBase.ImportOfTemplatedDeclWhenPreviousDeclHasNoDescribedTemplateSet/0
ASTTests: llvm-project/clang/lib/AST/ASTContext.cpp:4411: clang::QualType clang::ASTContext::getInjectedClassNameType(clang::CXXRecordDecl*, clang::QualType) const: Assertion `NeedsInjectedClassNameType(Decl)' failed.
 #0 0x00007feb23200d2e llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) llvm-project/llvm/lib/Support/Unix/Signals.inc:563:0
 #1 0x00007feb23200de5 PrintStackTraceSignalHandler(void*) llvm-project/llvm/lib/Support/Unix/Signals.inc:630:0
 #2 0x00007feb231feac9 llvm::sys::RunSignalHandlers() llvm-project/llvm/lib/Support/Signals.cpp:71:0
 #3 0x00007feb232006b0 SignalHandler(int) llvm-project/llvm/lib/Support/Unix/Signals.inc:405:0
 #4 0x00007feb279da980 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x12980)
 #5 0x00007feb226f1fb7 raise /build/glibc-S9d2JN/glibc-2.27/signal/../sysdeps/unix/sysv/linux/raise.c:51:0
 #6 0x00007feb226f3921 abort /build/glibc-S9d2JN/glibc-2.27/stdlib/abort.c:81:0
 #7 0x00007feb226e348a __assert_fail_base /build/glibc-S9d2JN/glibc-2.27/assert/assert.c:89:0
 #8 0x00007feb226e3502 (/lib/x86_64-linux-gnu/libc.so.6+0x30502)
 #9 0x00007feb248a9b3f clang::ASTContext::getInjectedClassNameType(clang::CXXRecordDecl*, clang::QualType) const llvm-project/clang/lib/AST/ASTContext.cpp:4412:0
#10 0x00007feb2499eac4 clang::ASTNodeImporter::VisitRecordDecl(clang::RecordDecl*) llvm-project/clang/lib/AST/ASTImporter.cpp:2898:0
#11 0x00007feb24a22700 clang::declvisitor::Base<std::add_pointer, clang::ASTNodeImporter, llvm::Expected<clang::Decl*> >::VisitCXXRecordDecl(clang::CXXRecordDecl*) build/Debug/tools/clang/include/clang/AST/DeclNodes.inc:263:0
#12 0x00007feb249fcf60 clang::declvisitor::Base<std::add_pointer, clang::ASTNodeImporter, llvm::Expected<clang::Decl*> >::Visit(clang::Decl*) build/Debug/tools/clang/include/clang/AST/DeclNodes.inc:263:0
#13 0x00007feb249c5ca1 clang::ASTImporter::ImportImpl(clang::Decl*) llvm-project/clang/lib/AST/ASTImporter.cpp:8119:0
#14 0x00007feb249c6c53 clang::ASTImporter::Import(clang::Decl*) llvm-project/clang/lib/AST/ASTImporter.cpp:8281:0
#15 0x00007feb249f3d21 llvm::Error clang::ASTNodeImporter::importInto<clang::CXXRecordDecl>(clang::CXXRecordDecl*&, clang::CXXRecordDecl*) llvm-project/clang/lib/AST/ASTImporter.cpp:155:0
#16 0x00007feb249af273 clang::ASTNodeImporter::VisitClassTemplateDecl(clang::ClassTemplateDecl*) llvm-project/clang/lib/AST/ASTImporter.cpp:5407:0
#17 0x00007feb249fce9c clang::declvisitor::Base<std::add_pointer, clang::ASTNodeImporter, llvm::Expected<clang::Decl*> >::Visit(clang::Decl*) build/Debug/tools/clang/include/clang/AST/DeclNodes.inc:207:0
#18 0x00007feb249c5ca1 clang::ASTImporter::ImportImpl(clang::Decl*) llvm-project/clang/lib/AST/ASTImporter.cpp:8119:0
#19 0x00007feb249c6c53 clang::ASTImporter::Import(clang::Decl*) llvm-project/clang/lib/AST/ASTImporter.cpp:8281:0
#20 0x00007feb249e53c9 llvm::Expected<clang::Decl*> clang::ASTNodeImporter::import<clang::Decl>(clang::Decl*) llvm-project/clang/lib/AST/ASTImporter.cpp:165:0
#21 0x00007feb2499735e clang::ASTNodeImporter::ImportDeclContext(clang::DeclContext*, bool) llvm-project/clang/lib/AST/ASTImporter.cpp:1761:0
#22 0x00007feb2499b408 clang::ASTNodeImporter::VisitNamespaceDecl(clang::NamespaceDecl*) llvm-project/clang/lib/AST/ASTImporter.cpp:2385:0
#23 0x00007feb249fcd4c clang::declvisitor::Base<std::add_pointer, clang::ASTNodeImporter, llvm::Expected<clang::Decl*> >::Visit(clang::Decl*) build/Debug/tools/clang/include/clang/AST/DeclNodes.inc:111:0
#24 0x00007feb249c5ca1 clang::ASTImporter::ImportImpl(clang::Decl*) llvm-project/clang/lib/AST/ASTImporter.cpp:8119:0
#25 0x00007feb249c6c53 clang::ASTImporter::Import(clang::Decl*) llvm-project/clang/lib/AST/ASTImporter.cpp:8281:0
#26 0x00007feb249c764d clang::ASTImporter::ImportContext(clang::DeclContext*) llvm-project/clang/lib/AST/ASTImporter.cpp:8387:0
#27 0x00007feb24997aeb clang::ASTNodeImporter::ImportDeclContext(clang::Decl*, clang::DeclContext*&, clang::DeclContext*&) llvm-project/clang/lib/AST/ASTImporter.cpp:1867:0
#28 0x00007feb24996796 clang::ASTNodeImporter::ImportDeclParts(clang::NamedDecl*, clang::DeclContext*&, clang::DeclContext*&, clang::DeclarationName&, clang::NamedDecl*&, clang::SourceLocation&) llvm-project/clang/lib/AST/ASTImporter.cpp:1624:0
#29 0x00007feb2499d9ae clang::ASTNodeImporter::VisitRecordDecl(clang::RecordDecl*) llvm-project/clang/lib/AST/ASTImporter.cpp:2737:0
#30 0x00007feb24a22700 clang::declvisitor::Base<std::add_pointer, clang::ASTNodeImporter, llvm::Expected<clang::Decl*> >::VisitCXXRecordDecl(clang::CXXRecordDecl*) build/Debug/tools/clang/include/clang/AST/DeclNodes.inc:263:0
#31 0x00007feb249fcf60 clang::declvisitor::Base<std::add_pointer, clang::ASTNodeImporter, llvm::Expected<clang::Decl*> >::Visit(clang::Decl*) build/Debug/tools/clang/include/clang/AST/DeclNodes.inc:263:0
#32 0x00007feb249c5ca1 clang::ASTImporter::ImportImpl(clang::Decl*) llvm-project/clang/lib/AST/ASTImporter.cpp:8119:0
#33 0x00007feb249c6c53 clang::ASTImporter::Import(clang::Decl*) llvm-project/clang/lib/AST/ASTImporter.cpp:8281:0
#34 0x00007feb249e6d4d llvm::Expected<clang::RecordDecl*> clang::ASTNodeImporter::import<clang::RecordDecl>(clang::RecordDecl*) llvm-project/clang/lib/AST/ASTImporter.cpp:165:0
#35 0x00007feb24994c34 clang::ASTNodeImporter::VisitRecordType(clang::RecordType const*) llvm-project/clang/lib/AST/ASTImporter.cpp:1397:0
#36 0x00007feb249fde18 clang::TypeVisitor<clang::ASTNodeImporter, llvm::Expected<clang::QualType> >::Visit(clang::Type const*) build/Debug/tools/clang/include/clang/AST/TypeNodes.inc:69:0
#37 0x00007feb249c5f78 clang::ASTImporter::Import(clang::QualType) llvm-project/clang/lib/AST/ASTImporter.cpp:8152:0
#38 0x00007feb249c60fa clang::ASTImporter::Import(clang::TypeSourceInfo*) llvm-project/clang/lib/AST/ASTImporter.cpp:8168:0
#39 0x00007feb249e4d3f llvm::Expected<clang::TypeSourceInfo*> clang::ASTNodeImporter::import<clang::TypeSourceInfo>(clang::TypeSourceInfo*) llvm-project/clang/lib/AST/ASTImporter.cpp:165:0
#40 0x00007feb249ae4b8 clang::ASTNodeImporter::VisitTemplateTypeParmDecl(clang::TemplateTypeParmDecl*) llvm-project/clang/lib/AST/ASTImporter.cpp:5262:0
#41 0x00007feb249fcfb4 clang::declvisitor::Base<std::add_pointer, clang::ASTNodeImporter, llvm::Expected<clang::Decl*> >::Visit(clang::Decl*) build/Debug/tools/clang/include/clang/AST/DeclNodes.inc:293:0
#42 0x00007feb249c5ca1 clang::ASTImporter::ImportImpl(clang::Decl*) llvm-project/clang/lib/AST/ASTImporter.cpp:8119:0
#43 0x00007feb249c6c53 clang::ASTImporter::Import(clang::Decl*) llvm-project/clang/lib/AST/ASTImporter.cpp:8281:0
#44 0x00007feb249ee1e5 llvm::Expected<clang::NamedDecl*> clang::ASTNodeImporter::import<clang::NamedDecl>(clang::NamedDecl*) llvm-project/clang/lib/AST/ASTImporter.cpp:165:0
#45 0x00007feb24a0a278 llvm::Error clang::ASTNodeImporter::ImportArrayChecked<clang::NamedDecl* const*, clang::NamedDecl**>(clang::NamedDecl* const*, clang::NamedDecl* const*, clang::NamedDecl**) llvm-project/clang/lib/AST/ASTImporter.cpp:653:0
#46 0x00007feb249e3b1d llvm::Error clang::ASTNodeImporter::ImportContainerChecked<clang::TemplateParameterList, llvm::SmallVector<clang::NamedDecl*, 4u> >(clang::TemplateParameterList const&, llvm::SmallVector<clang::NamedDecl*, 4u>&) llvm-project/clang/lib/AST/ASTImporter.cpp:669:0
#47 0x00007feb2498e7c5 llvm::Expected<clang::TemplateParameterList*> clang::ASTNodeImporter::import<clang::TemplateParameterList>(clang::TemplateParameterList*) llvm-project/clang/lib/AST/ASTImporter.cpp:747:0
#48 0x00007feb249af31f clang::ASTNodeImporter::VisitClassTemplateDecl(clang::ClassTemplateDecl*) llvm-project/clang/lib/AST/ASTImporter.cpp:5412:0
#49 0x00007feb249fce9c clang::declvisitor::Base<std::add_pointer, clang::ASTNodeImporter, llvm::Expected<clang::Decl*> >::Visit(clang::Decl*) build/Debug/tools/clang/include/clang/AST/DeclNodes.inc:207:0
#50 0x00007feb249c5ca1 clang::ASTImporter::ImportImpl(clang::Decl*) llvm-project/clang/lib/AST/ASTImporter.cpp:8119:0
#51 0x00007feb249c6c53 clang::ASTImporter::Import(clang::Decl*) llvm-project/clang/lib/AST/ASTImporter.cpp:8281:0
#52 0x00007feb249eb455 llvm::Error clang::ASTNodeImporter::importInto<clang::ClassTemplateDecl>(clang::ClassTemplateDecl*&, clang::ClassTemplateDecl*) llvm-project/clang/lib/AST/ASTImporter.cpp:155:0
#53 0x00007feb2499e802 clang::ASTNodeImporter::VisitRecordDecl(clang::RecordDecl*) llvm-project/clang/lib/AST/ASTImporter.cpp:2877:0
#54 0x00007feb24a22700 clang::declvisitor::Base<std::add_pointer, clang::ASTNodeImporter, llvm::Expected<clang::Decl*> >::VisitCXXRecordDecl(clang::CXXRecordDecl*) build/Debug/tools/clang/include/clang/AST/DeclNodes.inc:263:0
#55 0x00007feb249fcf60 clang::declvisitor::Base<std::add_pointer, clang::ASTNodeImporter, llvm::Expected<clang::Decl*> >::Visit(clang::Decl*) build/Debug/tools/clang/include/clang/AST/DeclNodes.inc:263:0
#56 0x00007feb249c5ca1 clang::ASTImporter::ImportImpl(clang::Decl*) llvm-project/clang/lib/AST/ASTImporter.cpp:8119:0
#57 0x00007feb249c6c53 clang::ASTImporter::Import(clang::Decl*) llvm-project/clang/lib/AST/ASTImporter.cpp:8281:0
#58 0x00007feb249f3d21 llvm::Error clang::ASTNodeImporter::importInto<clang::CXXRecordDecl>(clang::CXXRecordDecl*&, clang::CXXRecordDecl*) llvm-project/clang/lib/AST/ASTImporter.cpp:155:0
#59 0x00007feb249af273 clang::ASTNodeImporter::VisitClassTemplateDecl(clang::ClassTemplateDecl*) llvm-project/clang/lib/AST/ASTImporter.cpp:5407:0
#60 0x00007feb249fce9c clang::declvisitor::Base<std::add_pointer, clang::ASTNodeImporter, llvm::Expected<clang::Decl*> >::Visit(clang::Decl*) build/Debug/tools/clang/include/clang/AST/DeclNodes.inc:207:0
#61 0x00007feb249c5ca1 clang::ASTImporter::ImportImpl(clang::Decl*) llvm-project/clang/lib/AST/ASTImporter.cpp:8119:0
#62 0x00007feb249c6c53 clang::ASTImporter::Import(clang::Decl*) llvm-project/clang/lib/AST/ASTImporter.cpp:8281:0
#63 0x00007feb249e53c9 llvm::Expected<clang::Decl*> clang::ASTNodeImporter::import<clang::Decl>(clang::Decl*) llvm-project/clang/lib/AST/ASTImporter.cpp:165:0
#64 0x00007feb2499735e clang::ASTNodeImporter::ImportDeclContext(clang::DeclContext*, bool) llvm-project/clang/lib/AST/ASTImporter.cpp:1761:0
#65 0x00007feb2499b408 clang::ASTNodeImporter::VisitNamespaceDecl(clang::NamespaceDecl*) llvm-project/clang/lib/AST/ASTImporter.cpp:2385:0
#66 0x00007feb249fcd4c clang::declvisitor::Base<std::add_pointer, clang::ASTNodeImporter, llvm::Expected<clang::Decl*> >::Visit(clang::Decl*) build/Debug/tools/clang/include/clang/AST/DeclNodes.inc:111:0
#67 0x00007feb249c5ca1 clang::ASTImporter::ImportImpl(clang::Decl*) llvm-project/clang/lib/AST/ASTImporter.cpp:8119:0
#68 0x00007feb249c6c53 clang::ASTImporter::Import(clang::Decl*) llvm-project/clang/lib/AST/ASTImporter.cpp:8281:0
#69 0x00007feb249c764d clang::ASTImporter::ImportContext(clang::DeclContext*) llvm-project/clang/lib/AST/ASTImporter.cpp:8387:0
#70 0x00007feb24997aeb clang::ASTNodeImporter::ImportDeclContext(clang::Decl*, clang::DeclContext*&, clang::DeclContext*&) llvm-project/clang/lib/AST/ASTImporter.cpp:1867:0
#71 0x00007feb24996796 clang::ASTNodeImporter::ImportDeclParts(clang::NamedDecl*, clang::DeclContext*&, clang::DeclContext*&, clang::DeclarationName&, clang::NamedDecl*&, clang::SourceLocation&) llvm-project/clang/lib/AST/ASTImporter.cpp:1624:0
#72 0x00007feb249aee01 clang::ASTNodeImporter::VisitClassTemplateDecl(clang::ClassTemplateDecl*) llvm-project/clang/lib/AST/ASTImporter.cpp:5355:0
#73 0x00007feb249fce9c clang::declvisitor::Base<std::add_pointer, clang::ASTNodeImporter, llvm::Expected<clang::Decl*> >::Visit(clang::Decl*) build/Debug/tools/clang/include/clang/AST/DeclNodes.inc:207:0
#74 0x00007feb249c5ca1 clang::ASTImporter::ImportImpl(clang::Decl*) llvm-project/clang/lib/AST/ASTImporter.cpp:8119:0
#75 0x00007feb249c6c53 clang::ASTImporter::Import(clang::Decl*) llvm-project/clang/lib/AST/ASTImporter.cpp:8281:0
#76 0x000055b24844006d clang::ast_matchers::ASTImporterTestBase::TU::import(std::shared_ptr<clang::ASTImporterSharedState> const&, clang::ASTUnit*, clang::Decl*) llvm-project/clang/unittests/AST/ASTImporterFixtures.cpp:83:0
#77 0x000055b248440db9 clang::ast_matchers::ASTImporterTestBase::Import(clang::Decl*, clang::TestLanguage) llvm-project/clang/unittests/AST/ASTImporterFixtures.cpp:199:0
#78 0x000055b2484dc5d6 clang::ClassTemplateDecl* clang::ast_matchers::ASTImporterTestBase::Import<clang::ClassTemplateDecl>(clang::ClassTemplateDecl*, clang::TestLanguage) llvm-project/clang/unittests/AST/ASTImporterFixtures.h:185:0
#79 0x000055b2484aa73c clang::ast_matchers::ASTImporterOptionSpecificTestBase_ImportOfTemplatedDeclWhenPreviousDeclHasNoDescribedTemplateSet_Test::TestBody() llvm-project/clang/unittests/AST/ASTImporterTest.cpp:6180:0
#80 0x00007feb235f3a04 void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:2402:0
#81 0x00007feb235ecda2 void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:2455:0
#82 0x00007feb235d3666 testing::Test::Run() llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:2481:0
#83 0x00007feb235d3e2a testing::TestInfo::Run() llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:2660:0
#84 0x00007feb235d441e testing::TestCase::Run() llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:2773:0
#85 0x00007feb235da33c testing::internal::UnitTestImpl::RunAllTests() llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:4647:0
#86 0x00007feb235f4cbf bool testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:2404:0
#87 0x00007feb235ed98b bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:2455:0
#88 0x00007feb235d91c5 testing::UnitTest::Run() llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:4260:0
#89 0x00007feb277c5c36 RUN_ALL_TESTS() llvm-project/llvm/utils/unittest/googletest/include/gtest/gtest.h:2234:0
#90 0x00007feb277c5b42 main llvm-project/llvm/utils/unittest/UnitTestMain/TestMain.cpp:51:0
#91 0x00007feb226d4bf7 __libc_start_main /build/glibc-S9d2JN/glibc-2.27/csu/../csu/libc-start.c:344:0
#92 0x000055b2483f51fa _start (./tools/clang/unittests/AST/ASTTests+0xe31fa)
Aborted (core dumped)
Harbormaster completed remote builds in B83990: Diff 314518.Jan 5 2021, 1:44 AM
balazske added a comment.EditedJan 5 2021, 2:03 AM

Order of previous declarations may get confusable:

ClassTemplateDecl1 <-(Prev)- ClassTemplateDecl2 <-(Prev)- ClassTemplateDecl3
|                            |                            | 
CXXRecorddDecl1 <--          CXXRecordDecl2 -----(Prev)-> CXXRecordDecl3 -
                  |                                                      |
                  --(Prev)------------------------------------------------

This is because record declarations are encountered in different order than the class templates.
And the import of template parameters is done in a incomplete state of the related record declaration.

balazske added a reviewer: martong.Jan 5 2021, 2:06 AM
Herald added a subscriber: rnkovacs. · View Herald TranscriptJan 5 2021, 2:06 AM
balazske updated this revision to Diff 314604.Jan 5 2021, 7:26 AM

Found a better solution.
And the decl chain looks more normal if this change is used.

balazske mentioned this in D92103: [ASTImporter] Import the default argument of TemplateTypeParmDecl.Jan 5 2021, 7:27 AM
Harbormaster completed remote builds in B84047: Diff 314604.Jan 5 2021, 8:14 AM
shafik added inline comments.Jan 5 2021, 3:49 PM
clang/lib/AST/ASTImporter.cpp
2901

Is this to fix the bug or is this for efficiency sake?

balazske added inline comments.Jan 5 2021, 11:48 PM
clang/lib/AST/ASTImporter.cpp
2901

This is not needed for the fix, it was used in the first version of the fix (still only for efficiency). In the current form this looks like unrelated change (the old fix included other code at the same location) so I am not against removing this part (but add it in a separate change).

shafik added inline comments.Jan 6 2021, 9:32 AM
clang/lib/AST/ASTImporter.cpp
2901

Yes, please if we can split the two changes that would be great.

shafik accepted this revision.Jan 6 2021, 9:32 AM
This revision is now accepted and ready to land.Jan 6 2021, 9:32 AM
balazske updated this revision to Diff 315055.Jan 6 2021, 11:26 PM

Removing unrelated (for the bug fix) change.

Harbormaster completed remote builds in B84293: Diff 315055.Jan 6 2021, 11:57 PM
Closed by commit rG0877b963ef2d: [clang][ASTImporter] Fix a possible assertion failure… (authored by balazske). · Explain WhyJan 7 2021, 2:21 AM
This revision was automatically updated to reflect the committed changes.
balazske added a commit: rG0877b963ef2d: [clang][ASTImporter] Fix a possible assertion failure….
balazske marked an inline comment as done.Jan 7 2021, 6:16 AM
balazske added inline comments.
clang/lib/AST/ASTImporter.cpp
2901

I checked if this "skip" is needed, but it turned out (at least on this test file) that in all cases (1060 total) the InjectedClassNameType is already there, so this loop would do nothing useful if the skip is added. And the same for the code in if (Injected) branch, the type is already InjectedClassNameType. I do not plan to make a new change for this issue, but it needs investigation to check if this change of the type is needed at all (if not, and why not, if yes, is the "skip" useful). Probably a FIXME could be added here.

martong added a comment.Jan 19 2021, 5:28 AM

@balazske
Thanks Balázs! Great work!

Revision Contents

PathSize
clang/
lib/
AST/
8 lines
unittests/
AST/
35 lines

Diff 315072

clang/lib/AST/ASTImporter.cpp

  • This file is larger than 256 KB, so syntax highlighting is disabled by default.
Show First 20 Lines • Show All 2,892 Lines • ▼ Show 20 Lines if (ClassTemplateDecl *FromDescribed =
} }
} }
// Create an injected type for the whole redecl chain. // Create an injected type for the whole redecl chain.
SmallVector<Decl *, 2> Redecls = SmallVector<Decl *, 2> Redecls =
getCanonicalForwardRedeclChain(D2CXX); getCanonicalForwardRedeclChain(D2CXX);
for (auto *R : Redecls) { for (auto *R : Redecls) {
auto *RI = cast<CXXRecordDecl>(R); auto *RI = cast<CXXRecordDecl>(R);
RI->setTypeForDecl(nullptr); RI->setTypeForDecl(nullptr);
// Below we create a new injected type and assign that to the // Below we create a new injected type and assign that to the
shafikUnsubmitted
Not Done
ReplyInline Actions

Is this to fix the bug or is this for efficiency sake?

shafik: Is this to fix the bug or is this for efficiency sake?
balazskeAuthorUnsubmitted
Done
ReplyInline Actions

This is not needed for the fix, it was used in the first version of the fix (still only for efficiency). In the current form this looks like unrelated change (the old fix included other code at the same location) so I am not against removing this part (but add it in a separate change).

balazske: This is not needed for the fix, it was used in the first version of the fix (still only for…
shafikUnsubmitted
Not Done
ReplyInline Actions

Yes, please if we can split the two changes that would be great.

shafik: Yes, please if we can split the two changes that would be great.
balazskeAuthorUnsubmitted
Done
ReplyInline Actions

I checked if this "skip" is needed, but it turned out (at least on this test file) that in all cases (1060 total) the InjectedClassNameType is already there, so this loop would do nothing useful if the skip is added. And the same for the code in if (Injected) branch, the type is already InjectedClassNameType. I do not plan to make a new change for this issue, but it needs investigation to check if this change of the type is needed at all (if not, and why not, if yes, is the "skip" useful). Probably a FIXME could be added here.

balazske: I checked if this "skip" is needed, but it turned out (at least on this test file) that in all…
// canonical decl, subsequent declarations in the chain will reuse // canonical decl, subsequent declarations in the chain will reuse
// that type. // that type.
Importer.getToContext().getInjectedClassNameType( Importer.getToContext().getInjectedClassNameType(
RI, ToDescribed->getInjectedClassNameSpecialization()); RI, ToDescribed->getInjectedClassNameSpecialization());
} }
// Set the new type for the previous injected decl too. // Set the new type for the previous injected decl too.
if (Injected) { if (Injected) {
Injected->setTypeForDecl(nullptr); Injected->setTypeForDecl(nullptr);
▲ Show 20 Lines • Show All 2,478 Lines • ▼ Show 20 Lines if (!ConflictingDecls.empty()) {
Name = NameOrErr.get(); Name = NameOrErr.get();
else else
return NameOrErr.takeError(); return NameOrErr.takeError();
} }
} }
CXXRecordDecl *FromTemplated = D->getTemplatedDecl(); CXXRecordDecl *FromTemplated = D->getTemplatedDecl();
auto TemplateParamsOrErr = import(D->getTemplateParameters());
if (!TemplateParamsOrErr)
return TemplateParamsOrErr.takeError();
// Create the declaration that is being templated. // Create the declaration that is being templated.
CXXRecordDecl *ToTemplated; CXXRecordDecl *ToTemplated;
if (Error Err = importInto(ToTemplated, FromTemplated)) if (Error Err = importInto(ToTemplated, FromTemplated))
return std::move(Err); return std::move(Err);
// Create the class template declaration itself. // Create the class template declaration itself.
auto TemplateParamsOrErr = import(D->getTemplateParameters());
if (!TemplateParamsOrErr)
return TemplateParamsOrErr.takeError();
ClassTemplateDecl *D2; ClassTemplateDecl *D2;
if (GetImportedOrCreateDecl(D2, D, Importer.getToContext(), DC, Loc, Name, if (GetImportedOrCreateDecl(D2, D, Importer.getToContext(), DC, Loc, Name,
*TemplateParamsOrErr, ToTemplated)) *TemplateParamsOrErr, ToTemplated))
return D2; return D2;
ToTemplated->setDescribedClassTemplate(D2); ToTemplated->setDescribedClassTemplate(D2);
D2->setAccess(D->getAccess()); D2->setAccess(D->getAccess());
▲ Show 20 Lines • Show All 3,899 Lines • Show Last 20 Lines

clang/unittests/AST/ASTImporterTest.cpp

Show First 20 Lines • Show All 6,135 Lines • ▼ Show 20 LinesTEST_P(ASTImporterOptionSpecificTestBase, TypedefWithAttribute) {
auto *ToD = Import(FromD, Lang_CXX17); auto *ToD = Import(FromD, Lang_CXX17);
ASSERT_TRUE(ToD); ASSERT_TRUE(ToD);
ASSERT_EQ(ToD->getAttrs().size(), 1U); ASSERT_EQ(ToD->getAttrs().size(), 1U);
auto *ToAttr = dyn_cast<AnnotateAttr>(ToD->getAttrs()[0]); auto *ToAttr = dyn_cast<AnnotateAttr>(ToD->getAttrs()[0]);
ASSERT_TRUE(ToAttr); ASSERT_TRUE(ToAttr);
EXPECT_EQ(ToAttr->getAnnotation(), "A"); EXPECT_EQ(ToAttr->getAnnotation(), "A");
} }
TEST_P(ASTImporterOptionSpecificTestBase,
ImportOfTemplatedDeclWhenPreviousDeclHasNoDescribedTemplateSet) {
Decl *FromTU = getTuDecl(
R"(
namespace std {
template<typename T>
class basic_stringbuf;
}
namespace std {
class char_traits;
template<typename T = char_traits>
class basic_stringbuf;
}
namespace std {
template<typename T>
class basic_stringbuf {};
}
)",
Lang_CXX11);
auto *From1 = FirstDeclMatcher<ClassTemplateDecl>().match(
FromTU,
classTemplateDecl(hasName("basic_stringbuf"), unless(isImplicit())));
auto *To1 = cast_or_null<ClassTemplateDecl>(Import(From1, Lang_CXX11));
EXPECT_TRUE(To1);
auto *From2 = LastDeclMatcher<ClassTemplateDecl>().match(
FromTU,
classTemplateDecl(hasName("basic_stringbuf"), unless(isImplicit())));
auto *To2 = cast_or_null<ClassTemplateDecl>(Import(From2, Lang_CXX11));
EXPECT_TRUE(To2);
}
INSTANTIATE_TEST_CASE_P(ParameterizedTests, ASTImporterLookupTableTest, INSTANTIATE_TEST_CASE_P(ParameterizedTests, ASTImporterLookupTableTest,
DefaultTestValuesForRunOptions, ); DefaultTestValuesForRunOptions, );
INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportPath, INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportPath,
::testing::Values(std::vector<std::string>()), ); ::testing::Values(std::vector<std::string>()), );
INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportExpr, INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportExpr,
DefaultTestValuesForRunOptions, ); DefaultTestValuesForRunOptions, );
▲ Show 20 Lines • Show All 63 Lines • Show Last 20 Lines