This is an archive of the discontinued LLVM Phabricator instance.

[ASTImporter] Support CXXDeductionGuideDecl with local typedef
ClosedPublic

Authored by martong on Nov 27 2020, 12:13 AM.

Details

Summary

CXXDeductionGuideDecl with a local typedef has its own copy of the
TypedefDecl with the CXXDeductionGuideDecl as the DeclContext of that
TypedefDecl.

template <typename T> struct A {
  typedef T U;
  A(U, T);
};
A a{(int)0, (int)0};

Related discussion on cfe-dev:
http://lists.llvm.org/pipermail/cfe-dev/2020-November/067252.html

Without this fix, when we import the CXXDeductionGuideDecl (via
VisitFunctionDecl) then before creating the Decl we must import the
FunctionType. However, the first parameter's type is the afore mentioned
local typedef. So, we then start importing the TypedefDecl whose
DeclContext is the CXXDeductionGuideDecl itself. The infinite loop is
formed.

#0 clang::ASTNodeImporter::VisitCXXDeductionGuideDecl(clang::CXXDeductionGuideDecl*) clang/lib/AST/ASTImporter.cpp:3543:0
#1 clang::declvisitor::Base<std::add_pointer, clang::ASTNodeImporter, llvm::Expected<clang::Decl*> >::Visit(clang::Decl*) /home/egbomrt/WORK/llvm5/build/debug/tools/clang/include/clang/AST/DeclNodes.inc:405:0
#2 clang::ASTImporter::ImportImpl(clang::Decl*) clang/lib/AST/ASTImporter.cpp:8038:0
#3 clang::ASTImporter::Import(clang::Decl*) clang/lib/AST/ASTImporter.cpp:8200:0
#4 clang::ASTImporter::ImportContext(clang::DeclContext*) clang/lib/AST/ASTImporter.cpp:8297:0
#5 clang::ASTNodeImporter::ImportDeclContext(clang::Decl*, clang::DeclContext*&, clang::DeclContext*&) clang/lib/AST/ASTImporter.cpp:1852:0
#6 clang::ASTNodeImporter::ImportDeclParts(clang::NamedDecl*, clang::DeclContext*&, clang::DeclContext*&, clang::DeclarationName&, clang::NamedDecl*&, clang::SourceLocation&) clang/lib/AST/ASTImporter.cpp:1628:0
#7 clang::ASTNodeImporter::VisitTypedefNameDecl(clang::TypedefNameDecl*, bool) clang/lib/AST/ASTImporter.cpp:2419:0
#8 clang::ASTNodeImporter::VisitTypedefDecl(clang::TypedefDecl*) clang/lib/AST/ASTImporter.cpp:2500:0
#9 clang::declvisitor::Base<std::add_pointer, clang::ASTNodeImporter, llvm::Expected<clang::Decl*> >::Visit(clang::Decl*) /home/egbomrt/WORK/llvm5/build/debug/tools/clang/include/clang/AST/DeclNodes.inc:315:0
#10 clang::ASTImporter::ImportImpl(clang::Decl*) clang/lib/AST/ASTImporter.cpp:8038:0
#11 clang::ASTImporter::Import(clang::Decl*) clang/lib/AST/ASTImporter.cpp:8200:0
#12 llvm::Expected<clang::TypedefNameDecl*> clang::ASTNodeImporter::import<clang::TypedefNameDecl>(clang::TypedefNameDecl*) clang/lib/AST/ASTImporter.cpp:165:0
#13 clang::ASTNodeImporter::VisitTypedefType(clang::TypedefType const*) clang/lib/AST/ASTImporter.cpp:1304:0
#14 clang::TypeVisitor<clang::ASTNodeImporter, llvm::Expected<clang::QualType> >::Visit(clang::Type const*) /home/egbomrt/WORK/llvm5/build/debug/tools/clang/include/clang/AST/TypeNodes.inc:74:0
#15 clang::ASTImporter::Import(clang::QualType) clang/lib/AST/ASTImporter.cpp:8071:0
#16 llvm::Expected<clang::QualType> clang::ASTNodeImporter::import<clang::QualType>(clang::QualType const&) clang/lib/AST/ASTImporter.cpp:179:0
#17 clang::ASTNodeImporter::VisitFunctionProtoType(clang::FunctionProtoType const*) clang/lib/AST/ASTImporter.cpp:1244:0
#18 clang::TypeVisitor<clang::ASTNodeImporter, llvm::Expected<clang::QualType> >::Visit(clang::Type const*) /home/egbomrt/WORK/llvm5/build/debug/tools/clang/include/clang/AST/TypeNodes.inc:47:0
#19 clang::ASTImporter::Import(clang::QualType) clang/lib/AST/ASTImporter.cpp:8071:0
#20 llvm::Expected<clang::QualType> clang::ASTNodeImporter::import<clang::QualType>(clang::QualType const&) clang/lib/AST/ASTImporter.cpp:179:0
#21 clang::QualType clang::ASTNodeImporter::importChecked<clang::QualType>(llvm::Error&, clang::QualType const&) clang/lib/AST/ASTImporter.cpp:198:0
#22 clang::ASTNodeImporter::VisitFunctionDecl(clang::FunctionDecl*) clang/lib/AST/ASTImporter.cpp:3313:0
#23 clang::ASTNodeImporter::VisitCXXDeductionGuideDecl(clang::CXXDeductionGuideDecl*) clang/lib/AST/ASTImporter.cpp:3543:0

The fix is to first create the TypedefDecl and only then start to import
the DeclContext.
Basically, we could do this during the import of all other Decls (not
just for typedefs). But it seems, there is only one another AST
construct that has a similar cycle: a struct defined as a function
parameter:

int struct_in_proto(struct data_t{int a;int b;} *d);

In that case, however, we had decided to return simply with an error
back then because that seemed to be a very rare construct.

Diff Detail

Event Timeline

martong created this revision.Nov 27 2020, 12:13 AM
martong requested review of this revision.Nov 27 2020, 12:13 AM
shafik added inline comments.Dec 2 2020, 5:08 PM
clang/lib/AST/ASTImporter.cpp
2522

I am not super excited about this solution, I feel like several bugs we have had can be attributed to these exception control flow cases that we have in the ASTImporter. I don't have any immediate better solution.

martong marked an inline comment as done.Dec 3 2020, 9:31 AM
martong added inline comments.
clang/lib/AST/ASTImporter.cpp
2522

Before uploading this patch I had been thinking about several other solutions
but all of them had some problems:

Detect the loop in the AST and return with UnsupportedConstruct. We could do
the detection with the help of ImportPath. However, I realized this approach is
way too defensive and removes the support for an important AST node which is
against our goals.

Try to eliminate the looping contruct from the AST. I could do that for some
cases (D92101) but there are still cases when the Sema must create such a loop
deliberately (the test case in this patch shows that).

It is essential to add a newly created Decl to the lookupTable ASAP because it
makes it possible for subsequent imports to find the existing Decl and this way
avoiding the creation of duplicated Decls. This is why AddToLookupTable is
called in MapImported. But the lookuptable operates on the DeclContext of the
Decl, and in this patch we must not import the DeclContext before.
Consequently, we have to add to the loopkuptable once the DeclContext is
imported. Perhaps, we could provide an optional lambda function to
GetImportedOrCreateDecl which would be called before MapImported (and that
lambda would do the DC import), but this seems even more clumsy.

BTW, the lookuptable is not used in LLDB, there you use the traditional lookup
implemented in DeclContext (addDeclInternal and noload_lookup). One problem
with noload_lookup is that it does not find some Decls because it obeys to C++
visibility rules, thus new duplicated Decls are created. The ASTImporter must
be able to lookup e.g. template specialization Decls too, in order to avoid
creating a redundant duplicated clone and this is the task of the lookupTable.
I hope one day LLDB would be able to switch to ASTImporterLookupTable from
noload_lookup. The other problem is with addDeclInternal: we call this later,
not in MapImported. The only responsibility attached to the use of addDeclInternal
should be to clone the visibility as it is in the source context (and not managing
the do-not-create-duplicate-decls issue).

So yes, there are many exceptional control flow cases, but most of them stems
from the complexity of trying to support two different needs: noload_lookup and
minimal import are needed exceptionally for LLDB. I was thinking about to
create a separate ASTImporter implementation specifically for CTU and LLDB
could have it's own implementation. For that we just need to create an
interface class with virtual functions. Having two different implementations
could save us from braking each others tests and this could be a big gain, WDYT?
(On the other hand, we'd loose updates and fixes from the other team.)

martong marked an inline comment as done.Dec 3 2020, 10:27 AM

I was thinking about to create a separate ASTImporter implementation specifically for CTU and LLDB could have it's own implementation. For that we just need to create an interface class with virtual functions.

One implementation could reside in libCrossTU and the other in LLDB. At least, that's what I thought. Unfortunately there is an obstacle which renders this whole idea practically unfeasible: Currently ASTImporter and ASTNodeImporter are friends of almost all AST classes.

Ping.
@teemperor please take a look if you have some time. This is a really important patch which may influence some future directions regarding the ASTImporter.

Ping.
@teemperor please take a look if you have some time. This is a really important patch which may influence some future directions regarding the ASTImporter.

Apologies, last week was very busy here. I'll review this tomorrow morning.

teemperor accepted this revision.Dec 8 2020, 8:23 AM

There are several LLDB tests failing with this change. The TestImportBaseClassWhenClassHasDerivedMember.py is probably the only relevant one. The others are probably failing for the same reason but with a more convoluted setup. I looked into the failure and it seems this patch is now skipping the workaround in ImportContext that was introduced in D78000 (as the Importer.GetAlreadyImportedOrNull will give us a DeclContext and then we just use that as-is). If you remove the if (!DC || !LexicalDC) before the if ((Err = ImportDeclContext(D, DC, LexicalDC))) this should keep the old behaviour.

Beside that issue this LGTM. Removing the 'if' seems straightforward, so I'll accept this. Thanks!

clang/lib/AST/ASTImporter.cpp
2522

I hope one day LLDB would be able to switch to ASTImporterLookupTable from noload_lookup.

Done here: https://reviews.llvm.org/D92848

clang/unittests/AST/ASTImporterTest.cpp
5932

Second parameter seems not relevant for the test?

This revision is now accepted and ready to land.Dec 8 2020, 8:23 AM
martong updated this revision to Diff 310510.Dec 9 2020, 6:08 AM
  • Remove if (!DC || !LexicalDC)
martong marked an inline comment as done.Dec 9 2020, 6:11 AM

Thanks for the review guys!

clang/lib/AST/ASTImporter.cpp
2522

Thanks, this could enable some simplifications in the ASTImporter then.

martong updated this revision to Diff 310591.Dec 9 2020, 11:41 AM
martong marked an inline comment as done.
  • Remove not relevant param from test
This revision was landed with ongoing or failed builds.Dec 9 2020, 12:25 PM
This revision was automatically updated to reflect the committed changes.