From 6c4c17f82fd5e12d42a4c9358127e07ce9f026fb Mon Sep 17 00:00:00 2001 From: Jun Zhang Date: Sat, 24 Sep 2022 12:02:06 +0800 Subject: [PATCH 1/5] [skip-ci][cxxmodules][cling] Avoid loading some unnecessary modules When we run into an unkown identifier that is a namespace, we don't really need to load its corresponding modules. Instead, we create a new module that forward declared all namespaces and always load it first. By doing so, we can avoid loading a lot of unnecessary modules. Signed-off-by: Jun Zhang --- build/unix/module.modulemap | 5 +++ core/metacling/src/TCling.cxx | 83 ++++++++++++++++++++++++++++++++--- 2 files changed, 81 insertions(+), 7 deletions(-) diff --git a/build/unix/module.modulemap b/build/unix/module.modulemap index da55cc3b54ed9..81b732e432bdb 100644 --- a/build/unix/module.modulemap +++ b/build/unix/module.modulemap @@ -8,6 +8,11 @@ module ROOT_Foundation_C { export * } +module ROOT_FwdNamespaces { + module "AutoGeneratedForwardDeclaredNamespacesForGMI" { header "AutoGeneratedForwardDeclaredNamespacesForGMI.h" export *} + export * +} + // This module contains header files from module Core which are used as // configuration for ROOT. They contain a lot of macro definitions which are // supposed to be textually expanded in each TU. diff --git a/core/metacling/src/TCling.cxx b/core/metacling/src/TCling.cxx index 5899e789b2021..8d0a0c635b250 100644 --- a/core/metacling/src/TCling.cxx +++ b/core/metacling/src/TCling.cxx @@ -121,6 +121,7 @@ clang/LLVM technology. #include "llvm/Support/DynamicLibrary.h" #include "llvm/Support/raw_ostream.h" #include "llvm/Support/Path.h" +#include "llvm/Support/Program.h" #include "llvm/Support/Process.h" #include "llvm/Object/ELFObjectFile.h" #include "llvm/Object/ObjectFile.h" @@ -1084,6 +1085,8 @@ static bool HasASTFileOnDisk(clang::Module *M, const clang::Preprocessor &PP, st } static bool HaveFullGlobalModuleIndex = false; +static const std::string GeneratedFwdNamespaces = (TROOT::GetIncludeDir() + "AutoGeneratedForwardDeclaredNamespacesForGMI.h").Data(); + static GlobalModuleIndex *loadGlobalModuleIndex(cling::Interpreter &interp) { CompilerInstance &CI = *interp.getCI(); @@ -1124,6 +1127,8 @@ static GlobalModuleIndex *loadGlobalModuleIndex(cling::Interpreter &interp) TraverseDecl(TU); } bool VisitNamedDecl(NamedDecl *ND) { + if (ND->isInStdNamespace()) + return true; if (!ND->isFromASTFile()) return true; if (!ND->getIdentifier()) @@ -1135,17 +1140,22 @@ static GlobalModuleIndex *loadGlobalModuleIndex(cling::Interpreter &interp) if (TagDecl *TD = llvm::dyn_cast(ND)) { if (TD->isCompleteDefinition()) Register(TD); - } else if (NamespaceDecl *NSD = llvm::dyn_cast(ND)) { - Register(NSD, /*AddSingleEntry=*/ false); - } - else if (TypedefNameDecl *TND = dyn_cast(ND)) + } else if (TypedefNameDecl *TND = dyn_cast(ND)) Register(TND); + else if (TypeAliasDecl *TAD = dyn_cast(ND)) + Register(TAD); + else if (isa(ND) && !isa(ND)) + Register(ND); // FIXME: Add the rest... return true; // continue decending } + private: clang::GlobalModuleIndex::UserDefinedInterestingIDs &DefinitionIDs; - void Register(const NamedDecl* ND, bool AddSingleEntry = true) { + llvm::StringMap> Canonicals; + + void Register(const NamedDecl *ND) + { assert(ND->isFromASTFile()); // FIXME: All decls should have an owning module once rootcling // updates its generated decls from within the LookupHelper & co. @@ -1163,17 +1173,74 @@ static GlobalModuleIndex *loadGlobalModuleIndex(cling::Interpreter &interp) Module *OwningModule = ND->getOwningModule()->getTopLevelModule(); assert(OwningModule); assert(!ND->getName().empty() && "Empty name"); - if (AddSingleEntry && DefinitionIDs.count(ND->getName())) - return; + if (Canonicals.count(ND->getName())) { + if (llvm::is_contained(Canonicals[ND->getName()], ND->getCanonicalDecl())) + return; + } + // FIXME: The FileEntry in not stable to serialize. // FIXME: We might end up with many times with the same module. // FIXME: We might end up two modules containing a definition. // FIXME: What do we do if no definition is found. DefinitionIDs[ND->getName()].push_back(OwningModule->getASTFile()); + Canonicals[ND->getName()].push_back(ND->getCanonicalDecl()); } }; DefinitionFinder defFinder(IDs, CI.getASTContext().getTranslationUnitDecl()); + struct NamespaceDeclarer : public RecursiveASTVisitor { + std::string Fwds; + std::unordered_set Namespaces; + + NamespaceDeclarer(clang::TranslationUnitDecl *TU) { TraverseDecl(TU); } + std::string PrintNamespaceOpen(NamespaceDecl *NSD) + { + std::string result; + if (NSD->isInline()) + result += "inline "; + result += "namespace " + NSD->getNameAsString() + "{"; + return result; + } + bool VisitNamespaceDecl(NamespaceDecl *NSD) + { + if (NSD->isAnonymousNamespace() || NSD->isInline() || NSD->isInStdNamespace()) + return true; + if (Namespaces.count(NSD->getCanonicalDecl())) + return true; + + std::vector Parents; + // NS1::NS2::NS3-> namespace NS1 { namespace NS2 {namespace NS3{}}} + for (DeclContext *DC = NSD; DC && !DC->isTranslationUnit(); DC = DC->getParent()) { + if (auto D = dyn_cast(DC)) { + if (D->isAnonymousNamespace() || D->isInline() || D->isInStdNamespace()) + continue; + Namespaces.insert(D); + Parents.push_back(D); + } + } + std::reverse(Parents.begin(), Parents.end()); + + for (auto *D : Parents) + Fwds += PrintNamespaceOpen(D); + Fwds += std::string(Parents.size(), '}'); + Fwds += "\n"; + + Namespaces.insert(NSD->getCanonicalDecl()); + for (Decl *D : NSD->decls()) { + if (!isa(D)) + continue; + TraverseDecl(D); + } + return true; + } + }; + + if (!llvm::sys::fs::exists(GeneratedFwdNamespaces)) { + NamespaceDeclarer NSDeclarer(CI.getASTContext().getTranslationUnitDecl()); + + llvm::sys::writeFileWithEncoding(GeneratedFwdNamespaces, NSDeclarer.Fwds); + } + llvm::cantFail(GlobalModuleIndex::writeIndex(CI.getFileManager(), CI.getPCHContainerReader(), ModuleIndexPath, @@ -1196,6 +1263,8 @@ static void RegisterCxxModules(cling::Interpreter &clingInterp) cling::Interpreter::PushTransactionRAII deserRAII(&clingInterp); // Setup core C++ modules if we have any to setup. + if (llvm::sys::fs::exists(GeneratedFwdNamespaces)) + LoadModule("ROOT_FwdNamespaces", clingInterp); // Load libc and stl first. // Load vcruntime module for windows From 677d6d30c7bfc5dc1d405a82c2857857a1546de6 Mon Sep 17 00:00:00 2001 From: Jun Zhang Date: Wed, 5 Oct 2022 17:11:42 +0800 Subject: [PATCH 2/5] Don't find external visible decls if we're instantiating templates Signed-off-by: Jun Zhang --- .../lib/Interpreter/InterpreterCallbacks.cpp | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/interpreter/cling/lib/Interpreter/InterpreterCallbacks.cpp b/interpreter/cling/lib/Interpreter/InterpreterCallbacks.cpp index 4755009e77df2..4f36f0a0b60e9 100644 --- a/interpreter/cling/lib/Interpreter/InterpreterCallbacks.cpp +++ b/interpreter/cling/lib/Interpreter/InterpreterCallbacks.cpp @@ -215,6 +215,15 @@ namespace cling { } }; + static bool IsInstantiatingTemplates(clang::Sema* S) { + if (!S) + return false; + + const auto& CodeSynthesisContexts = S->CodeSynthesisContexts; + return !CodeSynthesisContexts.empty() && + CodeSynthesisContexts.back().isInstantiationRecord(); + } + ///\brief Translates 'interesting' for the interpreter ExternalSemaSource /// events into interpreter callbacks. /// @@ -274,10 +283,11 @@ namespace cling { bool FindExternalVisibleDeclsByName(const clang::DeclContext* DC, clang::DeclarationName Name) override { - if (m_Callbacks) - return m_Callbacks->LookupObject(DC, Name); - - return false; + if (!m_Callbacks) + return false; + if (IsInstantiatingTemplates(m_Sema)) + return false; + return m_Callbacks->LookupObject(DC, Name); } // Silence warning virtual function was hidden. From 707b699f58d07ea5ea5d1db2d2c8b6a2afbe908d Mon Sep 17 00:00:00 2001 From: Jun Zhang Date: Sun, 9 Oct 2022 22:18:24 +0800 Subject: [PATCH 3/5] Push transaction in ParseProto::Parse Signed-off-by: Jun Zhang --- interpreter/cling/lib/Interpreter/LookupHelper.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/interpreter/cling/lib/Interpreter/LookupHelper.cpp b/interpreter/cling/lib/Interpreter/LookupHelper.cpp index 23e6c086aab45..2555fe23bff6a 100644 --- a/interpreter/cling/lib/Interpreter/LookupHelper.cpp +++ b/interpreter/cling/lib/Interpreter/LookupHelper.cpp @@ -1669,6 +1669,7 @@ namespace cling { // Parse the prototype now. // + Interpreter::PushTransactionRAII clingRAII(Interp); StartParsingRAII ParseStarted(LH, funcProto, llvm::StringRef("func.prototype.file"), diagOnOff); From ee62ca884541aad8d14d397f41d8f7f3d38deec0 Mon Sep 17 00:00:00 2001 From: Jun Zhang Date: Sun, 23 Oct 2022 14:13:27 +0800 Subject: [PATCH 4/5] Revert workrounds since Jonas's patch (#11514) should fix everything --- .../lib/Interpreter/InterpreterCallbacks.cpp | 18 ++++-------------- .../cling/lib/Interpreter/LookupHelper.cpp | 1 - 2 files changed, 4 insertions(+), 15 deletions(-) diff --git a/interpreter/cling/lib/Interpreter/InterpreterCallbacks.cpp b/interpreter/cling/lib/Interpreter/InterpreterCallbacks.cpp index 4f36f0a0b60e9..4755009e77df2 100644 --- a/interpreter/cling/lib/Interpreter/InterpreterCallbacks.cpp +++ b/interpreter/cling/lib/Interpreter/InterpreterCallbacks.cpp @@ -215,15 +215,6 @@ namespace cling { } }; - static bool IsInstantiatingTemplates(clang::Sema* S) { - if (!S) - return false; - - const auto& CodeSynthesisContexts = S->CodeSynthesisContexts; - return !CodeSynthesisContexts.empty() && - CodeSynthesisContexts.back().isInstantiationRecord(); - } - ///\brief Translates 'interesting' for the interpreter ExternalSemaSource /// events into interpreter callbacks. /// @@ -283,11 +274,10 @@ namespace cling { bool FindExternalVisibleDeclsByName(const clang::DeclContext* DC, clang::DeclarationName Name) override { - if (!m_Callbacks) - return false; - if (IsInstantiatingTemplates(m_Sema)) - return false; - return m_Callbacks->LookupObject(DC, Name); + if (m_Callbacks) + return m_Callbacks->LookupObject(DC, Name); + + return false; } // Silence warning virtual function was hidden. diff --git a/interpreter/cling/lib/Interpreter/LookupHelper.cpp b/interpreter/cling/lib/Interpreter/LookupHelper.cpp index 2555fe23bff6a..23e6c086aab45 100644 --- a/interpreter/cling/lib/Interpreter/LookupHelper.cpp +++ b/interpreter/cling/lib/Interpreter/LookupHelper.cpp @@ -1669,7 +1669,6 @@ namespace cling { // Parse the prototype now. // - Interpreter::PushTransactionRAII clingRAII(Interp); StartParsingRAII ParseStarted(LH, funcProto, llvm::StringRef("func.prototype.file"), diagOnOff); From 268296f2848064f9c721bd89f3739231909182c5 Mon Sep 17 00:00:00 2001 From: Jun Zhang Date: Sun, 23 Oct 2022 15:33:39 +0800 Subject: [PATCH 5/5] Always preload ROOTDataFrame module Signed-off-by: Jun Zhang --- core/metacling/src/TCling.cxx | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/core/metacling/src/TCling.cxx b/core/metacling/src/TCling.cxx index 8d0a0c635b250..84c909cc2db9e 100644 --- a/core/metacling/src/TCling.cxx +++ b/core/metacling/src/TCling.cxx @@ -1302,7 +1302,9 @@ static void RegisterCxxModules(cling::Interpreter &clingInterp) // These modules should not be preloaded but they fix issues. // FIXME: Hist is not a core module but is very entangled to MathCore and // causes issues. - std::vector FIXMEModules = {"Hist"}; + // ROOTDataFrame is not a core module as well, but without it Clang reports + // some ODR related issues. + std::vector FIXMEModules = {"Hist", "ROOTDataFrame"}; clang::CompilerInstance &CI = *clingInterp.getCI(); clang::Preprocessor &PP = CI.getPreprocessor(); ModuleMap &MMap = PP.getHeaderSearchInfo().getModuleMap();