Skip to content
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.

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

Change working directory of C++ parser #572

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
1 change: 1 addition & 0 deletions model/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
set(ODB_SOURCES
include/model/buildaction.h
include/model/builddirectory.h
include/model/buildlog.h
include/model/buildsourcetarget.h
include/model/filecontent.h
Expand Down
37 changes: 37 additions & 0 deletions model/include/model/builddirectory.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
#ifndef CC_MODEL_BUILDDIRECTORY_H
#define CC_MODEL_BUILDDIRECTORY_H

#include <memory>

#include <odb/core.hxx>

#include <model/buildaction.h>
#include <model/file.h>

namespace cc
{
namespace model
{

struct BuildAction;

#pragma db object
struct BuildDirectory
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure why this has to be a brand new table, foreign key, and all that jazz for the storage. BuildAction already contains string command;, for example. If "directory" is just another member of the entries in a compilation database JSON (just like "command"), then why isn't this reflected in the schema? Every build action MUST have a build directory (it just may or may not exist or point to anything meaningful).

This struct, as it is now, won't do uniqueing of build directories either, because it is the build directory that refers to one build action each. If we want a separate record to only store one directory ("string") exactly once, then it is the buildaction that has to refer into this relation's key.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to keep backwards compatibility with existing databases, that's why I didn't want to change BuildAction's schema. Is that not a priority? In that case you're right, storing it in BuildAction would make more sense.

Deduplicating the strings is not worth the hassle, in the CodeCompass SQLite database BuildAction only takes up 0.012% of the space.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We can easily reparse projects, backward-compatibility with existing databases is not a high priority.

{
#pragma db id auto
std::uint64_t id;

#pragma db not_null
std::string directory;

#pragma db not_null
#pragma db on_delete(cascade)
std::shared_ptr<BuildAction> action;
};

typedef std::shared_ptr<BuildDirectory> BuildDirectoryPtr;

} // model
} // cc

#endif // CC_MODEL_BUILDDIRECTORY_H
4 changes: 2 additions & 2 deletions parser/include/parser/sourcemanager.h
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,8 @@ class SourceManager
* based on the given path_. The object is read from a cache. If the file is
* not in the cache yet then a model::File entry is created, persisted in the
* database and placed in the cache. If the file doesn't exist then it returns
* nullptr.
* @param path_ The file path to look up.
* a file with no contents.
* @param path_ The absolute file path to look up.
*/
model::FilePtr getFile(const std::string& path_);

Expand Down
2 changes: 1 addition & 1 deletion parser/src/sourcemanager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ model::FilePtr SourceManager::getFile(const std::string& path_)
boost::filesystem::path canonicalPath
= boost::filesystem::canonical(path_, ec);

//--- If the file can't be found on disk then return nullptr ---//
//--- If the file can't be found on disk then return a blank file ---//

bool fileExists = true;
if (ec)
Expand Down
24 changes: 16 additions & 8 deletions plugins/cpp/parser/include/cppparser/filelocutil.h
Original file line number Diff line number Diff line change
Expand Up @@ -79,23 +79,31 @@ class FileLocUtil
}

/**
* This function returns the file path in which loc_ location takes place. The
* location is meant to be the expanded location (in case of macro expansion).
* If the file can't be determined then empty string returns.
* This function returns the absolute path of the file in which loc_ location
* takes place. The location is meant to be the expanded location (in case of
* macro expansion).
* If the file can't be determined then an empty string is returned.
*/
std::string getFilePath(const clang::SourceLocation& loc_)
{
clang::SourceLocation expLoc = _clangSrcMan.getExpansionLoc(loc_);
clang::FileID fid = _clangSrcMan.getFileID(expLoc);
return getFilePath(
_clangSrcMan.getFileID(_clangSrcMan.getExpansionLoc(loc_)));
}

if (fid.isInvalid())
/**
* This function returns the absolute path of the file identified by fid_.
* If the file can't be determined then an empty string is returned.
*/
std::string getFilePath(const clang::FileID& fid_)
{
if (fid_.isInvalid())
return std::string();

const clang::FileEntry* fileEntry = _clangSrcMan.getFileEntryForID(fid);
const clang::FileEntry* fileEntry = _clangSrcMan.getFileEntryForID(fid_);
if (!fileEntry)
return std::string();

return fileEntry->getName();
return fileEntry->tryGetRealPathName();
}

private:
Expand Down
29 changes: 23 additions & 6 deletions plugins/cpp/parser/src/cppparser.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@

#include <model/buildaction.h>
#include <model/buildaction-odb.hxx>
#include <model/builddirectory.h>
#include <model/builddirectory-odb.hxx>
#include <model/buildsourcetarget.h>
#include <model/buildsourcetarget-odb.hxx>
#include <model/file.h>
Expand Down Expand Up @@ -254,6 +256,7 @@ void CppParser::addCompileCommand(

std::vector<model::BuildSource> sources;
std::vector<model::BuildTarget> targets;
model::BuildDirectory buildDir;

for (const auto& srcTarget : extractInputOutputs(command_))
{
Expand All @@ -277,6 +280,9 @@ void CppParser::addCompileCommand(

targets.push_back(std::move(buildTarget));
}

buildDir.directory = command_.Directory;
buildDir.action = buildAction_;

_ctx.srcMgr.persistFiles();

Expand All @@ -285,6 +291,7 @@ void CppParser::addCompileCommand(
_ctx.db->persist(buildSource);
for (model::BuildTarget buildTarget : targets)
_ctx.db->persist(buildTarget);
_ctx.db->persist(buildDir);
});
}

Expand All @@ -303,12 +310,21 @@ int CppParser::parseWorker(const clang::tooling::CompileCommand& command_)

int argc = commandLine.size();

std::string buildDir = command_.Directory;
if (!fs::is_directory(buildDir))
{
LOG(debug) << "Compilation directory " << buildDir
<< " is missing, using '/' instead.";
buildDir = "/";
}

std::string compilationDbLoadError;
std::unique_ptr<clang::tooling::FixedCompilationDatabase> compilationDb(
clang::tooling::FixedCompilationDatabase::loadFromCommandLine(
argc,
commandLine.data(),
compilationDbLoadError));
compilationDbLoadError,
buildDir));

if (!compilationDb)
{
Expand All @@ -324,12 +340,13 @@ int CppParser::parseWorker(const clang::tooling::CompileCommand& command_)

//--- Start the tool ---//

fs::path sourceFullPath(command_.Filename);
if (!sourceFullPath.is_absolute())
sourceFullPath = fs::path(command_.Directory) / command_.Filename;

VisitorActionFactory factory(_ctx);
clang::tooling::ClangTool tool(*compilationDb, sourceFullPath.string());

// Use a PhysicalFileSystem as it's thread-safe

clang::tooling::ClangTool tool(*compilationDb, command_.Filename,
std::make_shared<clang::PCHContainerOperations>(),
llvm::vfs::createPhysicalFileSystem().release());

llvm::IntrusiveRefCntPtr<clang::DiagnosticOptions> diagOpts
= new clang::DiagnosticOptions();
Expand Down
15 changes: 9 additions & 6 deletions plugins/cpp/parser/src/ppincludecallback.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -58,10 +58,10 @@ model::CppAstNodePtr PPIncludeCallback::createFileAstNode(
void PPIncludeCallback::InclusionDirective(
clang::SourceLocation hashLoc_,
const clang::Token&,
clang::StringRef fileName_,
clang::StringRef,
bool,
clang::CharSourceRange filenameRange_,
const clang::FileEntry*,
const clang::FileEntry* file_,
clang::StringRef searchPath_,
clang::StringRef,
const clang::Module*,
Expand All @@ -70,13 +70,16 @@ void PPIncludeCallback::InclusionDirective(
if (searchPath_.empty())
return;

if (!file_)
return;

clang::SourceLocation expLoc = _clangSrcMgr.getExpansionLoc(hashLoc_);
clang::PresumedLoc presLoc = _clangSrcMgr.getPresumedLoc(expLoc);

//--- Included file ---//

std::string includedPath = searchPath_.str() + '/' + fileName_.str();
model::FilePtr included = _ctx.srcMgr.getFile(includedPath);
model::FilePtr included = _ctx.srcMgr.getFile(
file_->tryGetRealPathName().str());
included->parseStatus = model::File::PSFullyParsed;
if (included->type != model::File::DIRECTORY_TYPE &&
included->type != _cppSourceType)
Expand All @@ -87,8 +90,8 @@ void PPIncludeCallback::InclusionDirective(

//--- Includer file ---//

std::string includerPath = presLoc.getFilename();
model::FilePtr includer = _ctx.srcMgr.getFile(includerPath);
model::FilePtr includer = _ctx.srcMgr.getFile(
_fileLocUtil.getFilePath(presLoc.getFileID()));
includer->parseStatus = model::File::PSFullyParsed;
if (includer->type != model::File::DIRECTORY_TYPE &&
includer->type != _cppSourceType)
Expand Down
6 changes: 4 additions & 2 deletions plugins/cpp/parser/src/ppmacrocallback.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -254,10 +254,12 @@ std::string PPMacroCallback::getUSR(const clang::MacroInfo* mi_)
= std::to_string(presLoc.getLine()) + ":" +
std::to_string(presLoc.getColumn()) + ":";

std::string filePath = _fileLocUtil.getFilePath(presLoc.getFileID());

return locStr
+ (isBuiltInMacro(mi_)
? presLoc.getFilename()
: std::to_string(_ctx.srcMgr.getFile(presLoc.getFilename())->id));
? filePath
: std::to_string(_ctx.srcMgr.getFile(filePath)->id));
}

} // parser
Expand Down
42 changes: 31 additions & 11 deletions plugins/cpp_reparse/service/src/databasefilesystem.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ DatabaseFileSystem::DatabaseFileSystem(std::shared_ptr<odb::database> db_)

ErrorOr<Status> DatabaseFileSystem::status(const Twine& path_)
{
model::FilePtr file = getFile(*_db, path_.str());
model::FilePtr file = getFile(*_db, toCanonicalOrSame(path_));
if (!file)
return std::error_code(ENOENT, std::generic_category());

Expand All @@ -211,7 +211,7 @@ ErrorOr<Status> DatabaseFileSystem::status(const Twine& path_)
ErrorOr<std::unique_ptr<File>>
DatabaseFileSystem::openFileForRead(const Twine& path_)
{
model::FilePtr file = getFile(*_db, path_.str());
model::FilePtr file = getFile(*_db, toCanonicalOrSame(path_));
if (!file)
return std::error_code(ENOENT, std::generic_category());
if (file->type == model::File::DIRECTORY_TYPE)
Expand All @@ -231,7 +231,7 @@ DatabaseFileSystem::openFileForRead(const Twine& path_)
directory_iterator
DatabaseFileSystem::dir_begin(const Twine& dir_, std::error_code& ec_)
{
model::FilePtr dirFile = getFile(*_db, dir_.str());
model::FilePtr dirFile = getFile(*_db, toCanonicalOrSame(dir_));
if (dirFile && dirFile->type == model::File::DIRECTORY_TYPE)
return directory_iterator(std::make_shared<DatabaseDirectoryIterator>(
*_db, dirFile, ec_));
Expand All @@ -248,16 +248,36 @@ ErrorOr<std::string> DatabaseFileSystem::getCurrentWorkingDirectory() const
std::error_code
DatabaseFileSystem::setCurrentWorkingDirectory(const Twine& path_)
{
SmallString<128> fullPath;
sys::fs::make_absolute(_currentWorkingDirectory, fullPath);
sys::path::append(fullPath, path_);
std::error_code error = sys::fs::make_absolute(fullPath);
llvm::ErrorOr<std::string> newWorkDir = toCanonical(path_);
if (!newWorkDir.getError())
_currentWorkingDirectory = *newWorkDir;

if (!error)
_currentWorkingDirectory = fullPath.str().str();
return newWorkDir.getError();
}

llvm::ErrorOr<std::string>
DatabaseFileSystem::toCanonical(const llvm::Twine& relPath_) const
{
llvm::SmallString<128> absPath, canonicalPath;

relPath_.toVector(absPath);

llvm::sys::fs::make_absolute(_currentWorkingDirectory, absPath);

getCurrentWorkingDirectory();
return error;
if (std::error_code ec = llvm::sys::fs::real_path(absPath, canonicalPath))
return ec;

return canonicalPath.str();
}

std::string
DatabaseFileSystem::toCanonicalOrSame(const llvm::Twine& relPath_) const
{
auto canonical = toCanonical(relPath_);
if (canonical.getError())
return relPath_.str();
else
return *canonical;
}

} // namespace reparse
Expand Down
4 changes: 4 additions & 0 deletions plugins/cpp_reparse/service/src/databasefilesystem.h
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,10 @@ class DatabaseFileSystem : public llvm::vfs::FileSystem
util::OdbTransaction _transaction;

std::string _currentWorkingDirectory;

llvm::ErrorOr<std::string> toCanonical(const llvm::Twine& relPath_) const;

std::string toCanonicalOrSame(const llvm::Twine& relPath_) const;
};

} //namespace reparse
Expand Down
Loading