[PATCH] D92209: [ASTImporter] Support CXXDeductionGuideDecl with local typedef

[Gabor Marton via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGa5e6590b15b1: [ASTImporter] Support CXXDeductionGuideDecl 
with local typedef (authored by martong).

Changed prior to commit:
  https://reviews.llvm.org/D92209?vs=310591=310611#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92209/new/

https://reviews.llvm.org/D92209

Files:
  clang/lib/AST/ASTImporter.cpp
  clang/unittests/AST/ASTImporterTest.cpp

Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -5998,6 +5998,22 @@
   EXPECT_TRUE(ToD->isExplicit());
 }
 
+TEST_P(ImportFunctions, CTADWithLocalTypedef) {
+  Decl *TU = getTuDecl(
+  R"(
+  template  struct A {
+typedef T U;
+A(U);
+  };
+  A a{(int)0};
+  )",
+  Lang_CXX17, "input.cc");
+  auto *FromD = FirstDeclMatcher().match(
+  TU, cxxDeductionGuideDecl());
+  auto *ToD = Import(FromD, Lang_CXX17);
+  ASSERT_TRUE(ToD);
+}
+
 // FIXME Move these tests out of ASTImporterTest. For that we need to factor
 // out the ASTImporter specific pars from ASTImporterOptionSpecificTestBase
 // into a new test Fixture. Then we should lift up this Fixture to its own
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -388,6 +388,8 @@
 ExpectedType VisitObjCObjectPointerType(const ObjCObjectPointerType *T);
 
 // Importing declarations
+Error ImportDeclParts(NamedDecl *D, DeclarationName , NamedDecl *,
+  SourceLocation );
 Error ImportDeclParts(
 NamedDecl *D, DeclContext *, DeclContext *,
 DeclarationName , NamedDecl *, SourceLocation );
@@ -1647,6 +1649,25 @@
   return Error::success();
 }
 
+Error ASTNodeImporter::ImportDeclParts(NamedDecl *D, DeclarationName ,
+   NamedDecl *, SourceLocation ) {
+
+  // Import the name of this declaration.
+  if (Error Err = importInto(Name, D->getDeclName()))
+return Err;
+
+  // Import the location of this declaration.
+  if (Error Err = importInto(Loc, D->getLocation()))
+return Err;
+
+  ToD = cast_or_null(Importer.GetAlreadyImportedOrNull(D));
+  if (ToD)
+if (Error Err = ASTNodeImporter(*this).ImportDefinitionIfNeeded(D, ToD))
+  return Err;
+
+  return Error::success();
+}
+
 Error ASTNodeImporter::ImportDefinitionIfNeeded(Decl *FromD, Decl *ToD) {
   if (!FromD)
 return Error::success();
@@ -2415,22 +2436,29 @@
 ExpectedDecl
 ASTNodeImporter::VisitTypedefNameDecl(TypedefNameDecl *D, bool IsAlias) {
   // Import the major distinguishing characteristics of this typedef.
-  DeclContext *DC, *LexicalDC;
   DeclarationName Name;
   SourceLocation Loc;
   NamedDecl *ToD;
-  if (Error Err = ImportDeclParts(D, DC, LexicalDC, Name, ToD, Loc))
+  // Do not import the DeclContext, we will import it once the TypedefNameDecl
+  // is created.
+  if (Error Err = ImportDeclParts(D, Name, ToD, Loc))
 return std::move(Err);
   if (ToD)
 return ToD;
 
+  DeclContext *DC = cast_or_null(
+  Importer.GetAlreadyImportedOrNull(cast(D->getDeclContext(;
+  DeclContext *LexicalDC =
+  cast_or_null(Importer.GetAlreadyImportedOrNull(
+  cast(D->getLexicalDeclContext(;
+
   // If this typedef is not in block scope, determine whether we've
   // seen a typedef with the same name (that we can merge with) or any
   // other entity by that name (which name lookup could conflict with).
   // Note: Repeated typedefs are not valid in C99:
   // 'typedef int T; typedef int T;' is invalid
   // We do not care about this now.
-  if (!DC->isFunctionOrMethod()) {
+  if (DC && !DC->isFunctionOrMethod()) {
 SmallVector ConflictingDecls;
 unsigned IDNS = Decl::IDNS_Ordinary;
 auto FoundDecls = Importer.findDeclsInToCtx(DC, Name);
@@ -2487,8 +2515,15 @@
   Name.getAsIdentifierInfo(), ToTypeSourceInfo))
 return ToTypedef;
 
-  ToTypedef->setAccess(D->getAccess());
+  // Import the DeclContext and set it to the Typedef.
+  if ((Err = ImportDeclContext(D, DC, LexicalDC)))
+return std::move(Err);
+  ToTypedef->setDeclContext(DC);
   ToTypedef->setLexicalDeclContext(LexicalDC);
+  // Add to the lookupTable because we could not do that in MapImported.
+  Importer.AddToLookupTable(ToTypedef);
+
+  ToTypedef->setAccess(D->getAccess());
 
   // Templated declarations should not appear in DeclContext.
   TypeAliasDecl *FromAlias = IsAlias ? cast(D) : nullptr;
@@ -9198,7 +9233,11 @@
   // This mapping should be maintained only in this function. Therefore do not
   // check for additional consistency.
   ImportedFromDecls[To] = From;
-  AddToLookupTable(To);
+  // In the case of TypedefNameDecl we 

[PATCH] D92209: [ASTImporter] Support CXXDeductionGuideDecl with local typedef

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 310591.
martong marked an inline comment as done.
martong added a comment.

- Remove not relevant param from test


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92209/new/

https://reviews.llvm.org/D92209

Files:
  clang/lib/AST/ASTImporter.cpp
  clang/unittests/AST/ASTImporterTest.cpp

Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -5925,6 +5925,22 @@
   EXPECT_TRUE(ToD->isExplicit());
 }
 
+TEST_P(ImportFunctions, CTADWithLocalTypedef) {
+  Decl *TU = getTuDecl(
+  R"(
+  template  struct A {
+typedef T U;
+A(U);
+  };
+  A a{(int)0};
+  )",
+  Lang_CXX17, "input.cc");
+  auto *FromD = FirstDeclMatcher().match(
+  TU, cxxDeductionGuideDecl());
+  auto *ToD = Import(FromD, Lang_CXX17);
+  ASSERT_TRUE(ToD);
+}
+
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ASTImporterLookupTableTest,
 DefaultTestValuesForRunOptions, );
 
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -385,6 +385,8 @@
 ExpectedType VisitObjCObjectPointerType(const ObjCObjectPointerType *T);
 
 // Importing declarations
+Error ImportDeclParts(NamedDecl *D, DeclarationName , NamedDecl *,
+  SourceLocation );
 Error ImportDeclParts(
 NamedDecl *D, DeclContext *, DeclContext *,
 DeclarationName , NamedDecl *, SourceLocation );
@@ -1644,6 +1646,25 @@
   return Error::success();
 }
 
+Error ASTNodeImporter::ImportDeclParts(NamedDecl *D, DeclarationName ,
+   NamedDecl *, SourceLocation ) {
+
+  // Import the name of this declaration.
+  if (Error Err = importInto(Name, D->getDeclName()))
+return Err;
+
+  // Import the location of this declaration.
+  if (Error Err = importInto(Loc, D->getLocation()))
+return Err;
+
+  ToD = cast_or_null(Importer.GetAlreadyImportedOrNull(D));
+  if (ToD)
+if (Error Err = ASTNodeImporter(*this).ImportDefinitionIfNeeded(D, ToD))
+  return Err;
+
+  return Error::success();
+}
+
 Error ASTNodeImporter::ImportDefinitionIfNeeded(Decl *FromD, Decl *ToD) {
   if (!FromD)
 return Error::success();
@@ -2412,22 +2433,29 @@
 ExpectedDecl
 ASTNodeImporter::VisitTypedefNameDecl(TypedefNameDecl *D, bool IsAlias) {
   // Import the major distinguishing characteristics of this typedef.
-  DeclContext *DC, *LexicalDC;
   DeclarationName Name;
   SourceLocation Loc;
   NamedDecl *ToD;
-  if (Error Err = ImportDeclParts(D, DC, LexicalDC, Name, ToD, Loc))
+  // Do not import the DeclContext, we will import it once the TypedefNameDecl
+  // is created.
+  if (Error Err = ImportDeclParts(D, Name, ToD, Loc))
 return std::move(Err);
   if (ToD)
 return ToD;
 
+  DeclContext *DC = cast_or_null(
+  Importer.GetAlreadyImportedOrNull(cast(D->getDeclContext(;
+  DeclContext *LexicalDC =
+  cast_or_null(Importer.GetAlreadyImportedOrNull(
+  cast(D->getLexicalDeclContext(;
+
   // If this typedef is not in block scope, determine whether we've
   // seen a typedef with the same name (that we can merge with) or any
   // other entity by that name (which name lookup could conflict with).
   // Note: Repeated typedefs are not valid in C99:
   // 'typedef int T; typedef int T;' is invalid
   // We do not care about this now.
-  if (!DC->isFunctionOrMethod()) {
+  if (DC && !DC->isFunctionOrMethod()) {
 SmallVector ConflictingDecls;
 unsigned IDNS = Decl::IDNS_Ordinary;
 auto FoundDecls = Importer.findDeclsInToCtx(DC, Name);
@@ -2484,8 +2512,15 @@
   Name.getAsIdentifierInfo(), ToTypeSourceInfo))
 return ToTypedef;
 
-  ToTypedef->setAccess(D->getAccess());
+  // Import the DeclContext and set it to the Typedef.
+  if ((Err = ImportDeclContext(D, DC, LexicalDC)))
+return std::move(Err);
+  ToTypedef->setDeclContext(DC);
   ToTypedef->setLexicalDeclContext(LexicalDC);
+  // Add to the lookupTable because we could not do that in MapImported.
+  Importer.AddToLookupTable(ToTypedef);
+
+  ToTypedef->setAccess(D->getAccess());
 
   // Templated declarations should not appear in DeclContext.
   TypeAliasDecl *FromAlias = IsAlias ? cast(D) : nullptr;
@@ -9189,7 +9224,11 @@
   // This mapping should be maintained only in this function. Therefore do not
   // check for additional consistency.
   ImportedFromDecls[To] = From;
-  AddToLookupTable(To);
+  // In the case of TypedefNameDecl we create the Decl first and only then we
+  // import and set its DeclContext. So, the DC is still not set when we reach
+  // here from GetImportedOrCreateDecl.
+  if (To->getDeclContext())
+AddToLookupTable(To);
   return To;
 }
 

[PATCH] D92209: [ASTImporter] Support CXXDeductionGuideDecl with local typedef

[Gabor Marton via Phabricator via cfe-commits]

martong marked an inline comment as done.
martong added a comment.

Thanks for the review guys!




Comment at: clang/lib/AST/ASTImporter.cpp:2522
+  // Add to the lookupTable because we could not do that in MapImported.
+  Importer.AddToLookupTable(ToTypedef);
+

teemperor wrote:
> martong wrote:
> > shafik wrote:
> > > 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.
> > > 
> > > 
> > Before uploading this patch I had been thinking about several other 
> > solutions
> > but all of them had some problems:
> > 
> > 1) 
> > 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.
> > 
> > 2) 
> > 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.)
> > 
> > I hope one day LLDB would be able to switch to ASTImporterLookupTable from 
> > noload_lookup.
> 
> Done here: https://reviews.llvm.org/D92848
Thanks, this could enable some simplifications in the ASTImporter then.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92209/new/

https://reviews.llvm.org/D92209

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D92209: [ASTImporter] Support CXXDeductionGuideDecl with local typedef

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 310510.
martong added a comment.

- Remove `if (!DC || !LexicalDC)`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92209/new/

https://reviews.llvm.org/D92209

Files:
  clang/lib/AST/ASTImporter.cpp
  clang/unittests/AST/ASTImporterTest.cpp

Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -5925,6 +5925,22 @@
   EXPECT_TRUE(ToD->isExplicit());
 }
 
+TEST_P(ImportFunctions, CTADWithLocalTypedef) {
+  Decl *TU = getTuDecl(
+  R"(
+  template  struct A {
+typedef T U;
+A(U, T);
+  };
+  A a{(int)0, (int)0};
+  )",
+  Lang_CXX17, "input.cc");
+  auto *FromD = FirstDeclMatcher().match(
+  TU, cxxDeductionGuideDecl());
+  auto *ToD = Import(FromD, Lang_CXX17);
+  ASSERT_TRUE(ToD);
+}
+
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ASTImporterLookupTableTest,
 DefaultTestValuesForRunOptions, );
 
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -385,6 +385,8 @@
 ExpectedType VisitObjCObjectPointerType(const ObjCObjectPointerType *T);
 
 // Importing declarations
+Error ImportDeclParts(NamedDecl *D, DeclarationName , NamedDecl *,
+  SourceLocation );
 Error ImportDeclParts(
 NamedDecl *D, DeclContext *, DeclContext *,
 DeclarationName , NamedDecl *, SourceLocation );
@@ -1644,6 +1646,25 @@
   return Error::success();
 }
 
+Error ASTNodeImporter::ImportDeclParts(NamedDecl *D, DeclarationName ,
+   NamedDecl *, SourceLocation ) {
+
+  // Import the name of this declaration.
+  if (Error Err = importInto(Name, D->getDeclName()))
+return Err;
+
+  // Import the location of this declaration.
+  if (Error Err = importInto(Loc, D->getLocation()))
+return Err;
+
+  ToD = cast_or_null(Importer.GetAlreadyImportedOrNull(D));
+  if (ToD)
+if (Error Err = ASTNodeImporter(*this).ImportDefinitionIfNeeded(D, ToD))
+  return Err;
+
+  return Error::success();
+}
+
 Error ASTNodeImporter::ImportDefinitionIfNeeded(Decl *FromD, Decl *ToD) {
   if (!FromD)
 return Error::success();
@@ -2412,22 +2433,29 @@
 ExpectedDecl
 ASTNodeImporter::VisitTypedefNameDecl(TypedefNameDecl *D, bool IsAlias) {
   // Import the major distinguishing characteristics of this typedef.
-  DeclContext *DC, *LexicalDC;
   DeclarationName Name;
   SourceLocation Loc;
   NamedDecl *ToD;
-  if (Error Err = ImportDeclParts(D, DC, LexicalDC, Name, ToD, Loc))
+  // Do not import the DeclContext, we will import it once the TypedefNameDecl
+  // is created.
+  if (Error Err = ImportDeclParts(D, Name, ToD, Loc))
 return std::move(Err);
   if (ToD)
 return ToD;
 
+  DeclContext *DC = cast_or_null(
+  Importer.GetAlreadyImportedOrNull(cast(D->getDeclContext(;
+  DeclContext *LexicalDC =
+  cast_or_null(Importer.GetAlreadyImportedOrNull(
+  cast(D->getLexicalDeclContext(;
+
   // If this typedef is not in block scope, determine whether we've
   // seen a typedef with the same name (that we can merge with) or any
   // other entity by that name (which name lookup could conflict with).
   // Note: Repeated typedefs are not valid in C99:
   // 'typedef int T; typedef int T;' is invalid
   // We do not care about this now.
-  if (!DC->isFunctionOrMethod()) {
+  if (DC && !DC->isFunctionOrMethod()) {
 SmallVector ConflictingDecls;
 unsigned IDNS = Decl::IDNS_Ordinary;
 auto FoundDecls = Importer.findDeclsInToCtx(DC, Name);
@@ -2484,8 +2512,15 @@
   Name.getAsIdentifierInfo(), ToTypeSourceInfo))
 return ToTypedef;
 
-  ToTypedef->setAccess(D->getAccess());
+  // Import the DeclContext and set it to the Typedef.
+  if ((Err = ImportDeclContext(D, DC, LexicalDC)))
+return std::move(Err);
+  ToTypedef->setDeclContext(DC);
   ToTypedef->setLexicalDeclContext(LexicalDC);
+  // Add to the lookupTable because we could not do that in MapImported.
+  Importer.AddToLookupTable(ToTypedef);
+
+  ToTypedef->setAccess(D->getAccess());
 
   // Templated declarations should not appear in DeclContext.
   TypeAliasDecl *FromAlias = IsAlias ? cast(D) : nullptr;
@@ -9189,7 +9224,11 @@
   // This mapping should be maintained only in this function. Therefore do not
   // check for additional consistency.
   ImportedFromDecls[To] = From;
-  AddToLookupTable(To);
+  // In the case of TypedefNameDecl we create the Decl first and only then we
+  // import and set its DeclContext. So, the DC is still not set when we reach
+  // here from GetImportedOrCreateDecl.
+  if (To->getDeclContext())
+AddToLookupTable(To);
   return To;
 }
 
___
cfe-commits mailing list

[PATCH] D92209: [ASTImporter] Support CXXDeductionGuideDecl with local typedef

[Raphael Isemann via Phabricator via cfe-commits]

teemperor accepted this revision.
teemperor added a comment.
This revision is now accepted and ready to land.

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!




Comment at: clang/lib/AST/ASTImporter.cpp:2522
+  // Add to the lookupTable because we could not do that in MapImported.
+  Importer.AddToLookupTable(ToTypedef);
+

martong wrote:
> shafik wrote:
> > 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.
> > 
> > 
> Before uploading this patch I had been thinking about several other solutions
> but all of them had some problems:
> 
> 1) 
> 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.
> 
> 2) 
> 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.)
> 
> I hope one day LLDB would be able to switch to ASTImporterLookupTable from 
> noload_lookup.

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



Comment at: clang/unittests/AST/ASTImporterTest.cpp:5932
+typedef T U;
+A(U, T);
+  };

Second parameter seems not relevant for the test?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92209/new/

https://reviews.llvm.org/D92209

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D92209: [ASTImporter] Support CXXDeductionGuideDecl with local typedef

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added a comment.

In D92209#2437416 , @martong wrote:

> 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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92209/new/

https://reviews.llvm.org/D92209

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D92209: [ASTImporter] Support CXXDeductionGuideDecl with local typedef

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92209/new/

https://reviews.llvm.org/D92209

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D92209: [ASTImporter] Support CXXDeductionGuideDecl with local typedef

[Gabor Marton via Phabricator via cfe-commits]

martong marked an inline comment as done.
martong added a comment.

> 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 `friend`s of almost all AST classes.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92209/new/

https://reviews.llvm.org/D92209

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D92209: [ASTImporter] Support CXXDeductionGuideDecl with local typedef

[Gabor Marton via Phabricator via cfe-commits]

martong marked an inline comment as done.
martong added inline comments.



Comment at: clang/lib/AST/ASTImporter.cpp:2522
+  // Add to the lookupTable because we could not do that in MapImported.
+  Importer.AddToLookupTable(ToTypedef);
+

shafik wrote:
> 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.
> 
> 
Before uploading this patch I had been thinking about several other solutions
but all of them had some problems:

1) 
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.

2) 
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.)



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92209/new/

https://reviews.llvm.org/D92209

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D92209: [ASTImporter] Support CXXDeductionGuideDecl with local typedef

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added inline comments.



Comment at: clang/lib/AST/ASTImporter.cpp:2522
+  // Add to the lookupTable because we could not do that in MapImported.
+  Importer.AddToLookupTable(ToTypedef);
+

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.




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92209/new/

https://reviews.llvm.org/D92209

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D92209: [ASTImporter] Support CXXDeductionGuideDecl with local typedef

[Gabor Marton via Phabricator via cfe-commits]

martong created this revision.
martong added reviewers: teemperor, balazske.
Herald added subscribers: cfe-commits, gamesh411, Szelethus, dkrupp, rnkovacs.
Herald added a reviewer: a.sidorin.
Herald added a reviewer: shafik.
Herald added a project: clang.
martong requested review of this revision.

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

  template  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 >::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 >::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::ASTNodeImporter::import(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 >::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::ASTNodeImporter::import(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 >::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::ASTNodeImporter::import(clang::QualType const&) 
clang/lib/AST/ASTImporter.cpp:179:0
  #21 clang::QualType 
clang::ASTNodeImporter::importChecked(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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D92209

Files:
  clang/lib/AST/ASTImporter.cpp
  clang/unittests/AST/ASTImporterTest.cpp

Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -5924,6 +5924,22 @@
   EXPECT_TRUE(ToD->isExplicit());
 }
 
+TEST_P(ImportFunctions, CTADWithLocalTypedef) {
+  Decl *TU = getTuDecl(
+  R"(
+  template  struct A {
+typedef T U;
+A(U, T);
+  };
+  A a{(int)0, (int)0};
+  )",
+  Lang_CXX17, "input.cc");
+  auto *FromD =