From 58104d0da8d54ddcf70b9039bcca1c716d701dcd Mon Sep 17 00:00:00 2001 From: Javier Lopez-Gomez Date: Tue, 29 Aug 2023 16:43:05 +0200 Subject: [PATCH 1/3] [cling] DeclUnloader: remove StaticVarCollector StaticVarCollector recursively visited descendants of a `FunctionDecl` node to collect static local variables. However, these always appear in the enclosing DeclContext. --- .../cling/lib/Interpreter/DeclUnloader.cpp | 31 +++++-------------- 1 file changed, 8 insertions(+), 23 deletions(-) diff --git a/interpreter/cling/lib/Interpreter/DeclUnloader.cpp b/interpreter/cling/lib/Interpreter/DeclUnloader.cpp index 25c01b1ef3c0b..6f7fe4746aba1 100644 --- a/interpreter/cling/lib/Interpreter/DeclUnloader.cpp +++ b/interpreter/cling/lib/Interpreter/DeclUnloader.cpp @@ -327,24 +327,6 @@ namespace { } } - typedef llvm::SmallVector Vars; - class StaticVarCollector : public RecursiveASTVisitor { - Vars& m_V; - - public: - StaticVarCollector(FunctionDecl* FD, Vars& V) : m_V(V) { - TraverseStmt(FD->getBody()); - } - bool VisitDeclStmt(DeclStmt* DS) { - for (DeclStmt::decl_iterator I = DS->decl_begin(), E = DS->decl_end(); - I != E; ++I) - if (VarDecl* VD = dyn_cast(*I)) - if (VD->isStaticLocal()) - m_V.push_back(VD); - return true; - } - }; - // Template instantiation of templated function first creates a canonical // declaration and after the actual template specialization. For example: // template T TemplatedF(T t); @@ -677,15 +659,18 @@ namespace cling { // which we will remove soon. (Eg. mangleDeclName iterates the redecls) GlobalDecl GD(FD); MaybeRemoveDeclFromModule(GD); + // Handle static locals. void func() { static int var; } is represented in // the llvm::Module is a global named @func.var - Vars V; - StaticVarCollector c(FD, V); - for (Vars::iterator I = V.begin(), E = V.end(); I != E; ++I) { - GlobalDecl GD(*I); - MaybeRemoveDeclFromModule(GD); + for (auto D : FunctionDecl::castToDeclContext(FD)->noload_decls()) { + if (auto VD = dyn_cast(D)) + if (VD->isStaticLocal()) { + GlobalDecl GD(VD); + MaybeRemoveDeclFromModule(GD); + } } } + // VisitRedeclarable() will mess around with this! bool wasCanonical = FD->isCanonicalDecl(); // FunctionDecl : DeclaratiorDecl, DeclContext, Redeclarable From fe6c0d94751b23477238e07f70ac9a37f9621a44 Mon Sep 17 00:00:00 2001 From: Javier Lopez-Gomez Date: Tue, 29 Aug 2023 16:43:05 +0200 Subject: [PATCH 2/3] [cling] DeclUnloader: do not delete instantiated member functions The body of member functions of a templated class only gets instantiated when the function is used. These `CXXMethodDecl` should not be deleted from the AST; instead return them to the 'instantiation pending' state. --- .../cling/lib/Interpreter/DeclUnloader.cpp | 30 +++++++++++++++++++ .../cling/test/CodeUnloading/Classes.C | 20 ++++++++++++- 2 files changed, 49 insertions(+), 1 deletion(-) diff --git a/interpreter/cling/lib/Interpreter/DeclUnloader.cpp b/interpreter/cling/lib/Interpreter/DeclUnloader.cpp index 6f7fe4746aba1..21cdd678de2ac 100644 --- a/interpreter/cling/lib/Interpreter/DeclUnloader.cpp +++ b/interpreter/cling/lib/Interpreter/DeclUnloader.cpp @@ -678,6 +678,36 @@ namespace cling { // DeclContext and when trying to remove them we need the full redecl chain // still in place. bool Successful = VisitDeclContext(FD); + // The body of member functions of a templated class only gets instantiated + // when the function is used, i.e. + // `-ClassTemplateDecl + // |-TemplateTypeParmDecl referenced typename depth 0 index 0 T + // |-CXXRecordDecl struct Foo definition + // | |-DefinitionData + // | `-CXXMethodDecl f 'T (T)' + // | |-ParmVarDecl 0x55e5787cac70 referenced x 'T' + // | `-CompoundStmt + // | `-ReturnStmt + // | `-DeclRefExpr 'T' lvalue ParmVar 0x55e5787cac70 'x' 'T' + // `-ClassTemplateSpecializationDecl struct Foo definition + // |-DefinitionData + // |-TemplateArgument type 'int' + // | `-BuiltinType 'int' + // |-CXXMethodDecl f 'int (int)' <<<< Instantiation pending + // | `-ParmVarDecl x 'int':'int' + // |-CXXConstructorDecl implicit used constexpr Foo 'void () noexcept' + // inline default trivial + // + // Such functions should not be deleted from the AST, but returned to the + // 'pending instantiation' state. + if (auto MSI = FD->getMemberSpecializationInfo()) { + MSI->setPointOfInstantiation(SourceLocation()); + MSI->setTemplateSpecializationKind( + TemplateSpecializationKind::TSK_ImplicitInstantiation); + FD->setBody(nullptr); + FD->setInstantiationIsPending(true); + return Successful; + } Successful &= VisitRedeclarable(FD, FD->getDeclContext()); Successful &= VisitDeclaratorDecl(FD); diff --git a/interpreter/cling/test/CodeUnloading/Classes.C b/interpreter/cling/test/CodeUnloading/Classes.C index 76afb20f37602..fc30502658b92 100644 --- a/interpreter/cling/test/CodeUnloading/Classes.C +++ b/interpreter/cling/test/CodeUnloading/Classes.C @@ -8,6 +8,9 @@ // RUN: cat %s | %cling 2>&1 | FileCheck %s +#include +#include + extern "C" int printf(const char* fmt, ...); .storeState "preUnload" class MyClass{ @@ -22,5 +25,20 @@ public: .compareState "preUnload" //CHECK-NOT: Differences float MyClass = 1.1 -//CHECK: (float) 1.1 +//CHECK: (float) 1.10000f + +template +struct MyStruct { T f(T x) { return x; } }; +MyStruct obj; +obj.f(42.0) +//CHECK: (float) 42.0000f +.undo +obj.f(42.0) +//CHECK: (float) 42.0000f + +auto p = std::make_unique("string"); +(unsigned long)p.size() // expected-error{{no member named 'size' in 'std::unique_ptr>'; did you mean to use '->' instead of '.'?}} +(unsigned long)p->size() +//CHECK: (unsigned long) 6 + .q From c224025e3a4072c6be9f6d1f9d617e5ad797c766 Mon Sep 17 00:00:00 2001 From: Jonas Hahnfeld Date: Fri, 24 Nov 2023 14:39:34 +0100 Subject: [PATCH 3/3] [cling] DeclUnloader: Remove extra check isInstantiatedInPCH This effectively reverts commit 74472caaa9 ("[cling] Fixes issue in DeclUnloader: do not unload templates intantiated in the PCH"), it's not needed anymore, all tests pass and the snippet in the summary of https://github.com/root-project/root/pull/4447 works, but it filters too many declarations from the unloader. --- .../cling/lib/Interpreter/DeclUnloader.cpp | 15 --------------- interpreter/cling/lib/Interpreter/DeclUnloader.h | 4 +--- 2 files changed, 1 insertion(+), 18 deletions(-) diff --git a/interpreter/cling/lib/Interpreter/DeclUnloader.cpp b/interpreter/cling/lib/Interpreter/DeclUnloader.cpp index 21cdd678de2ac..85874db9a5f42 100644 --- a/interpreter/cling/lib/Interpreter/DeclUnloader.cpp +++ b/interpreter/cling/lib/Interpreter/DeclUnloader.cpp @@ -473,21 +473,6 @@ namespace { namespace cling { using namespace clang; - ///\brief Return whether `D' is a template that was first instantiated non- - /// locally, i.e. in a PCH/module. If `D' is not an instantiation, return - /// false. - bool DeclUnloader::isInstantiatedInPCH(const Decl* D) { - SourceManager& SM = D->getASTContext().getSourceManager(); - if (const auto FD = dyn_cast(D)) - return FD->isTemplateInstantiation() && - !SM.isLocalSourceLocation(FD->getPointOfInstantiation()); - else if (const auto CTSD = dyn_cast(D)) - return !SM.isLocalSourceLocation(CTSD->getPointOfInstantiation()); - else if (const auto VTSD = dyn_cast(D)) - return !SM.isLocalSourceLocation(VTSD->getPointOfInstantiation()); - return false; - } - void DeclUnloader::resetDefinitionData(TagDecl* decl) { auto canon = dyn_cast(decl->getCanonicalDecl()); assert(canon && "Only CXXRecordDecl have DefinitionData"); diff --git a/interpreter/cling/lib/Interpreter/DeclUnloader.h b/interpreter/cling/lib/Interpreter/DeclUnloader.h index 500fa7f6d1e4e..b5829155a96d6 100644 --- a/interpreter/cling/lib/Interpreter/DeclUnloader.h +++ b/interpreter/cling/lib/Interpreter/DeclUnloader.h @@ -60,7 +60,7 @@ namespace cling { ///\returns true on success. /// bool UnloadDecl(clang::Decl* D) { - if (D->isFromASTFile() || isInstantiatedInPCH(D)) + if (D->isFromASTFile()) return true; return Visit(D); } @@ -270,8 +270,6 @@ namespace cling { /// void CollectFilesToUncache(clang::SourceLocation Loc); - bool isInstantiatedInPCH(const clang::Decl *D); - template bool VisitRedeclarable(clang::Redeclarable* R, clang::DeclContext* DC); };