[PATCH] D129640: [clang][ASTImporter] Improved handling of functions with auto return type.
This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGacd80a29ae7d: [clang][ASTImporter] Improved handling of functions with auto return type. (authored by balazske). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129640/new/ https://reviews.llvm.org/D129640 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 @@ -6320,6 +6320,24 @@ struct ImportAutoFunctions : ASTImporterOptionSpecificTestBase {}; +TEST_P(ImportAutoFunctions, ReturnWithTemplateWithIntegerArgDeclaredInside) { + Decl *FromTU = getTuDecl( + R"( + template struct Tmpl {}; + auto foo() { +constexpr int X = 1; +return Tmpl(); + } + )", + Lang_CXX14, "input0.cc"); + FunctionDecl *From = FirstDeclMatcher().match( + FromTU, functionDecl(hasName("foo"))); + + FunctionDecl *To = Import(From, Lang_CXX14); + EXPECT_TRUE(To); + EXPECT_TRUE(isa(To->getReturnType())); +} + TEST_P(ImportAutoFunctions, ReturnWithTemplateWithStructDeclaredInside1) { Decl *FromTU = getTuDecl( R"( Index: clang/lib/AST/ASTImporter.cpp === --- clang/lib/AST/ASTImporter.cpp +++ clang/lib/AST/ASTImporter.cpp @@ -12,9 +12,9 @@ //===--===// #include "clang/AST/ASTImporter.h" -#include "clang/AST/ASTImporterSharedState.h" #include "clang/AST/ASTContext.h" #include "clang/AST/ASTDiagnostic.h" +#include "clang/AST/ASTImporterSharedState.h" #include "clang/AST/ASTStructuralEquivalence.h" #include "clang/AST/Attr.h" #include "clang/AST/Decl.h" @@ -34,6 +34,7 @@ #include "clang/AST/LambdaCapture.h" #include "clang/AST/NestedNameSpecifier.h" #include "clang/AST/OperationKinds.h" +#include "clang/AST/ParentMapContext.h" #include "clang/AST/Stmt.h" #include "clang/AST/StmtCXX.h" #include "clang/AST/StmtObjC.h" @@ -58,8 +59,8 @@ #include "llvm/ADT/DenseMap.h" #include "llvm/ADT/None.h" #include "llvm/ADT/Optional.h" -#include "llvm/ADT/ScopeExit.h" #include "llvm/ADT/STLExtras.h" +#include "llvm/ADT/ScopeExit.h" #include "llvm/ADT/SmallVector.h" #include "llvm/Support/Casting.h" #include "llvm/Support/ErrorHandling.h" @@ -3219,9 +3220,12 @@ } // Returns true if the given D has a DeclContext up to the TranslationUnitDecl -// which is equal to the given DC. +// which is equal to the given DC, or D is equal to DC. static bool isAncestorDeclContextOf(const DeclContext *DC, const Decl *D) { - const DeclContext *DCi = D->getDeclContext(); + const DeclContext *DCi = dyn_cast(D); + if (!DCi) +DCi = D->getDeclContext(); + assert(DCi && "Declaration should have a context"); while (DCi != D->getTranslationUnitDecl()) { if (DCi == DC) return true; @@ -3230,9 +3234,36 @@ return false; } +// Returns true if the statement S has a parent declaration that has a +// DeclContext that is inside (or equal to) DC. In a specific use case if DC is +// a FunctionDecl, check if statement S resides in the body of the function. +static bool isAncestorDeclContextOf(const DeclContext *DC, const Stmt *S) { + ParentMapContext = DC->getParentASTContext().getParentMapContext(); + DynTypedNodeList Parents = ParentC.getParents(*S); + while (!Parents.empty()) { +if (const Decl *PD = Parents.begin()->get()) + return isAncestorDeclContextOf(DC, PD); +Parents = ParentC.getParents(*Parents.begin()); + } + return false; +} + static bool hasTypeDeclaredInsideFunction(QualType T, const FunctionDecl *FD) { if (T.isNull()) return false; + + auto CheckTemplateArgument = [FD](const TemplateArgument ) { +switch (Arg.getKind()) { +case TemplateArgument::Type: + return hasTypeDeclaredInsideFunction(Arg.getAsType(), FD); +case TemplateArgument::Expression: + return isAncestorDeclContextOf(FD, Arg.getAsExpr()); +default: + // FIXME: Handle other argument kinds. + return false; +} + }; + if (const auto *RecordT = T->getAs()) { const RecordDecl *RD = RecordT->getDecl(); assert(RD); @@ -3241,12 +3272,15 @@ return true; } if (const auto *RDTempl = dyn_cast(RD)) - return llvm::count_if(RDTempl->getTemplateArgs().asArray(), -[FD](const TemplateArgument ) { - return hasTypeDeclaredInsideFunction( - Arg.getAsType(), FD); -}); + if (llvm::count_if(RDTempl->getTemplateArgs().asArray(), + CheckTemplateArgument)) +return true; +// Note: It is possible that T can be get
[PATCH] D129640: [clang][ASTImporter] Improved handling of functions with auto return type.
martong accepted this revision. martong added a comment. This revision is now accepted and ready to land. LGTM Comment at: clang/lib/AST/ASTImporter.cpp:3237 + ParentMapContext = DC->getParentASTContext().getParentMapContext(); + DynTypedNodeList P = ParentC.getParents(*S); + while (!P.empty()) { martong wrote: > martong wrote: > > > The first call of `getParents` will create the parent map, via a full-blown > AST visitation. I am concerned a bit about the additional performance > overhead. Could you please run some measurements? (E.g. a CTU run on > `protobuf` and `bitcoin` with our internal CI infra) > The first call of `getParents` will create the parent map, via a full-blown > AST visitation. I am concerned a bit about the additional performance > overhead. Could you please run some measurements? (E.g. a CTU run on > `protobuf` and `bitcoin` with our internal CI infra) I've seen the measurement results, they look good. {F23872820} We have errors in the CTU analysis of 'qtbase' and 'contour' even with the base line. The duration for the remaining 3 (xerces, protobuf, bincoin) looks good. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129640/new/ https://reviews.llvm.org/D129640 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D129640: [clang][ASTImporter] Improved handling of functions with auto return type.
martong added inline comments. Comment at: clang/lib/AST/ASTImporter.cpp:3269-3273 +// Note: It is possible that T can be get as both a RecordType and a +// TemplateSpecializationType. + } + if (const auto *TST = T->getAs()) { +return llvm::count_if(TST->template_arguments(), CheckTemplateArgument); balazske wrote: > martong wrote: > > Is it possible that `T` is both a `RecordType` and a > > `TemplateSpecializationType` at the same time? From the [[ > > https://clang.llvm.org/doxygen/classclang_1_1TemplateSpecializationType.html > > | hierarchy ]] this seems impossible (?) > The type dump shows that a `RecordType` is contained within the > `TemplateSpecializationType` and it looks like that the get function handles > this case. When the `else` is added the check of `TemplateSpecializationType` > is skipped and the template argument expression `X` is not found (only > integer constant `1`). > ``` > TemplateSpecializationType 0x23fd8c0 'Tmpl' sugar Tmpl > |-TemplateArgument expr > | `-ConstantExpr 0x23fd798 'int' > | |-value: Int 1 > | `-ImplicitCastExpr 0x23fd780 'int' > | `-DeclRefExpr 0x23fd760 'const int' lvalue Var 0x23fd648 'X' 'const > int' non_odr_use_constant > `-RecordType 0x23fd8a0 'struct Tmpl<1>' > `-ClassTemplateSpecialization 0x23fd7b8 'Tmpl' > ``` Okay. I've dug deeper to understand how that is working. So, in the `TemplateSpecializationType` 'Tmpl' is actually a "sugar" to the `RecordType` 'struct Tmpl<1>'. And `getAs` will return with the desugared type: ``` // If this is a typedef for the type, strip the typedef off without // losing all typedef information. return cast(getUnqualifiedDesugaredType()); ``` Thus, to answer my own question, it is possible that T is both a RecordType and a TemplateSpecializationType at the same time! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129640/new/ https://reviews.llvm.org/D129640 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D129640: [clang][ASTImporter] Improved handling of functions with auto return type.
balazske added inline comments. Comment at: clang/lib/AST/ASTImporter.cpp:3269-3273 +// Note: It is possible that T can be get as both a RecordType and a +// TemplateSpecializationType. + } + if (const auto *TST = T->getAs()) { +return llvm::count_if(TST->template_arguments(), CheckTemplateArgument); martong wrote: > Is it possible that `T` is both a `RecordType` and a > `TemplateSpecializationType` at the same time? From the [[ > https://clang.llvm.org/doxygen/classclang_1_1TemplateSpecializationType.html > | hierarchy ]] this seems impossible (?) The type dump shows that a `RecordType` is contained within the `TemplateSpecializationType` and it looks like that the get function handles this case. When the `else` is added the check of `TemplateSpecializationType` is skipped and the template argument expression `X` is not found (only integer constant `1`). ``` TemplateSpecializationType 0x23fd8c0 'Tmpl' sugar Tmpl |-TemplateArgument expr | `-ConstantExpr 0x23fd798 'int' | |-value: Int 1 | `-ImplicitCastExpr 0x23fd780 'int' | `-DeclRefExpr 0x23fd760 'const int' lvalue Var 0x23fd648 'X' 'const int' non_odr_use_constant `-RecordType 0x23fd8a0 'struct Tmpl<1>' `-ClassTemplateSpecialization 0x23fd7b8 'Tmpl' ``` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129640/new/ https://reviews.llvm.org/D129640 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D129640: [clang][ASTImporter] Improved handling of functions with auto return type.
balazske updated this revision to Diff 446057. balazske marked 3 inline comments as done. balazske added a comment. Small code NFC improvements. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129640/new/ https://reviews.llvm.org/D129640 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 @@ -6320,6 +6320,24 @@ struct ImportAutoFunctions : ASTImporterOptionSpecificTestBase {}; +TEST_P(ImportAutoFunctions, ReturnWithTemplateWithIntegerArgDeclaredInside) { + Decl *FromTU = getTuDecl( + R"( + template struct Tmpl {}; + auto foo() { +constexpr int X = 1; +return Tmpl(); + } + )", + Lang_CXX14, "input0.cc"); + FunctionDecl *From = FirstDeclMatcher().match( + FromTU, functionDecl(hasName("foo"))); + + FunctionDecl *To = Import(From, Lang_CXX14); + EXPECT_TRUE(To); + EXPECT_TRUE(isa(To->getReturnType())); +} + TEST_P(ImportAutoFunctions, ReturnWithTemplateWithStructDeclaredInside1) { Decl *FromTU = getTuDecl( R"( Index: clang/lib/AST/ASTImporter.cpp === --- clang/lib/AST/ASTImporter.cpp +++ clang/lib/AST/ASTImporter.cpp @@ -12,9 +12,9 @@ //===--===// #include "clang/AST/ASTImporter.h" -#include "clang/AST/ASTImporterSharedState.h" #include "clang/AST/ASTContext.h" #include "clang/AST/ASTDiagnostic.h" +#include "clang/AST/ASTImporterSharedState.h" #include "clang/AST/ASTStructuralEquivalence.h" #include "clang/AST/Attr.h" #include "clang/AST/Decl.h" @@ -34,6 +34,7 @@ #include "clang/AST/LambdaCapture.h" #include "clang/AST/NestedNameSpecifier.h" #include "clang/AST/OperationKinds.h" +#include "clang/AST/ParentMapContext.h" #include "clang/AST/Stmt.h" #include "clang/AST/StmtCXX.h" #include "clang/AST/StmtObjC.h" @@ -58,8 +59,8 @@ #include "llvm/ADT/DenseMap.h" #include "llvm/ADT/None.h" #include "llvm/ADT/Optional.h" -#include "llvm/ADT/ScopeExit.h" #include "llvm/ADT/STLExtras.h" +#include "llvm/ADT/ScopeExit.h" #include "llvm/ADT/SmallVector.h" #include "llvm/Support/Casting.h" #include "llvm/Support/ErrorHandling.h" @@ -3214,9 +3215,12 @@ } // Returns true if the given D has a DeclContext up to the TranslationUnitDecl -// which is equal to the given DC. +// which is equal to the given DC, or D is equal to DC. static bool isAncestorDeclContextOf(const DeclContext *DC, const Decl *D) { - const DeclContext *DCi = D->getDeclContext(); + const DeclContext *DCi = dyn_cast(D); + if (!DCi) +DCi = D->getDeclContext(); + assert(DCi && "Declaration should have a context"); while (DCi != D->getTranslationUnitDecl()) { if (DCi == DC) return true; @@ -3225,9 +3229,36 @@ return false; } +// Returns true if the statement S has a parent declaration that has a +// DeclContext that is inside (or equal to) DC. In a specific use case if DC is +// a FunctionDecl, check if statement S resides in the body of the function. +static bool isAncestorDeclContextOf(const DeclContext *DC, const Stmt *S) { + ParentMapContext = DC->getParentASTContext().getParentMapContext(); + DynTypedNodeList Parents = ParentC.getParents(*S); + while (!Parents.empty()) { +if (const Decl *PD = Parents.begin()->get()) + return isAncestorDeclContextOf(DC, PD); +Parents = ParentC.getParents(*Parents.begin()); + } + return false; +} + static bool hasTypeDeclaredInsideFunction(QualType T, const FunctionDecl *FD) { if (T.isNull()) return false; + + auto CheckTemplateArgument = [FD](const TemplateArgument ) { +switch (Arg.getKind()) { +case TemplateArgument::Type: + return hasTypeDeclaredInsideFunction(Arg.getAsType(), FD); +case TemplateArgument::Expression: + return isAncestorDeclContextOf(FD, Arg.getAsExpr()); +default: + // FIXME: Handle other argument kinds. + return false; +} + }; + if (const auto *RecordT = T->getAs()) { const RecordDecl *RD = RecordT->getDecl(); assert(RD); @@ -3236,12 +3267,15 @@ return true; } if (const auto *RDTempl = dyn_cast(RD)) - return llvm::count_if(RDTempl->getTemplateArgs().asArray(), -[FD](const TemplateArgument ) { - return hasTypeDeclaredInsideFunction( - Arg.getAsType(), FD); -}); + if (llvm::count_if(RDTempl->getTemplateArgs().asArray(), + CheckTemplateArgument)) +return true; +// Note: It is possible that T can be get as both a RecordType and a +// TemplateSpecializationType. } + if (const auto *TST = T->getAs()) +return
[PATCH] D129640: [clang][ASTImporter] Improved handling of functions with auto return type.
martong added a comment. Thanks, nice work! Comment at: clang/lib/AST/ASTImporter.cpp:3237 + ParentMapContext = DC->getParentASTContext().getParentMapContext(); + DynTypedNodeList P = ParentC.getParents(*S); + while (!P.empty()) { Comment at: clang/lib/AST/ASTImporter.cpp:3237 + ParentMapContext = DC->getParentASTContext().getParentMapContext(); + DynTypedNodeList P = ParentC.getParents(*S); + while (!P.empty()) { martong wrote: > The first call of `getParents` will create the parent map, via a full-blown AST visitation. I am concerned a bit about the additional performance overhead. Could you please run some measurements? (E.g. a CTU run on `protobuf` and `bitcoin` with our internal CI infra) Comment at: clang/lib/AST/ASTImporter.cpp:3251-3254 +if (Arg.getKind() == TemplateArgument::Type) + return hasTypeDeclaredInsideFunction(Arg.getAsType(), FD); +if (Arg.getKind() == TemplateArgument::Expression) + return isAncestorDeclContextOf(FD, Arg.getAsExpr()); Should this be handled in a `switch` rather, perhaps with an `llvm_unreachable` at the `default` case? Just to make sure that no "kind" is left out. Comment at: clang/lib/AST/ASTImporter.cpp:3269-3273 +// Note: It is possible that T can be get as both a RecordType and a +// TemplateSpecializationType. + } + if (const auto *TST = T->getAs()) { +return llvm::count_if(TST->template_arguments(), CheckTemplateArgument); Is it possible that `T` is both a `RecordType` and a `TemplateSpecializationType` at the same time? From the [[ https://clang.llvm.org/doxygen/classclang_1_1TemplateSpecializationType.html | hierarchy ]] this seems impossible (?) Comment at: clang/unittests/AST/ASTImporterTest.cpp:6323 +TEST_P(ImportAutoFunctions, ReturnWithTemplateWithIntegerArgDeclaredInside) { + Decl *FromTU = getTuDecl( Nice test! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129640/new/ https://reviews.llvm.org/D129640 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D129640: [clang][ASTImporter] Improved handling of functions with auto return type.
balazske created this revision. Herald added subscribers: steakhal, martong, gamesh411, Szelethus, dkrupp. Herald added a reviewer: a.sidorin. Herald added a reviewer: shafik. Herald added a project: All. balazske requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Avoid a crash if a function is imported that has auto return type that references to a template with an expression-type of argument that references into the function's body. Fixes issue #56047 Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D129640 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 @@ -6320,6 +6320,24 @@ struct ImportAutoFunctions : ASTImporterOptionSpecificTestBase {}; +TEST_P(ImportAutoFunctions, ReturnWithTemplateWithIntegerArgDeclaredInside) { + Decl *FromTU = getTuDecl( + R"( + template struct Tmpl {}; + auto foo() { +constexpr int X = 1; +return Tmpl(); + } + )", + Lang_CXX14, "input0.cc"); + FunctionDecl *From = FirstDeclMatcher().match( + FromTU, functionDecl(hasName("foo"))); + + FunctionDecl *To = Import(From, Lang_CXX14); + EXPECT_TRUE(To); + EXPECT_TRUE(isa(To->getReturnType())); +} + TEST_P(ImportAutoFunctions, ReturnWithTemplateWithStructDeclaredInside1) { Decl *FromTU = getTuDecl( R"( Index: clang/lib/AST/ASTImporter.cpp === --- clang/lib/AST/ASTImporter.cpp +++ clang/lib/AST/ASTImporter.cpp @@ -12,9 +12,9 @@ //===--===// #include "clang/AST/ASTImporter.h" -#include "clang/AST/ASTImporterSharedState.h" #include "clang/AST/ASTContext.h" #include "clang/AST/ASTDiagnostic.h" +#include "clang/AST/ASTImporterSharedState.h" #include "clang/AST/ASTStructuralEquivalence.h" #include "clang/AST/Attr.h" #include "clang/AST/Decl.h" @@ -34,6 +34,7 @@ #include "clang/AST/LambdaCapture.h" #include "clang/AST/NestedNameSpecifier.h" #include "clang/AST/OperationKinds.h" +#include "clang/AST/ParentMapContext.h" #include "clang/AST/Stmt.h" #include "clang/AST/StmtCXX.h" #include "clang/AST/StmtObjC.h" @@ -58,8 +59,8 @@ #include "llvm/ADT/DenseMap.h" #include "llvm/ADT/None.h" #include "llvm/ADT/Optional.h" -#include "llvm/ADT/ScopeExit.h" #include "llvm/ADT/STLExtras.h" +#include "llvm/ADT/ScopeExit.h" #include "llvm/ADT/SmallVector.h" #include "llvm/Support/Casting.h" #include "llvm/Support/ErrorHandling.h" @@ -3214,9 +3215,12 @@ } // Returns true if the given D has a DeclContext up to the TranslationUnitDecl -// which is equal to the given DC. +// which is equal to the given DC, or D is equal to DC. static bool isAncestorDeclContextOf(const DeclContext *DC, const Decl *D) { - const DeclContext *DCi = D->getDeclContext(); + const DeclContext *DCi = dyn_cast(D); + if (!DCi) +DCi = D->getDeclContext(); + assert(DCi && "Declaration should have a context"); while (DCi != D->getTranslationUnitDecl()) { if (DCi == DC) return true; @@ -3225,9 +3229,32 @@ return false; } +// Returns true if the statement S has a parent declaration that has a +// DeclContext that is inside (or equal to) DC. In a specific use case if DC is +// a FunctionDecl, check if statement S resides in the body of the function. +static bool isAncestorDeclContextOf(const DeclContext *DC, const Stmt *S) { + ParentMapContext = DC->getParentASTContext().getParentMapContext(); + DynTypedNodeList P = ParentC.getParents(*S); + while (!P.empty()) { +if (const Decl *PD = P.begin()->get()) + return isAncestorDeclContextOf(DC, PD); +P = ParentC.getParents(*P.begin()); + } + return false; +} + static bool hasTypeDeclaredInsideFunction(QualType T, const FunctionDecl *FD) { if (T.isNull()) return false; + + auto CheckTemplateArgument = [FD](const TemplateArgument ) { +if (Arg.getKind() == TemplateArgument::Type) + return hasTypeDeclaredInsideFunction(Arg.getAsType(), FD); +if (Arg.getKind() == TemplateArgument::Expression) + return isAncestorDeclContextOf(FD, Arg.getAsExpr()); +return false; + }; + if (const auto *RecordT = T->getAs()) { const RecordDecl *RD = RecordT->getDecl(); assert(RD); @@ -3236,11 +3263,14 @@ return true; } if (const auto *RDTempl = dyn_cast(RD)) - return llvm::count_if(RDTempl->getTemplateArgs().asArray(), -[FD](const TemplateArgument ) { - return hasTypeDeclaredInsideFunction( - Arg.getAsType(), FD); -}); + if (llvm::count_if(RDTempl->getTemplateArgs().asArray(), +