subject:"\[PATCH\] D65907\: Introduce FileEntryRef and use it when handling includes to report correct dependencies when the FileManager is reused across invocations"

[PATCH] D65907: Introduce FileEntryRef and use it when handling includes to report correct dependencies when the FileManager is reused across invocations

2019-08-27 Thread James Nagurne via Phabricator via cfe-commits

JamesNagurne added a comment.

In D65907#1645474 , @arphaman wrote:

> In D65907#1643800 , @JamesNagurne 
> wrote:
>
> > In D65907#1643650 , @arphaman 
> > wrote:
> >
> > > No the windows test failure was different, there were no Deps at all. I'm 
> > > currently investigating it on a windows VM.
> > >
> > > @JamesNagurne I think there's some issue with the working directory, 
> > > which is not added in your case. Which platform are you running your 
> > > build/test on? Which cmake options are you using?
> >
> >
> > I apologize for not giving such information in the first reply. 
> > Unfortunately this isn't an easy remote reproduction, as our ToolChain and 
> > some integral changes aren't upstreamed. This is an embedded ARM 
> > cross-compiled on Linux. Might be able to reproduce with arm-none-none-eabi.
> >  LLVM is built as an external project. Looking at the build system, it 
> > looks like we have the CMAKE_ARGS:
> >
> >  
> >   -DLLVM_DEFAULT_TARGET_TRIPLE=arm-ti-none-eabi
> >   -DLLVM_EXTERNAL_CLANG_SOURCE_DIR=${CMAKE_SOURCE_DIR}/llvm-project/clang
> >   -DLLVM_TARGETS_TO_BUILD=ARM
> >   -DCMAKE_BUILD_TYPE=Release
> >   -DCMAKE_CXX_COMPILER=clang++
> >   -DCMAKE_C_COMPILER=clang
> >   -DLLVM_USE_LINKER=gold
> >   -GNinja
> >   
> >
> > Nothing suspicious, except maybe the default triple and ARM target.
> >  Looking at our (not upstream) toolchain file, we do have some RTLibs, 
> > LibInternal, libcxx, and System include changes, along with a 
> > -nostdsysteminc to avoid pulling host includes into our cross compiler. 
> > However, none of this should affect general "#include" behavior, correct?
> >  Another glance at your changes don't seem to affect any target-specific 
> > handling either, at least directly.
>
>
> Yes, I don't see anything in your changes that is an obvious thing to blame.
>
> > I might have to just bite the bullet and drop into the test with a debugger.
>
> I fixed the Windows issue, and it was completely unrelated to your issue. It 
> looks like the FileManager isn't constructing absolute paths when accessing 
> the files, which is somewhat unexpected. It would be useful to debug 
> invocations of `getFileEntryRef`, to see if the `InterndFilename` in the 
> function is getting changed into an absolute path or not.


We found and fixed the issue on our end. Turns out we had a nefarious little 
piece of code in AddClangSystemIncludeArgs:

  if (!DriverArgs.hasArg(options::OPT_nostdlibinc)) {
SmallString<128> Dir(getDriver().SysRoot);
llvm::sys::path::append(Dir, "include");
addSystemInclude(DriverArgs, CC1Args, Dir.str());
  }

Turns out that, as a cross compiler, we had set SysRoot to the empty string. 
So, we were adding "-internal-isystem" "include" to the cc1 arguments, which 
somehow caused the test to fail. I don't know precisely why it exposes issues 
in your test, but the code is clearly busted, so we've removed it.

I appreciate the responses as we've worked through the problems on our end!


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65907/new/

https://reviews.llvm.org/D65907



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65907: Introduce FileEntryRef and use it when handling includes to report correct dependencies when the FileManager is reused across invocations

2019-08-26 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman added a comment.

In D65907#1643800 , @JamesNagurne 
wrote:

> In D65907#1643650 , @arphaman wrote:
>
> > No the windows test failure was different, there were no Deps at all. I'm 
> > currently investigating it on a windows VM.
> >
> > @JamesNagurne I think there's some issue with the working directory, which 
> > is not added in your case. Which platform are you running your build/test 
> > on? Which cmake options are you using?
>
>
> I apologize for not giving such information in the first reply. Unfortunately 
> this isn't an easy remote reproduction, as our ToolChain and some integral 
> changes aren't upstreamed. This is an embedded ARM cross-compiled on Linux. 
> Might be able to reproduce with arm-none-none-eabi.
>  LLVM is built as an external project. Looking at the build system, it looks 
> like we have the CMAKE_ARGS:
>
>  
>   -DLLVM_DEFAULT_TARGET_TRIPLE=arm-ti-none-eabi
>   -DLLVM_EXTERNAL_CLANG_SOURCE_DIR=${CMAKE_SOURCE_DIR}/llvm-project/clang
>   -DLLVM_TARGETS_TO_BUILD=ARM
>   -DCMAKE_BUILD_TYPE=Release
>   -DCMAKE_CXX_COMPILER=clang++
>   -DCMAKE_C_COMPILER=clang
>   -DLLVM_USE_LINKER=gold
>   -GNinja
>   
>
> Nothing suspicious, except maybe the default triple and ARM target.
>  Looking at our (not upstream) toolchain file, we do have some RTLibs, 
> LibInternal, libcxx, and System include changes, along with a -nostdsysteminc 
> to avoid pulling host includes into our cross compiler. However, none of this 
> should affect general "#include" behavior, correct?
>  Another glance at your changes don't seem to affect any target-specific 
> handling either, at least directly.


Yes, I don't see anything in your changes that is an obvious thing to blame.

> I might have to just bite the bullet and drop into the test with a debugger.

I fixed the Windows issue, and it was completely unrelated to your issue. It 
looks like the FileManager isn't constructing absolute paths when accessing the 
files, which is somewhat unexpected. It would be useful to debug invocations of 
`getFileEntryRef`, to see if the `InterndFilename` in the function is getting 
changed into an absolute path or not.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65907/new/

https://reviews.llvm.org/D65907



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65907: Introduce FileEntryRef and use it when handling includes to report correct dependencies when the FileManager is reused across invocations

2019-08-23 Thread James Nagurne via Phabricator via cfe-commits

JamesNagurne added a comment.

In D65907#1643650 , @arphaman wrote:

> No the windows test failure was different, there were no Deps at all. I'm 
> currently investigating it on a windows VM.
>
> @JamesNagurne I think there's some issue with the working directory, which is 
> not added in your case. Which platform are you running your build/test on? 
> Which cmake options are you using?

I apologize for not giving such information in the first reply. Unfortunately 
this isn't an easy remote reproduction, as our ToolChain and some integral 
changes aren't upstreamed. This is an embedded ARM cross-compiled on Linux. 
Might be able to reproduce with arm-none-none-eabi.
LLVM is built as an external project. Looking at the build system, it looks 
like we have the CMAKE_ARGS:

  -DLLVM_DEFAULT_TARGET_TRIPLE=arm-ti-none-eabi
  -DLLVM_EXTERNAL_CLANG_SOURCE_DIR=${CMAKE_SOURCE_DIR}/llvm-project/clang
  -DLLVM_TARGETS_TO_BUILD=ARM
  -DCMAKE_BUILD_TYPE=Release
  -DCMAKE_CXX_COMPILER=clang++
  -DCMAKE_C_COMPILER=clang
  -DLLVM_USE_LINKER=gold
  -GNinja

Nothing suspicious, except maybe the default triple and ARM target.
Looking at our (not upstream) toolchain file, we do have some RTLibs, 
LibInternal, libcxx, and System include changes, along with a -nostdsysteminc 
to avoid pulling host includes into our cross compiler. However, none of this 
should affect general "#include" behavior, correct?
Another glance at your changes don't seem to affect any target-specific 
handling either, at least directly.

I might have to just bite the bullet and drop into the test with a debugger.

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65907/new/

https://reviews.llvm.org/D65907

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65907: Introduce FileEntryRef and use it when handling includes to report correct dependencies when the FileManager is reused across invocations

2019-08-23 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman added a comment.

In D65907#1643364 , @JamesNagurne 
wrote:

> @arphaman you disabled this test on Windows, but did not specify exactly how 
> it fails.
>  My team works on an embedded ARM compiler (most similar to arm-none-eabi), 
> and we're now seeing failures from DependencyScannerTest. I can't find a 
> buildbot failure for this test so I can't cross-reference to see if we have 
> the same issue.
>
> Does this failure look similar to what you saw on Windows, or could it be an 
> option we're adding as part of the Compilation setup?
>
>   [ RUN  ] DependencyScanner.ScanDepsReuseFilemanager
>   .../clang/unittests/Tooling/DependencyScannerTest.cpp:100: Failure
> Expected: Deps[1]
> Which is: "symlink.h"
>   To be equal to: "/root/symlink.h"
>   .../clang/unittests/Tooling/DependencyScannerTest.cpp:101: Failure
> Expected: Deps[2]
> Which is: "header.h"
>   To be equal to: "/root/header.h"
>   .../clang/unittests/Tooling/DependencyScannerTest.cpp:113: Failure
> Expected: Deps[1]
> Which is: "symlink.h"
>   To be equal to: "/root/symlink.h"
>   .../clang/unittests/Tooling/DependencyScannerTest.cpp:114: Failure
> Expected: Deps[2]
> Which is: "header.h"
>   To be equal to: "/root/header.h"
>   [  FAILED  ] DependencyScanner.ScanDepsReuseFilemanager (5 ms)


No the windows test failure was different, there were no Deps at all. I'm 
currently investigating it on a windows VM.

@JamesNagurne I think there's some issue with the working directory, which is 
not added in your case. Which platform are you running your build/test on? 
Which cmake options are you using?


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65907/new/

https://reviews.llvm.org/D65907



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65907: Introduce FileEntryRef and use it when handling includes to report correct dependencies when the FileManager is reused across invocations

2019-08-23 Thread James Nagurne via Phabricator via cfe-commits

JamesNagurne added a comment.
Herald added a subscriber: ributzka.

@arphaman you disabled this test on Windows, but did not specify exactly how it 
fails.
My team works on an embedded ARM compiler (most similar to arm-none-eabi), and 
we're now seeing failures from DependencyScannerTest. I can't find a buildbot 
failure for this test so I can't cross-reference to see if we have the same 
issue.

Does this failure look similar to what you saw on Windows, or could it be an 
option we're adding as part of the Compilation setup?

  [ RUN  ] DependencyScanner.ScanDepsReuseFilemanager
  .../clang/unittests/Tooling/DependencyScannerTest.cpp:100: Failure
Expected: Deps[1]
Which is: "symlink.h"
  To be equal to: "/root/symlink.h"
  .../clang/unittests/Tooling/DependencyScannerTest.cpp:101: Failure
Expected: Deps[2]
Which is: "header.h"
  To be equal to: "/root/header.h"
  .../clang/unittests/Tooling/DependencyScannerTest.cpp:113: Failure
Expected: Deps[1]
Which is: "symlink.h"
  To be equal to: "/root/symlink.h"
  .../clang/unittests/Tooling/DependencyScannerTest.cpp:114: Failure
Expected: Deps[2]
Which is: "header.h"
  To be equal to: "/root/header.h"
  [  FAILED  ] DependencyScanner.ScanDepsReuseFilemanager (5 ms)


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65907/new/

https://reviews.llvm.org/D65907



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65907: Introduce FileEntryRef and use it when handling includes to report correct dependencies when the FileManager is reused across invocations

2019-08-22 Thread Alex Lorenz via Phabricator via cfe-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rL369680: Introduce FileEntryRef and use it when handling 
includes to report correct… (authored by arphaman, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D65907?vs=215719=216676#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65907/new/

https://reviews.llvm.org/D65907

Files:
  cfe/trunk/include/clang/Basic/FileManager.h
  cfe/trunk/include/clang/Basic/SourceManager.h
  cfe/trunk/include/clang/Lex/DirectoryLookup.h
  cfe/trunk/include/clang/Lex/HeaderMap.h
  cfe/trunk/include/clang/Lex/HeaderSearch.h
  cfe/trunk/include/clang/Lex/Preprocessor.h
  cfe/trunk/lib/Basic/FileManager.cpp
  cfe/trunk/lib/Basic/SourceManager.cpp
  cfe/trunk/lib/Frontend/CompilerInstance.cpp
  cfe/trunk/lib/Frontend/DependencyFile.cpp
  cfe/trunk/lib/Frontend/FrontendActions.cpp
  cfe/trunk/lib/Frontend/Rewrite/InclusionRewriter.cpp
  cfe/trunk/lib/Frontend/VerifyDiagnosticConsumer.cpp
  cfe/trunk/lib/Lex/HeaderMap.cpp
  cfe/trunk/lib/Lex/HeaderSearch.cpp
  cfe/trunk/lib/Lex/PPDirectives.cpp
  cfe/trunk/lib/Lex/PPMacroExpansion.cpp
  cfe/trunk/lib/Lex/Pragma.cpp
  cfe/trunk/lib/Lex/Preprocessor.cpp
  cfe/trunk/lib/Serialization/ASTReader.cpp
  cfe/trunk/lib/Serialization/ASTWriter.cpp
  cfe/trunk/test/VFS/external-names.c
  cfe/trunk/unittests/Tooling/CMakeLists.txt
  cfe/trunk/unittests/Tooling/DependencyScannerTest.cpp

Index: cfe/trunk/lib/Lex/Preprocessor.cpp
===
--- cfe/trunk/lib/Lex/Preprocessor.cpp
+++ cfe/trunk/lib/Lex/Preprocessor.cpp
@@ -563,7 +563,7 @@
 // Lookup and save the FileID for the through header. If it isn't found
 // in the search path, it's a fatal error.
 const DirectoryLookup *CurDir;
-const FileEntry *File = LookupFile(
+Optional File = LookupFile(
 SourceLocation(), PPOpts->PCHThroughHeader,
 /*isAngled=*/false, /*FromDir=*/nullptr, /*FromFile=*/nullptr, CurDir,
 /*SearchPath=*/nullptr, /*RelativePath=*/nullptr,
@@ -575,7 +575,7 @@
   return;
 }
 setPCHThroughHeaderFileID(
-SourceMgr.createFileID(File, SourceLocation(), SrcMgr::C_User));
+SourceMgr.createFileID(*File, SourceLocation(), SrcMgr::C_User));
   }
 
   // Skip tokens from the Predefines and if needed the main file.
Index: cfe/trunk/lib/Lex/HeaderMap.cpp
===
--- cfe/trunk/lib/Lex/HeaderMap.cpp
+++ cfe/trunk/lib/Lex/HeaderMap.cpp
@@ -196,17 +196,17 @@
 
 /// LookupFile - Check to see if the specified relative filename is located in
 /// this HeaderMap.  If so, open it and return its FileEntry.
-const FileEntry *HeaderMap::LookupFile(
-StringRef Filename, FileManager ) const {
+Optional HeaderMap::LookupFile(StringRef Filename,
+ FileManager ) const {
 
   SmallString<1024> Path;
   StringRef Dest = HeaderMapImpl::lookupFilename(Filename, Path);
   if (Dest.empty())
-return nullptr;
+return None;
 
-  if (auto File = FM.getFile(Dest))
+  if (auto File = FM.getFileRef(Dest))
 return *File;
-  return nullptr;
+  return None;
 }
 
 StringRef HeaderMapImpl::lookupFilename(StringRef Filename,
Index: cfe/trunk/lib/Lex/PPMacroExpansion.cpp
===
--- cfe/trunk/lib/Lex/PPMacroExpansion.cpp
+++ cfe/trunk/lib/Lex/PPMacroExpansion.cpp
@@ -1210,19 +1210,21 @@
 
   // Search include directories.
   const DirectoryLookup *CurDir;
-  const FileEntry *File =
+  Optional File =
   PP.LookupFile(FilenameLoc, Filename, isAngled, LookupFrom, LookupFromFile,
 CurDir, nullptr, nullptr, nullptr, nullptr, nullptr);
 
   if (PPCallbacks *Callbacks = PP.getPPCallbacks()) {
 SrcMgr::CharacteristicKind FileType = SrcMgr::C_User;
 if (File)
-  FileType = PP.getHeaderSearchInfo().getFileDirFlavor(File);
-Callbacks->HasInclude(FilenameLoc, Filename, isAngled, File, FileType);
+  FileType =
+  PP.getHeaderSearchInfo().getFileDirFlavor(>getFileEntry());
+Callbacks->HasInclude(FilenameLoc, Filename, isAngled,
+  File ? >getFileEntry() : nullptr, FileType);
   }
 
   // Get the result value.  A result of true means the file exists.
-  return File != nullptr;
+  return File.hasValue();
 }
 
 /// EvaluateHasInclude - Process a '__has_include("path")' expression.
Index: cfe/trunk/lib/Lex/HeaderSearch.cpp
===
--- cfe/trunk/lib/Lex/HeaderSearch.cpp
+++ cfe/trunk/lib/Lex/HeaderSearch.cpp
@@ -304,14 +304,13 @@
   return getHeaderMap()->getFileName();
 }
 
-const FileEntry *HeaderSearch::getFileAndSuggestModule(
+Optional HeaderSearch::getFileAndSuggestModule(
 StringRef FileName, SourceLocation IncludeLoc,

[PATCH] D65907: Introduce FileEntryRef and use it when handling includes to report correct dependencies when the FileManager is reused across invocations

2019-08-16 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman updated this revision to Diff 215719.
arphaman added a comment.

remove accidentally include diff's `.rej` file.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65907/new/

https://reviews.llvm.org/D65907

Files:
  clang/include/clang/Basic/FileManager.h
  clang/include/clang/Basic/SourceManager.h
  clang/include/clang/Lex/DirectoryLookup.h
  clang/include/clang/Lex/HeaderMap.h
  clang/include/clang/Lex/HeaderSearch.h
  clang/include/clang/Lex/Preprocessor.h
  clang/lib/Basic/FileManager.cpp
  clang/lib/Basic/SourceManager.cpp
  clang/lib/Frontend/CompilerInstance.cpp
  clang/lib/Frontend/DependencyFile.cpp
  clang/lib/Frontend/FrontendActions.cpp
  clang/lib/Frontend/Rewrite/InclusionRewriter.cpp
  clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
  clang/lib/Lex/HeaderMap.cpp
  clang/lib/Lex/HeaderSearch.cpp
  clang/lib/Lex/PPDirectives.cpp
  clang/lib/Lex/PPMacroExpansion.cpp
  clang/lib/Lex/Pragma.cpp
  clang/lib/Lex/Preprocessor.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/VFS/external-names.c
  clang/unittests/Tooling/CMakeLists.txt
  clang/unittests/Tooling/DependencyScannerTest.cpp

Index: clang/unittests/Tooling/DependencyScannerTest.cpp
===
--- /dev/null
+++ clang/unittests/Tooling/DependencyScannerTest.cpp
@@ -0,0 +1,118 @@
+//===- unittest/Tooling/ToolingTest.cpp - Tooling unit tests --===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/AST/ASTConsumer.h"
+#include "clang/AST/DeclCXX.h"
+#include "clang/AST/DeclGroup.h"
+#include "clang/Frontend/ASTUnit.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Frontend/FrontendAction.h"
+#include "clang/Frontend/FrontendActions.h"
+#include "clang/Tooling/CompilationDatabase.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/TargetRegistry.h"
+#include "llvm/Support/TargetSelect.h"
+#include "gtest/gtest.h"
+#include 
+#include 
+
+namespace clang {
+namespace tooling {
+
+namespace {
+
+/// Prints out all of the gathered dependencies into a string.
+class TestFileCollector : public DependencyFileGenerator {
+public:
+  TestFileCollector(DependencyOutputOptions ,
+std::vector )
+  : DependencyFileGenerator(Opts), Deps(Deps) {}
+
+  void finishedMainFile(DiagnosticsEngine ) override {
+Deps = getDependencies();
+  }
+
+private:
+  std::vector 
+};
+
+class TestDependencyScanningAction : public tooling::ToolAction {
+public:
+  TestDependencyScanningAction(std::vector ) : Deps(Deps) {}
+
+  bool runInvocation(std::shared_ptr Invocation,
+ FileManager *FileMgr,
+ std::shared_ptr PCHContainerOps,
+ DiagnosticConsumer *DiagConsumer) override {
+CompilerInstance Compiler(std::move(PCHContainerOps));
+Compiler.setInvocation(std::move(Invocation));
+Compiler.setFileManager(FileMgr);
+
+Compiler.createDiagnostics(DiagConsumer, /*ShouldOwnClient=*/false);
+if (!Compiler.hasDiagnostics())
+  return false;
+
+Compiler.createSourceManager(*FileMgr);
+Compiler.addDependencyCollector(std::make_shared(
+Compiler.getInvocation().getDependencyOutputOpts(), Deps));
+
+auto Action = std::make_unique();
+return Compiler.ExecuteAction(*Action);
+  }
+
+private:
+  std::vector 
+};
+
+} // namespace
+
+TEST(DependencyScanner, ScanDepsReuseFilemanager) {
+  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);
+  VFS->addFile("/root/header.h", 0, llvm::MemoryBuffer::getMemBuffer("\n"));
+  VFS->addHardLink("/root/symlink.h", "/root/header.h");
+  VFS->addFile("/root/test.cpp", 0,
+   llvm::MemoryBuffer::getMemBuffer(
+   "#include \"symlink.h\"\n#include \"header.h\"\n"));
+
+  ClangTool Tool(CDB, {"test.cpp"}, std::make_shared(),
+ VFS);
+  Tool.clearArgumentsAdjusters();
+  std::vector Deps;
+  TestDependencyScanningAction Action(Deps);
+  Tool.run();
+  // The first invocation should return dependencies in order of access.
+  ASSERT_EQ(Deps.size(), 3u);
+  EXPECT_EQ(Deps[0], "/root/test.cpp");
+  EXPECT_EQ(Deps[1], "/root/symlink.h");
+  EXPECT_EQ(Deps[2], "/root/header.h");
+
+  // The file manager should still have two FileEntries, as one file is a
+  // hardlink.
+  FileManager  = Tool.getFiles();
+  EXPECT_EQ(Files.getNumUniqueRealFiles(), 2u);
+
+  Deps.clear();
+  Tool.run();
+  // The second invocation should have the

[PATCH] D65907: Introduce FileEntryRef and use it when handling includes to report correct dependencies when the FileManager is reused across invocations

2019-08-16 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman updated this revision to Diff 215718.
arphaman marked 8 inline comments as done.
arphaman added a comment.

Address review comments and proper `use-external-names` support.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65907/new/

https://reviews.llvm.org/D65907

Files:
  clang/include/clang/Basic/FileManager.h
  clang/include/clang/Basic/SourceManager.h
  clang/include/clang/Lex/DirectoryLookup.h
  clang/include/clang/Lex/HeaderMap.h
  clang/include/clang/Lex/HeaderSearch.h
  clang/include/clang/Lex/Preprocessor.h
  clang/lib/Basic/FileManager.cpp
  clang/lib/Basic/SourceManager.cpp
  clang/lib/Frontend/CompilerInstance.cpp
  clang/lib/Frontend/DependencyFile.cpp
  clang/lib/Frontend/FrontendActions.cpp
  clang/lib/Frontend/Rewrite/InclusionRewriter.cpp
  clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
  clang/lib/Lex/HeaderMap.cpp
  clang/lib/Lex/HeaderSearch.cpp
  clang/lib/Lex/HeaderSearch.cpp.rej
  clang/lib/Lex/PPDirectives.cpp
  clang/lib/Lex/PPMacroExpansion.cpp
  clang/lib/Lex/Pragma.cpp
  clang/lib/Lex/Preprocessor.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/VFS/external-names.c
  clang/unittests/Tooling/CMakeLists.txt
  clang/unittests/Tooling/DependencyScannerTest.cpp

Index: clang/unittests/Tooling/DependencyScannerTest.cpp
===
--- /dev/null
+++ clang/unittests/Tooling/DependencyScannerTest.cpp
@@ -0,0 +1,118 @@
+//===- unittest/Tooling/ToolingTest.cpp - Tooling unit tests --===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/AST/ASTConsumer.h"
+#include "clang/AST/DeclCXX.h"
+#include "clang/AST/DeclGroup.h"
+#include "clang/Frontend/ASTUnit.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Frontend/FrontendAction.h"
+#include "clang/Frontend/FrontendActions.h"
+#include "clang/Tooling/CompilationDatabase.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/TargetRegistry.h"
+#include "llvm/Support/TargetSelect.h"
+#include "gtest/gtest.h"
+#include 
+#include 
+
+namespace clang {
+namespace tooling {
+
+namespace {
+
+/// Prints out all of the gathered dependencies into a string.
+class TestFileCollector : public DependencyFileGenerator {
+public:
+  TestFileCollector(DependencyOutputOptions ,
+std::vector )
+  : DependencyFileGenerator(Opts), Deps(Deps) {}
+
+  void finishedMainFile(DiagnosticsEngine ) override {
+Deps = getDependencies();
+  }
+
+private:
+  std::vector 
+};
+
+class TestDependencyScanningAction : public tooling::ToolAction {
+public:
+  TestDependencyScanningAction(std::vector ) : Deps(Deps) {}
+
+  bool runInvocation(std::shared_ptr Invocation,
+ FileManager *FileMgr,
+ std::shared_ptr PCHContainerOps,
+ DiagnosticConsumer *DiagConsumer) override {
+CompilerInstance Compiler(std::move(PCHContainerOps));
+Compiler.setInvocation(std::move(Invocation));
+Compiler.setFileManager(FileMgr);
+
+Compiler.createDiagnostics(DiagConsumer, /*ShouldOwnClient=*/false);
+if (!Compiler.hasDiagnostics())
+  return false;
+
+Compiler.createSourceManager(*FileMgr);
+Compiler.addDependencyCollector(std::make_shared(
+Compiler.getInvocation().getDependencyOutputOpts(), Deps));
+
+auto Action = std::make_unique();
+return Compiler.ExecuteAction(*Action);
+  }
+
+private:
+  std::vector 
+};
+
+} // namespace
+
+TEST(DependencyScanner, ScanDepsReuseFilemanager) {
+  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);
+  VFS->addFile("/root/header.h", 0, llvm::MemoryBuffer::getMemBuffer("\n"));
+  VFS->addHardLink("/root/symlink.h", "/root/header.h");
+  VFS->addFile("/root/test.cpp", 0,
+   llvm::MemoryBuffer::getMemBuffer(
+   "#include \"symlink.h\"\n#include \"header.h\"\n"));
+
+  ClangTool Tool(CDB, {"test.cpp"}, std::make_shared(),
+ VFS);
+  Tool.clearArgumentsAdjusters();
+  std::vector Deps;
+  TestDependencyScanningAction Action(Deps);
+  Tool.run();
+  // The first invocation should return dependencies in order of access.
+  ASSERT_EQ(Deps.size(), 3u);
+  EXPECT_EQ(Deps[0], "/root/test.cpp");
+  EXPECT_EQ(Deps[1], "/root/symlink.h");
+  EXPECT_EQ(Deps[2], "/root/header.h");
+
+  // The file manager should still have two FileEntries, as one file is a
+  // hardlink.
+  FileManager  = Tool.getFiles();
+

[PATCH] D65907: Introduce FileEntryRef and use it when handling includes to report correct dependencies when the FileManager is reused across invocations

2019-08-16 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman added inline comments.



Comment at: clang/include/clang/Basic/FileManager.h:110
+/// A reference to a \c FileEntry that includes the name of the file as it was
+/// accessed by the FileManager's client.
+class FileEntryRef {

bruno wrote:
> How does that work with the VFS? Will it keep the virtual path as the 
> FileName to return with getName() or the external-content? Should it change 
> when in face of `use-external-names` attribute?
Good catch. In the first patch `getFileRef` didn't honor `use-external-names` 
correctly (specifically once a file was opened once, for subsequent open of 
that file, it returned the VFS name, not the external name). 

I fixed this issue by introducing a potential indirection to `SeenFileEntries` 
in the update patch. Now the `getFileRef` should always return the virtual 
path, unless `use-external-names` is specified, and then it will return the 
external path.



Comment at: clang/include/clang/Basic/FileManager.h:130
+
+  const DirectoryEntry *getDir() const { return Entry.getDir(); }
+

Bigcheese wrote:
> Isn't this incorrect in the case of symlinks?
Right, I was planning to provide another way to get a directory with name. For 
now I'll just remove this method, so clients will have to call into FileEntry.



Comment at: clang/include/clang/Basic/FileManager.h:249-251
+  /// This function is deprecated and will be removed at some point in the
+  /// future, new clients should use
+  ///  \c getFileRef.

bruno wrote:
> Bigcheese wrote:
> > `LLVM_DEPRECATED()`?  (or w/e the name is of our depreciation attribute 
> > macro).
> Probably can't be done until we move all uses over? There's probably still 
> enough of them that the warning would be very annoying?
That's right, I can't mark the function as deprecated just yet, as you'll get a 
lot of warnings when building clang.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65907/new/

https://reviews.llvm.org/D65907



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65907: Introduce FileEntryRef and use it when handling includes to report correct dependencies when the FileManager is reused across invocations

2019-08-16 Thread Bruno Cardoso Lopes via Phabricator via cfe-commits

bruno added inline comments.



Comment at: clang/include/clang/Basic/FileManager.h:110
+/// A reference to a \c FileEntry that includes the name of the file as it was
+/// accessed by the FileManager's client.
+class FileEntryRef {

How does that work with the VFS? Will it keep the virtual path as the FileName 
to return with getName() or the external-content? Should it change when in face 
of `use-external-names` attribute?



Comment at: clang/include/clang/Basic/FileManager.h:249-251
+  /// This function is deprecated and will be removed at some point in the
+  /// future, new clients should use
+  ///  \c getFileRef.

Bigcheese wrote:
> `LLVM_DEPRECATED()`?  (or w/e the name is of our depreciation attribute 
> macro).
Probably can't be done until we move all uses over? There's probably still 
enough of them that the warning would be very annoying?



Comment at: clang/lib/Basic/FileManager.cpp:194
+static llvm::ErrorOr
+promoteFileRef(StringRef name, llvm::ErrorOr value) {
+  if (value)

I know the surrounding functions are not consistent with capitalization, but 
maybe `name` and `value` here should?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65907/new/

https://reviews.llvm.org/D65907



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65907: Introduce FileEntryRef and use it when handling includes to report correct dependencies when the FileManager is reused across invocations

2019-08-15 Thread Michael Spencer via Phabricator via cfe-commits

Bigcheese requested changes to this revision.
Bigcheese added inline comments.
This revision now requires changes to proceed.



Comment at: clang/include/clang/Basic/SourceManager.h:1024
+  Optional getFileEntryRefForID(FileID FID) const {
+bool MyInvalid = false;
+const SrcMgr::SLocEntry  = getSLocEntry(FID, );

`Invalid`?  I realize this is copied from above, but i'd fix the name.



Comment at: clang/lib/Lex/PPDirectives.cpp:1757
 
-  if (!File) {
+  auto LookupHeaderFile = [&]() -> Optional {
+Optional File = LookupFile(

This lambda is huge and I only see it used once.  Should this be a separate 
function?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65907/new/

https://reviews.llvm.org/D65907



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65907: Introduce FileEntryRef and use it when handling includes to report correct dependencies when the FileManager is reused across invocations

2019-08-14 Thread Michael Spencer via Phabricator via cfe-commits

Bigcheese added inline comments.



Comment at: clang/include/clang/Basic/FileManager.h:130
+
+  const DirectoryEntry *getDir() const { return Entry.getDir(); }
+

Isn't this incorrect in the case of symlinks?



Comment at: clang/include/clang/Basic/FileManager.h:249-251
+  /// This function is deprecated and will be removed at some point in the
+  /// future, new clients should use
+  ///  \c getFileRef.

`LLVM_DEPRECATED()`?  (or w/e the name is of our depreciation attribute macro).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65907/new/

https://reviews.llvm.org/D65907



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65907: Introduce FileEntryRef and use it when handling includes to report correct dependencies when the FileManager is reused across invocations

2019-08-07 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman updated this revision to Diff 214015.
arphaman added a comment.

Fix the FIXME.
The patch is now ready for review.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65907/new/

https://reviews.llvm.org/D65907

Files:
  clang/include/clang/Basic/FileManager.h
  clang/include/clang/Basic/SourceManager.h
  clang/include/clang/Lex/DirectoryLookup.h
  clang/include/clang/Lex/HeaderMap.h
  clang/include/clang/Lex/HeaderSearch.h
  clang/include/clang/Lex/Preprocessor.h
  clang/lib/Basic/FileManager.cpp
  clang/lib/Basic/SourceManager.cpp
  clang/lib/Frontend/CompilerInstance.cpp
  clang/lib/Frontend/DependencyFile.cpp
  clang/lib/Frontend/FrontendActions.cpp
  clang/lib/Frontend/Rewrite/InclusionRewriter.cpp
  clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
  clang/lib/Lex/HeaderMap.cpp
  clang/lib/Lex/HeaderSearch.cpp
  clang/lib/Lex/PPDirectives.cpp
  clang/lib/Lex/PPMacroExpansion.cpp
  clang/lib/Lex/Pragma.cpp
  clang/lib/Lex/Preprocessor.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/unittests/Tooling/CMakeLists.txt
  clang/unittests/Tooling/DependencyScannerTest.cpp

Index: clang/unittests/Tooling/DependencyScannerTest.cpp
===
--- /dev/null
+++ clang/unittests/Tooling/DependencyScannerTest.cpp
@@ -0,0 +1,118 @@
+//===- unittest/Tooling/ToolingTest.cpp - Tooling unit tests --===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/AST/ASTConsumer.h"
+#include "clang/AST/DeclCXX.h"
+#include "clang/AST/DeclGroup.h"
+#include "clang/Frontend/ASTUnit.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Frontend/FrontendAction.h"
+#include "clang/Frontend/FrontendActions.h"
+#include "clang/Tooling/CompilationDatabase.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/TargetRegistry.h"
+#include "llvm/Support/TargetSelect.h"
+#include "gtest/gtest.h"
+#include 
+#include 
+
+namespace clang {
+namespace tooling {
+
+namespace {
+
+/// Prints out all of the gathered dependencies into a string.
+class TestFileCollector : public DependencyFileGenerator {
+public:
+  TestFileCollector(DependencyOutputOptions ,
+std::vector )
+  : DependencyFileGenerator(Opts), Deps(Deps) {}
+
+  void finishedMainFile(DiagnosticsEngine ) override {
+Deps = getDependencies();
+  }
+
+private:
+  std::vector 
+};
+
+class TestDependencyScanningAction : public tooling::ToolAction {
+public:
+  TestDependencyScanningAction(std::vector ) : Deps(Deps) {}
+
+  bool runInvocation(std::shared_ptr Invocation,
+ FileManager *FileMgr,
+ std::shared_ptr PCHContainerOps,
+ DiagnosticConsumer *DiagConsumer) override {
+CompilerInstance Compiler(std::move(PCHContainerOps));
+Compiler.setInvocation(std::move(Invocation));
+Compiler.setFileManager(FileMgr);
+
+Compiler.createDiagnostics(DiagConsumer, /*ShouldOwnClient=*/false);
+if (!Compiler.hasDiagnostics())
+  return false;
+
+Compiler.createSourceManager(*FileMgr);
+Compiler.addDependencyCollector(std::make_shared(
+Compiler.getInvocation().getDependencyOutputOpts(), Deps));
+
+auto Action = llvm::make_unique();
+return Compiler.ExecuteAction(*Action);
+  }
+
+private:
+  std::vector 
+};
+
+} // namespace
+
+TEST(DependencyScanner, ScanDepsReuseFilemanager) {
+  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);
+  VFS->addFile("/root/header.h", 0, llvm::MemoryBuffer::getMemBuffer("\n"));
+  VFS->addHardLink("/root/symlink.h", "/root/header.h");
+  VFS->addFile("/root/test.cpp", 0,
+   llvm::MemoryBuffer::getMemBuffer(
+   "#include \"symlink.h\"\n#include \"header.h\"\n"));
+
+  ClangTool Tool(CDB, {"test.cpp"}, std::make_shared(),
+ VFS);
+  Tool.clearArgumentsAdjusters();
+  std::vector Deps;
+  TestDependencyScanningAction Action(Deps);
+  Tool.run();
+  // The first invocation should return dependencies in order of access.
+  ASSERT_EQ(Deps.size(), 3u);
+  EXPECT_EQ(Deps[0], "/root/test.cpp");
+  EXPECT_EQ(Deps[1], "/root/symlink.h");
+  EXPECT_EQ(Deps[2], "/root/header.h");
+
+  // The file manager should still have two FileEntries, as one file is a
+  // hardlink.
+  FileManager  = Tool.getFiles();
+  EXPECT_EQ(Files.getNumUniqueRealFiles(), 2u);
+
+  Deps.clear();
+  Tool.run();
+  // The second invocation should have the same order of dependencies.
+

[PATCH] D65907: Introduce FileEntryRef and use it when handling includes to report correct dependencies when the FileManager is reused across invocations

2019-08-07 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman planned changes to this revision.
arphaman added a comment.

I forgot I left a FIXME that I intended to fix in there, I need to resolve that 
first. I'll update the patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65907/new/

https://reviews.llvm.org/D65907



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65907: Introduce FileEntryRef and use it when handling includes to report correct dependencies when the FileManager is reused across invocations

2019-08-07 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman updated this revision to Diff 214010.
arphaman added a comment.

Fix a bug with dereferencing `None` File value in one call.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65907/new/

https://reviews.llvm.org/D65907

Files:
  clang/include/clang/Basic/FileManager.h
  clang/include/clang/Basic/SourceManager.h
  clang/include/clang/Lex/DirectoryLookup.h
  clang/include/clang/Lex/HeaderSearch.h
  clang/include/clang/Lex/Preprocessor.h
  clang/lib/Basic/FileManager.cpp
  clang/lib/Basic/SourceManager.cpp
  clang/lib/Frontend/CompilerInstance.cpp
  clang/lib/Frontend/DependencyFile.cpp
  clang/lib/Frontend/FrontendActions.cpp
  clang/lib/Frontend/Rewrite/InclusionRewriter.cpp
  clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
  clang/lib/Lex/HeaderSearch.cpp
  clang/lib/Lex/PPDirectives.cpp
  clang/lib/Lex/PPMacroExpansion.cpp
  clang/lib/Lex/Pragma.cpp
  clang/lib/Lex/Preprocessor.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/unittests/Tooling/CMakeLists.txt
  clang/unittests/Tooling/DependencyScannerTest.cpp

Index: clang/unittests/Tooling/DependencyScannerTest.cpp
===
--- /dev/null
+++ clang/unittests/Tooling/DependencyScannerTest.cpp
@@ -0,0 +1,118 @@
+//===- unittest/Tooling/ToolingTest.cpp - Tooling unit tests --===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/AST/ASTConsumer.h"
+#include "clang/AST/DeclCXX.h"
+#include "clang/AST/DeclGroup.h"
+#include "clang/Frontend/ASTUnit.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Frontend/FrontendAction.h"
+#include "clang/Frontend/FrontendActions.h"
+#include "clang/Tooling/CompilationDatabase.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/TargetRegistry.h"
+#include "llvm/Support/TargetSelect.h"
+#include "gtest/gtest.h"
+#include 
+#include 
+
+namespace clang {
+namespace tooling {
+
+namespace {
+
+/// Prints out all of the gathered dependencies into a string.
+class TestFileCollector : public DependencyFileGenerator {
+public:
+  TestFileCollector(DependencyOutputOptions ,
+std::vector )
+  : DependencyFileGenerator(Opts), Deps(Deps) {}
+
+  void finishedMainFile(DiagnosticsEngine ) override {
+Deps = getDependencies();
+  }
+
+private:
+  std::vector 
+};
+
+class TestDependencyScanningAction : public tooling::ToolAction {
+public:
+  TestDependencyScanningAction(std::vector ) : Deps(Deps) {}
+
+  bool runInvocation(std::shared_ptr Invocation,
+ FileManager *FileMgr,
+ std::shared_ptr PCHContainerOps,
+ DiagnosticConsumer *DiagConsumer) override {
+CompilerInstance Compiler(std::move(PCHContainerOps));
+Compiler.setInvocation(std::move(Invocation));
+Compiler.setFileManager(FileMgr);
+
+Compiler.createDiagnostics(DiagConsumer, /*ShouldOwnClient=*/false);
+if (!Compiler.hasDiagnostics())
+  return false;
+
+Compiler.createSourceManager(*FileMgr);
+Compiler.addDependencyCollector(std::make_shared(
+Compiler.getInvocation().getDependencyOutputOpts(), Deps));
+
+auto Action = llvm::make_unique();
+return Compiler.ExecuteAction(*Action);
+  }
+
+private:
+  std::vector 
+};
+
+} // namespace
+
+TEST(DependencyScanner, ScanDepsReuseFilemanager) {
+  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);
+  VFS->addFile("/root/header.h", 0, llvm::MemoryBuffer::getMemBuffer("\n"));
+  VFS->addHardLink("/root/symlink.h", "/root/header.h");
+  VFS->addFile("/root/test.cpp", 0,
+   llvm::MemoryBuffer::getMemBuffer(
+   "#include \"symlink.h\"\n#include \"header.h\"\n"));
+
+  ClangTool Tool(CDB, {"test.cpp"}, std::make_shared(),
+ VFS);
+  Tool.clearArgumentsAdjusters();
+  std::vector Deps;
+  TestDependencyScanningAction Action(Deps);
+  Tool.run();
+  // The first invocation should return dependencies in order of access.
+  ASSERT_EQ(Deps.size(), 3u);
+  EXPECT_EQ(Deps[0], "/root/test.cpp");
+  EXPECT_EQ(Deps[1], "/root/symlink.h");
+  EXPECT_EQ(Deps[2], "/root/header.h");
+
+  // The file manager should still have two FileEntries, as one file is a
+  // hardlink.
+  FileManager  = Tool.getFiles();
+  EXPECT_EQ(Files.getNumUniqueRealFiles(), 2u);
+
+  Deps.clear();
+  Tool.run();
+  // The second invocation should have the same order of dependencies.
+

[PATCH] D65907: Introduce FileEntryRef and use it when handling includes to report correct dependencies when the FileManager is reused across invocations

2019-08-07 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman created this revision.
arphaman added reviewers: rsmith, bruno, Bigcheese, jkorous, harlanhaskins.
Herald added subscribers: dexonsmith, mgorny.
Herald added a project: clang.

This patch introduces a parallel API to FileManager's `getFile`: 
`getFileEntryRef`, which returns a reference to the `FileEntry`, and the name 
that was used to access the file.

The new API is adopted only in the HeaderSearch and Preprocessor for include 
file lookup, so that the accessed path can be propagated to SourceManager's 
`FileInfo`. SourceManager's `FileInfo` now can report this accessed path, using 
the new `getName` method. This API is then adopted in the dependency collector, 
which now correctly reports dependencies when a file is included both using a 
symlink and a real path in the case when the FileManager is reused across 
multiple Preprocessor invocations.

Note that this patch does not fix all dependency collector issues, as the same 
problem is still present in other cases when dependencies are obtained using 
`FileSkipped`, `InclusionDirective`, and `HasInclude`. This will be fixed in a 
follow-up patch.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D65907

Files:
  clang/include/clang/Basic/FileManager.h
  clang/include/clang/Basic/SourceManager.h
  clang/include/clang/Lex/DirectoryLookup.h
  clang/include/clang/Lex/HeaderSearch.h
  clang/include/clang/Lex/Preprocessor.h
  clang/lib/Basic/FileManager.cpp
  clang/lib/Basic/SourceManager.cpp
  clang/lib/Frontend/CompilerInstance.cpp
  clang/lib/Frontend/DependencyFile.cpp
  clang/lib/Frontend/FrontendActions.cpp
  clang/lib/Frontend/Rewrite/InclusionRewriter.cpp
  clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
  clang/lib/Lex/HeaderSearch.cpp
  clang/lib/Lex/PPDirectives.cpp
  clang/lib/Lex/PPMacroExpansion.cpp
  clang/lib/Lex/Pragma.cpp
  clang/lib/Lex/Preprocessor.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/unittests/Tooling/CMakeLists.txt
  clang/unittests/Tooling/DependencyScannerTest.cpp

Index: clang/unittests/Tooling/DependencyScannerTest.cpp
===
--- /dev/null
+++ clang/unittests/Tooling/DependencyScannerTest.cpp
@@ -0,0 +1,118 @@
+//===- unittest/Tooling/ToolingTest.cpp - Tooling unit tests --===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/AST/ASTConsumer.h"
+#include "clang/AST/DeclCXX.h"
+#include "clang/AST/DeclGroup.h"
+#include "clang/Frontend/ASTUnit.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Frontend/FrontendAction.h"
+#include "clang/Frontend/FrontendActions.h"
+#include "clang/Tooling/CompilationDatabase.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/TargetRegistry.h"
+#include "llvm/Support/TargetSelect.h"
+#include "gtest/gtest.h"
+#include 
+#include 
+
+namespace clang {
+namespace tooling {
+
+namespace {
+
+/// Prints out all of the gathered dependencies into a string.
+class TestFileCollector : public DependencyFileGenerator {
+public:
+  TestFileCollector(DependencyOutputOptions ,
+std::vector )
+  : DependencyFileGenerator(Opts), Deps(Deps) {}
+
+  void finishedMainFile(DiagnosticsEngine ) override {
+Deps = getDependencies();
+  }
+
+private:
+  std::vector 
+};
+
+class TestDependencyScanningAction : public tooling::ToolAction {
+public:
+  TestDependencyScanningAction(std::vector ) : Deps(Deps) {}
+
+  bool runInvocation(std::shared_ptr Invocation,
+ FileManager *FileMgr,
+ std::shared_ptr PCHContainerOps,
+ DiagnosticConsumer *DiagConsumer) override {
+CompilerInstance Compiler(std::move(PCHContainerOps));
+Compiler.setInvocation(std::move(Invocation));
+Compiler.setFileManager(FileMgr);
+
+Compiler.createDiagnostics(DiagConsumer, /*ShouldOwnClient=*/false);
+if (!Compiler.hasDiagnostics())
+  return false;
+
+Compiler.createSourceManager(*FileMgr);
+Compiler.addDependencyCollector(std::make_shared(
+Compiler.getInvocation().getDependencyOutputOpts(), Deps));
+
+auto Action = llvm::make_unique();
+return Compiler.ExecuteAction(*Action);
+  }
+
+private:
+  std::vector 
+};
+
+} // namespace
+
+TEST(DependencyScanner, ScanDepsReuseFilemanager) {
+  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);
+  VFS->addFile("/root/header.h", 0, llvm::MemoryBuffer::getMemBuffer("\n"));
+

[PATCH] D65907: Introduce FileEntryRef and use it when handling includes to report correct dependencies when the FileManager is reused across invocations

[PATCH] D65907: Introduce FileEntryRef and use it when handling includes to report correct dependencies when the FileManager is reused across invocations

[PATCH] D65907: Introduce FileEntryRef and use it when handling includes to report correct dependencies when the FileManager is reused across invocations

[PATCH] D65907: Introduce FileEntryRef and use it when handling includes to report correct dependencies when the FileManager is reused across invocations

[PATCH] D65907: Introduce FileEntryRef and use it when handling includes to report correct dependencies when the FileManager is reused across invocations

[PATCH] D65907: Introduce FileEntryRef and use it when handling includes to report correct dependencies when the FileManager is reused across invocations

[PATCH] D65907: Introduce FileEntryRef and use it when handling includes to report correct dependencies when the FileManager is reused across invocations

[PATCH] D65907: Introduce FileEntryRef and use it when handling includes to report correct dependencies when the FileManager is reused across invocations

[PATCH] D65907: Introduce FileEntryRef and use it when handling includes to report correct dependencies when the FileManager is reused across invocations

[PATCH] D65907: Introduce FileEntryRef and use it when handling includes to report correct dependencies when the FileManager is reused across invocations

[PATCH] D65907: Introduce FileEntryRef and use it when handling includes to report correct dependencies when the FileManager is reused across invocations

[PATCH] D65907: Introduce FileEntryRef and use it when handling includes to report correct dependencies when the FileManager is reused across invocations

[PATCH] D65907: Introduce FileEntryRef and use it when handling includes to report correct dependencies when the FileManager is reused across invocations

[PATCH] D65907: Introduce FileEntryRef and use it when handling includes to report correct dependencies when the FileManager is reused across invocations

[PATCH] D65907: Introduce FileEntryRef and use it when handling includes to report correct dependencies when the FileManager is reused across invocations

[PATCH] D65907: Introduce FileEntryRef and use it when handling includes to report correct dependencies when the FileManager is reused across invocations

16 matches

Site Navigation

Mail list logo

Footer information