Skip to content

[cxxmodules] Add initial implementation of the semantic GMI. #5094

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
vgvassilev merged 4 commits into from
Jun 17, 2020
Merged

[cxxmodules] Add initial implementation of the semantic GMI. #5094

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cfbaeae
Add initial implementation of the semantic GMI.
vgvassilev Mar 6, 2020
e4e82e6
Add Rint to the core modules.
vgvassilev Jun 4, 2020
11bbc24
[gmi] TCling::Declare is not allowed to autoload missing libraries.
vgvassilev Jun 8, 2020
6c26e5f
LoadPCMImpl may need to deserialize.
vgvassilev Jun 14, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions bindings/pyroot/pythonizations/test/numbadeclare.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,6 +312,11 @@ class NumbaDeclareArray(unittest.TestCase):

test_inputs = [default_test_inputs]

# The global module index does not have RVec entities preloaded and
# gInterpreter.Declare is not allowed to load libROOTVecOps for RVec.
# Preload the library now.
ROOT.gSystem.Load("libROOTVecOps")

@unittest.skipIf(skip, skip_reason)
def test_wrapper_in_vecf(self):
"""
Expand Down
5 changes: 5 additions & 0 deletions bindings/pyroot/pythonizations/test/rdataframe_asnumpy.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,11 @@ def test_read_tlorentzvector(self):
"""
Testing reading a TLorentzVector
"""

# The global module index does not have it preloaded and
# gInterpreter.Declare is not allowed to load libPhysics for
# TLorentzVector. Preload the library now.
ROOT.gSystem.Load("libPhysics")
ROOT.gInterpreter.Declare("""
TLorentzVector create_tlorentzvector() {
auto v = TLorentzVector();
Expand Down
4 changes: 4 additions & 0 deletions bindings/pyroot_legacy/test/rdataframe_asnumpy.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,10 @@ def test_read_vector_variablesize(self):
self.assertIn("ROOT.vector<unsigned int>", str(type(npy["x"][0])))

def test_read_tlorentzvector(self):
# The global module index does not have it preloaded and
# gInterpreter.Declare is not allowed to load libPhysics for
# TLorentzVector. Preload the library now.
ROOT.gSystem.Load("libPhysics")
ROOT.gInterpreter.Declare("""
TLorentzVector create_tlorentzvector() {
auto v = TLorentzVector();
Expand Down
2 changes: 1 addition & 1 deletion core/metacling/src/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,11 @@ endif()

ROOT_OBJECT_LIBRARY(MetaCling
rootclingTCling.cxx
TCling.cxx
TClingBaseClassInfo.cxx
TClingCallbacks.cxx
TClingCallFunc.cxx
TClingClassInfo.cxx
TCling.cxx
TClingDataMemberInfo.cxx
TClingDeclInfo.cxx
TClingMethodArgInfo.cxx
Expand Down
78 changes: 72 additions & 6 deletions core/metacling/src/TCling.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1122,7 +1122,63 @@ static GlobalModuleIndex *loadGlobalModuleIndex(SourceLocation TriggerLoc, cling
RecreateIndex = true;
}
if (RecreateIndex) {
GlobalModuleIndex::writeIndex(CI.getFileManager(), CI.getPCHContainerReader(), ModuleIndexPath);
cling::Interpreter::PushTransactionRAII deserRAII(&interp);
clang::GlobalModuleIndex::UserDefinedInterestingIDs IDs;

struct DefinitionFinder : public RecursiveASTVisitor<DefinitionFinder> {
DefinitionFinder(clang::GlobalModuleIndex::UserDefinedInterestingIDs& IDs,
clang::TranslationUnitDecl* TU) : DefinitionIDs(IDs) {
TraverseDecl(TU);
}
bool VisitNamedDecl(NamedDecl *ND) {
for (auto R : ND->redecls()) {
if (!R->isFromASTFile())
continue;
if (TagDecl *TD = llvm::dyn_cast<TagDecl>(R)) {
if (TD->isCompleteDefinition())
Register(TD);
} else if (NamespaceDecl *NSD = llvm::dyn_cast<NamespaceDecl>(R))
Register(NSD, /*AddSingleEntry=*/ false);
else if (TypedefNameDecl *TND = dyn_cast<TypedefNameDecl>(R))
Register(TND);
Copy link
Member

@pcanal pcanal Jun 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't typedef decl be repeated (in several header files and thus several modules)?

Copy link
Member Author

@vgvassilev vgvassilev Jun 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but it should define the same entity, otherwise modules ODR hash will complain. So whichever we pick should be fine. I suspect that may lead to loading the 'wrong' module to resolve the intended typedef but cannot do anything better.

Alternatively, we could load all modules which have this typedef name -- that would be redundant because if the typedefs are all the same what's the point; if the typedefs are different we will get an ODR violation diagnostic.

Copy link
Member

@pcanal pcanal Jun 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to load the pcm for the target of the typedef?

Copy link
Member Author

@vgvassilev vgvassilev Jun 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we need to load something that has a clang::Decl for the missing identifier

// FIXME: Add the rest...
}
return true; // continue decending
}
private:
clang::GlobalModuleIndex::UserDefinedInterestingIDs &DefinitionIDs;
void Register(NamedDecl* ND, bool AddSingleEntry = true) {
assert(ND->isFromASTFile());
// FIXME: All decls should have an owning module once rootcling
// updates its generated decls from within the LookupHelper & co.
if (!ND->hasOwningModule()) {
#ifndef NDEBUG
SourceManager &SM = ND->getASTContext().getSourceManager();
SourceLocation Loc = ND->getLocation();
const FileEntry *FE = SM.getFileEntryForID(SM.getFileID(Loc));
(void)FE;
assert(FE->getName().contains("input_line_"));
#endif
return;
}

Module *OwningModule = ND->getOwningModule()->getTopLevelModule();
assert(OwningModule);
if (AddSingleEntry && DefinitionIDs.count(ND->getName()))
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());
}
};
DefinitionFinder defFinder(IDs, CI.getASTContext().getTranslationUnitDecl());

GlobalModuleIndex::writeIndex(CI.getFileManager(),
CI.getPCHContainerReader(),
ModuleIndexPath,
&IDs);
ModuleManager->resetForReload();
ModuleManager->loadGlobalIndex();
GlobalIndex = ModuleManager->getGlobalIndex();
Expand Down Expand Up @@ -1161,6 +1217,7 @@ static void RegisterCxxModules(cling::Interpreter &clingInterp)
"ROOT_Rtypes",
"ROOT_Foundation_Stage1_NoRTTI",
"Core",
"Rint",
"RIO"};

LoadModules(CoreModules, clingInterp);
Expand All @@ -1171,19 +1228,27 @@ static void RegisterCxxModules(cling::Interpreter &clingInterp)
LoadModules(CommonModules, clingInterp);

// These modules should not be preloaded but they fix issues.
std::vector<std::string> FIXMEModules = {"Hist", "Gpad", "Graf",
"GenVector", "Smatrix", "Tree",
"TreePlayer", "Physics",
"Proof", "Geom"};
// FIXME: Hist is not a core module but is very entangled to MathCore and
// causes issues.
std::vector<std::string> FIXMEModules = {"Hist"};
LoadModules(FIXMEModules, clingInterp);

clang::CompilerInstance &CI = *clingInterp.getCI();
GlobalModuleIndex *GlobalIndex = nullptr;
// Conservatively enable platform by platform.
bool supportedPlatform = false;
// Allow forcefully enabling the GMI.
const char *experimentalGMI = gSystem->Getenv("ROOT_EXPERIMENTAL_GMI");
if (experimentalGMI && strcmp(experimentalGMI,"false") != 0) {
if (experimentalGMI && strcmp(experimentalGMI,"false") != 0)
supportedPlatform = true;

if (supportedPlatform && !gSystem->Getenv("ROOT_DISABLE_GMI")) {
loadGlobalModuleIndex(SourceLocation(), clingInterp);
// FIXME: The ASTReader still calls loadGlobalIndex and loads the file
// We should investigate how to suppress it completely.
GlobalIndex = CI.getModuleManager()->getGlobalIndex();
}

llvm::StringSet<> KnownModuleFileNames;
if (GlobalIndex)
GlobalIndex->getKnownModuleFileNames(KnownModuleFileNames);
Expand Down Expand Up @@ -1771,6 +1836,7 @@ void TCling::LoadPCM(std::string pcmFileNameFullPath)
std::string RDictFileOpts = pcmFileNameFullPath + "?filetype=pcm";
TMemFile pcmMemFile(RDictFileOpts.c_str(), range);

cling::Interpreter::PushTransactionRAII deserRAII(GetInterpreterImpl());
LoadPCMImpl(pcmMemFile);
fPendingRdicts.erase(pendingRdict);

Expand Down
7 changes: 7 additions & 0 deletions core/metacling/src/TClingCallbacks.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,9 @@ bool TClingCallbacks::LookupObject(LookupResult &R, Scope *S) {

bool TClingCallbacks::findInGlobalModuleIndex(DeclarationName Name, bool loadFirstMatchOnly /*=true*/)
{
if (::getenv("ROOT_DISABLE_GMI"))
return false;

const CompilerInstance *CI = m_Interpreter->getCI();
const LangOptions &LangOpts = CI->getPreprocessor().getLangOpts();

Expand Down Expand Up @@ -315,6 +318,10 @@ bool TClingCallbacks::findInGlobalModuleIndex(DeclarationName Name, bool loadFir
for (auto FileName : FoundModules) {
StringRef ModuleName = llvm::sys::path::stem(*FileName);
fIsLoadingModule = true;
if (gDebug > 2)
llvm::errs() << "Loading " << ModuleName << " on demand"
<< " for " << Name.getAsString() << "\n";

m_Interpreter->loadModule(ModuleName);
fIsLoadingModule = false;
if (loadFirstMatchOnly)
Expand Down
7 changes: 6 additions & 1 deletion interpreter/llvm/src/tools/clang/include/clang/Serialization/GlobalModuleIndex.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,9 @@ class GlobalModuleIndex {
EC_IOError
};

using UserDefinedInterestingIDs =
llvm::StringMap<llvm::SmallVector<const FileEntry*, 2>>;

/// \brief Read a global index file for the given directory.
///
/// \param Path The path to the specific module cache where the module files
Expand Down Expand Up @@ -205,9 +208,11 @@ class GlobalModuleIndex {
/// creating modules.
/// \param Path The path to the directory containing module files, into
/// which the global index will be written.
/// \param Optionally pass already precomputed interesting identifiers.
static ErrorCode writeIndex(FileManager &FileMgr,
const PCHContainerReader &PCHContainerRdr,
StringRef Path);
StringRef Path,
UserDefinedInterestingIDs *ExternalIDs = nullptr);
};
}

Expand Down
95 changes: 54 additions & 41 deletions interpreter/llvm/src/tools/clang/lib/Serialization/GlobalModuleIndex.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -422,9 +422,6 @@ namespace {

/// \brief Builder that generates the global module index file.
class GlobalModuleIndexBuilder {
FileManager &FileMgr;
const PCHContainerReader &PCHContainerRdr;

/// Mapping from files to module file information.
typedef llvm::MapVector<const FileEntry *, ModuleFileInfo> ModuleFilesMap;

Expand Down Expand Up @@ -464,14 +461,20 @@ namespace {
}

public:
explicit GlobalModuleIndexBuilder(
FileManager &FileMgr, const PCHContainerReader &PCHContainerRdr)
: FileMgr(FileMgr), PCHContainerRdr(PCHContainerRdr) {}
explicit GlobalModuleIndexBuilder(GlobalModuleIndex::UserDefinedInterestingIDs* ExternalIDs) {
if (!ExternalIDs)
return;

for (const auto & I : *ExternalIDs)
for (const FileEntry * J : I.getValue())
InterestingIdentifiers[I.getKey()].push_back(getModuleFileInfo(J).ID);
}

/// \brief Load the contents of the given module file into the builder.
///
/// \returns true if an error occurred, false otherwise.
bool loadModuleFile(const FileEntry *File);
bool loadModuleFile(const FileEntry *File, FileManager &FileMgr,
const PCHContainerReader &PCHContainerRdr);

/// \brief Write the index to the given bitstream.
/// \returns true if an error occurred, false otherwise.
Expand Down Expand Up @@ -542,7 +545,9 @@ namespace {
};
}

bool GlobalModuleIndexBuilder::loadModuleFile(const FileEntry *File) {
bool GlobalModuleIndexBuilder::loadModuleFile(const FileEntry *File,
FileManager &FileMgr,
const PCHContainerReader &PCHContainerRdr) {
// Open the module file.

auto Buffer = FileMgr.getBufferForFile(File, /*isVolatile=*/true);
Expand Down Expand Up @@ -693,7 +698,10 @@ bool GlobalModuleIndexBuilder::loadModuleFile(const FileEntry *File) {
DEnd = Table->data_end();
D != DEnd; ++D) {
std::pair<StringRef, bool> Ident = *D;
InterestingIdentifiers[Ident.first].push_back(ID);
if (Ident.second)
InterestingIdentifiers[Ident.first].push_back(ID);
else
(void)InterestingIdentifiers[Ident.first];
}
}

Expand Down Expand Up @@ -755,14 +763,15 @@ bool GlobalModuleIndexBuilder::writeIndex(llvm::BitstreamWriter &Stream) {
for (auto MapEntry : ImportedModuleFiles) {
auto *File = MapEntry.first;
ImportedModuleFileInfo &Info = MapEntry.second;
// if (getModuleFileInfo(File).Signature) {
// if (getModuleFileInfo(File).Signature != Info.StoredSignature)
// // Verify Signature.
// return true;
// } else if (Info.StoredSize != File->getSize() ||
// Info.StoredModTime != File->getModificationTime())
// // Verify Size and ModTime.
// return true;
if (getModuleFileInfo(File).Signature) {
if (getModuleFileInfo(File).Signature != Info.StoredSignature)
// Verify Signature.
return true;
} else if (Info.StoredSize != File->getSize() ||
(Info.StoredModTime &&
Info.StoredModTime != File->getModificationTime()))
// Verify Size and ModTime.
return true;
}

using namespace llvm;
Expand Down Expand Up @@ -846,7 +855,9 @@ bool GlobalModuleIndexBuilder::writeIndex(llvm::BitstreamWriter &Stream) {
GlobalModuleIndex::ErrorCode
GlobalModuleIndex::writeIndex(FileManager &FileMgr,
const PCHContainerReader &PCHContainerRdr,
StringRef Path) {
StringRef Path,
UserDefinedInterestingIDs *ExternalIDs /* = nullptr */) {

llvm::SmallString<128> IndexPath;
IndexPath += Path;
llvm::sys::path::append(IndexPath, IndexFileName);
Expand All @@ -869,32 +880,34 @@ GlobalModuleIndex::writeIndex(FileManager &FileMgr,
}

// The module index builder.
GlobalModuleIndexBuilder Builder(FileMgr, PCHContainerRdr);

// Load each of the module files.
std::error_code EC;
for (llvm::sys::fs::directory_iterator D(Path, EC), DEnd;
D != DEnd && !EC;
D.increment(EC)) {
// If this isn't a module file, we don't care.
if (llvm::sys::path::extension(D->path()) != ".pcm") {
// ... unless it's a .pcm.lock file, which indicates that someone is
// in the process of rebuilding a module. They'll rebuild the index
// at the end of that translation unit, so we don't have to.
if (llvm::sys::path::extension(D->path()) == ".pcm.lock")
return EC_Building;
GlobalModuleIndexBuilder Builder(ExternalIDs);

if (!ExternalIDs) {
// Load each of the module files.
std::error_code EC;
for (llvm::sys::fs::directory_iterator D(Path, EC), DEnd;
D != DEnd && !EC;
D.increment(EC)) {
// If this isn't a module file, we don't care.
if (llvm::sys::path::extension(D->path()) != ".pcm") {
// ... unless it's a .pcm.lock file, which indicates that someone is
// in the process of rebuilding a module. They'll rebuild the index
// at the end of that translation unit, so we don't have to.
if (llvm::sys::path::extension(D->path()) == ".pcm.lock")
Copy link
Member

@pcanal pcanal Jun 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens in the follow scenario:

(1) Process A starts and build a module and thus creates a .pcm.lock
(2) Process A is put to sleep (and/or is very very slow)
(3) some other pcm is created fully (and have names such it is early in the directory iteration).
(4) Process B starts and try to write the index and gives ups
(5) Process B finishes
(6) Process A wakes ups and finish write the module.idx.

Are the extra pcm creates in step (3) included in the module.idx (create by Process A)

Copy link
Member Author

@vgvassilev vgvassilev Jun 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will not add it in the index probably.

Note that this is pre-existing llvm code and the ROOT GMI does not actually use it. It provides a table with ExternalIDs

Copy link
Member

@pcanal pcanal Jun 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will not add it in the index probably.

I see. Then the comment is not appropriate for our use case [And the result is (silently) sub-optimal, isn't it?]

Copy link
Member Author

@vgvassilev vgvassilev Jun 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. Let’s see if this will improve when we upgrade llvm.

return EC_Building;

continue;
}
continue;
}

// If we can't find the module file, skip it.
const FileEntry *ModuleFile = FileMgr.getFile(D->path());
if (!ModuleFile)
continue;
// If we can't find the module file, skip it.
const FileEntry *ModuleFile = FileMgr.getFile(D->path());
if (!ModuleFile)
continue;

// Load this module file.
if (Builder.loadModuleFile(ModuleFile))
return EC_IOError;
// Load this module file.
if (Builder.loadModuleFile(ModuleFile, FileMgr, PCHContainerRdr))
return EC_IOError;
}
}

// The output buffer, into which the global index will be written.
Expand Down