From 67d25fede9aa7be37b2dcd20e3402f3f190e41f9 Mon Sep 17 00:00:00 2001 From: Alex Lorenz Date: Tue, 27 Aug 2019 01:03:25 +0000 Subject: [PATCH] Use FileEntryRef for PPCallbacks::FileSkipped This fixes the issue where a filename dependendency was missing if the file that was skipped was included through a symlink in an earlier run, if the file manager was reused between runs. llvm-svn: 369998 --- clang-tools-extra/clangd/ClangdUnit.cpp | 4 +- .../pp-trace/PPCallbacksTracker.cpp | 4 +- .../pp-trace/PPCallbacksTracker.h | 2 +- clang/include/clang/Lex/PPCallbacks.h | 8 ++-- clang/lib/Frontend/DependencyFile.cpp | 2 +- .../Frontend/Rewrite/InclusionRewriter.cpp | 6 +-- clang/lib/Lex/PPDirectives.cpp | 2 +- .../Tooling/DependencyScannerTest.cpp | 42 ++++++++++++++++++- 8 files changed, 55 insertions(+), 15 deletions(-) diff --git a/clang-tools-extra/clangd/ClangdUnit.cpp b/clang-tools-extra/clangd/ClangdUnit.cpp index 82ca7eda2e46..f85cac200da3 100644 --- a/clang-tools-extra/clangd/ClangdUnit.cpp +++ b/clang-tools-extra/clangd/ClangdUnit.cpp @@ -268,7 +268,9 @@ private: FilenameTok.getEndLoc()), File, "SearchPath", "RelPath", /*Imported=*/nullptr, Inc.FileKind); if (File) - Delegate->FileSkipped(*File, FilenameTok, Inc.FileKind); + // FIXME: Use correctly named FileEntryRef. + Delegate->FileSkipped(FileEntryRef(File->getName(), *File), FilenameTok, + Inc.FileKind); else { llvm::SmallString<1> UnusedRecovery; Delegate->FileNotFound(WrittenFilename, UnusedRecovery); diff --git a/clang-tools-extra/pp-trace/PPCallbacksTracker.cpp b/clang-tools-extra/pp-trace/PPCallbacksTracker.cpp index 4b8a658b4389..6000dcb3a87e 100644 --- a/clang-tools-extra/pp-trace/PPCallbacksTracker.cpp +++ b/clang-tools-extra/pp-trace/PPCallbacksTracker.cpp @@ -112,11 +112,11 @@ void PPCallbacksTracker::FileChanged(SourceLocation Loc, // Callback invoked whenever a source file is skipped as the result // of header guard optimization. -void PPCallbacksTracker::FileSkipped(const FileEntry &SkippedFile, +void PPCallbacksTracker::FileSkipped(const FileEntryRef &SkippedFile, const Token &FilenameTok, SrcMgr::CharacteristicKind FileType) { beginCallback("FileSkipped"); - appendArgument("ParentFile", &SkippedFile); + appendArgument("ParentFile", &SkippedFile.getFileEntry()); appendArgument("FilenameTok", FilenameTok); appendArgument("FileType", FileType, CharacteristicKindStrings); } diff --git a/clang-tools-extra/pp-trace/PPCallbacksTracker.h b/clang-tools-extra/pp-trace/PPCallbacksTracker.h index 726a393a1beb..da5d1b68889f 100644 --- a/clang-tools-extra/pp-trace/PPCallbacksTracker.h +++ b/clang-tools-extra/pp-trace/PPCallbacksTracker.h @@ -89,7 +89,7 @@ public: void FileChanged(SourceLocation Loc, PPCallbacks::FileChangeReason Reason, SrcMgr::CharacteristicKind FileType, FileID PrevFID = FileID()) override; - void FileSkipped(const FileEntry &SkippedFile, const Token &FilenameTok, + void FileSkipped(const FileEntryRef &SkippedFile, const Token &FilenameTok, SrcMgr::CharacteristicKind FileType) override; bool FileNotFound(llvm::StringRef FileName, llvm::SmallVectorImpl &RecoveryPath) override; diff --git a/clang/include/clang/Lex/PPCallbacks.h b/clang/include/clang/Lex/PPCallbacks.h index f3f3796b1a30..5f31b98a3cc1 100644 --- a/clang/include/clang/Lex/PPCallbacks.h +++ b/clang/include/clang/Lex/PPCallbacks.h @@ -57,10 +57,9 @@ public: /// \param FilenameTok The file name token in \#include "FileName" directive /// or macro expanded file name token from \#include MACRO(PARAMS) directive. /// Note that FilenameTok contains corresponding quotes/angles symbols. - virtual void FileSkipped(const FileEntry &SkippedFile, + virtual void FileSkipped(const FileEntryRef &SkippedFile, const Token &FilenameTok, - SrcMgr::CharacteristicKind FileType) { - } + SrcMgr::CharacteristicKind FileType) {} /// Callback invoked whenever an inclusion directive results in a /// file-not-found error. @@ -390,8 +389,7 @@ public: Second->FileChanged(Loc, Reason, FileType, PrevFID); } - void FileSkipped(const FileEntry &SkippedFile, - const Token &FilenameTok, + void FileSkipped(const FileEntryRef &SkippedFile, const Token &FilenameTok, SrcMgr::CharacteristicKind FileType) override { First->FileSkipped(SkippedFile, FilenameTok, FileType); Second->FileSkipped(SkippedFile, FilenameTok, FileType); diff --git a/clang/lib/Frontend/DependencyFile.cpp b/clang/lib/Frontend/DependencyFile.cpp index aeff571c145b..86e9ccfe3a40 100644 --- a/clang/lib/Frontend/DependencyFile.cpp +++ b/clang/lib/Frontend/DependencyFile.cpp @@ -59,7 +59,7 @@ struct DepCollectorPPCallbacks : public PPCallbacks { /*IsModuleFile*/false, /*IsMissing*/false); } - void FileSkipped(const FileEntry &SkippedFile, const Token &FilenameTok, + void FileSkipped(const FileEntryRef &SkippedFile, const Token &FilenameTok, SrcMgr::CharacteristicKind FileType) override { StringRef Filename = llvm::sys::path::remove_leading_dotslash(SkippedFile.getName()); diff --git a/clang/lib/Frontend/Rewrite/InclusionRewriter.cpp b/clang/lib/Frontend/Rewrite/InclusionRewriter.cpp index f8388c506860..984b63866422 100644 --- a/clang/lib/Frontend/Rewrite/InclusionRewriter.cpp +++ b/clang/lib/Frontend/Rewrite/InclusionRewriter.cpp @@ -70,7 +70,7 @@ private: void FileChanged(SourceLocation Loc, FileChangeReason Reason, SrcMgr::CharacteristicKind FileType, FileID PrevFID) override; - void FileSkipped(const FileEntry &SkippedFile, const Token &FilenameTok, + void FileSkipped(const FileEntryRef &SkippedFile, const Token &FilenameTok, SrcMgr::CharacteristicKind FileType) override; void InclusionDirective(SourceLocation HashLoc, const Token &IncludeTok, StringRef FileName, bool IsAngled, @@ -169,8 +169,8 @@ void InclusionRewriter::FileChanged(SourceLocation Loc, /// Called whenever an inclusion is skipped due to canonical header protection /// macros. -void InclusionRewriter::FileSkipped(const FileEntry &/*SkippedFile*/, - const Token &/*FilenameTok*/, +void InclusionRewriter::FileSkipped(const FileEntryRef & /*SkippedFile*/, + const Token & /*FilenameTok*/, SrcMgr::CharacteristicKind /*FileType*/) { assert(LastInclusionLocation.isValid() && "A file, that wasn't found via an inclusion directive, was skipped"); diff --git a/clang/lib/Lex/PPDirectives.cpp b/clang/lib/Lex/PPDirectives.cpp index 2642807b7316..5eac1da115c0 100644 --- a/clang/lib/Lex/PPDirectives.cpp +++ b/clang/lib/Lex/PPDirectives.cpp @@ -2029,7 +2029,7 @@ Preprocessor::ImportAction Preprocessor::HandleHeaderIncludeOrImport( RelativePath, Action == Import ? SuggestedModule.getModule() : nullptr, FileCharacter); if (Action == Skip && File) - Callbacks->FileSkipped(File->getFileEntry(), FilenameTok, FileCharacter); + Callbacks->FileSkipped(*File, FilenameTok, FileCharacter); } if (!File) diff --git a/clang/unittests/Tooling/DependencyScannerTest.cpp b/clang/unittests/Tooling/DependencyScannerTest.cpp index 1ca9e12bf5c8..458bb9e01740 100644 --- a/clang/unittests/Tooling/DependencyScannerTest.cpp +++ b/clang/unittests/Tooling/DependencyScannerTest.cpp @@ -37,7 +37,8 @@ public: : DependencyFileGenerator(Opts), Deps(Deps) {} void finishedMainFile(DiagnosticsEngine &Diags) override { - Deps = getDependencies(); + auto NewDeps = getDependencies(); + Deps.insert(Deps.end(), NewDeps.begin(), NewDeps.end()); } private: @@ -121,5 +122,44 @@ TEST(DependencyScanner, ScanDepsReuseFilemanager) { EXPECT_EQ(Files.getNumUniqueRealFiles(), 2u); } +TEST(DependencyScanner, ScanDepsReuseFilemanagerSkippedFile) { + std::vector Compilation = {"-c", "-E", "-MT", "test.cpp.o"}; + StringRef CWD = "/root"; + FixedCompilationDatabase CDB(CWD, Compilation); + + auto VFS = new llvm::vfs::InMemoryFileSystem(); + VFS->setCurrentWorkingDirectory(CWD); + auto Sept = llvm::sys::path::get_separator(); + std::string HeaderPath = llvm::formatv("{0}root{0}header.h", Sept); + std::string SymlinkPath = llvm::formatv("{0}root{0}symlink.h", Sept); + std::string TestPath = llvm::formatv("{0}root{0}test.cpp", Sept); + std::string Test2Path = llvm::formatv("{0}root{0}test2.cpp", Sept); + + VFS->addFile(HeaderPath, 0, + llvm::MemoryBuffer::getMemBuffer("#pragma once\n")); + VFS->addHardLink(SymlinkPath, HeaderPath); + VFS->addFile(TestPath, 0, + llvm::MemoryBuffer::getMemBuffer( + "#include \"header.h\"\n#include \"symlink.h\"\n")); + VFS->addFile(Test2Path, 0, + llvm::MemoryBuffer::getMemBuffer( + "#include \"symlink.h\"\n#include \"header.h\"\n")); + + ClangTool Tool(CDB, {"test.cpp", "test2.cpp"}, + std::make_shared(), VFS); + Tool.clearArgumentsAdjusters(); + std::vector Deps; + TestDependencyScanningAction Action(Deps); + Tool.run(&Action); + using llvm::sys::path::convert_to_slash; + ASSERT_EQ(Deps.size(), 6u); + EXPECT_EQ(convert_to_slash(Deps[0]), "/root/test.cpp"); + EXPECT_EQ(convert_to_slash(Deps[1]), "/root/header.h"); + EXPECT_EQ(convert_to_slash(Deps[2]), "/root/symlink.h"); + EXPECT_EQ(convert_to_slash(Deps[3]), "/root/test2.cpp"); + EXPECT_EQ(convert_to_slash(Deps[4]), "/root/symlink.h"); + EXPECT_EQ(convert_to_slash(Deps[5]), "/root/header.h"); +} + } // end namespace tooling } // end namespace clang