Reapply fixed "Honour 'use-external-names' in FileManager"

Was r202442 There were two issues with the original patch that have now been fixed. 1. We were memset'ing over a FileEntry in a test case. After adding a std::string to FileEntry, this still happened to not break for me. 2. I didn't pass the FileManager into the new compiler instance in compileModule. This was hidden in some cases by the fact I didn't clear the module cache in the test. Also, I changed the copy constructor for FileEntry, which was memcpy'ing in a (now) unsafe way. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@202539 91177308-0d34-0410-b5e6-96231b3b80d8
miguelzf · Feb 28, 2014 · d066fe9 · d066fe9
1 parent d353a18
commit d066fe9
Show file tree

Hide file tree

Showing 13 changed files with 97 additions and 19 deletions.
diff --git a/include/clang/Basic/FileManager.h b/include/clang/Basic/FileManager.h
@@ -59,7 +59,7 @@ class DirectoryEntry {
 /// If the 'File' member is valid, then this FileEntry has an open file
 /// descriptor for the file.
 class FileEntry {
-  const char *Name;           // Name of the file.
+  std::string Name;           // Name of the file.
   off_t Size;                 // File size in bytes.
   time_t ModTime;             // Modification time of file.
   const DirectoryEntry *Dir;  // Directory file lives in.
@@ -81,18 +81,19 @@ class FileEntry {
 
 public:
   FileEntry()
-      : Name(0), UniqueID(0, 0), IsNamedPipe(false), InPCH(false),
-        IsValid(false)
+      : UniqueID(0, 0), IsNamedPipe(false), InPCH(false), IsValid(false)
   {}
 
   // FIXME: this is here to allow putting FileEntry in std::map.  Once we have
   // emplace, we shouldn't need a copy constructor anymore.
-  FileEntry(const FileEntry &FE) {
-    memcpy(this, &FE, sizeof(FE));
+  /// Intentionally does not copy fields that are not set in an uninitialized
+  /// \c FileEntry.
+  FileEntry(const FileEntry &FE) : UniqueID(FE.UniqueID),
+      IsNamedPipe(FE.IsNamedPipe), InPCH(FE.InPCH), IsValid(FE.IsValid) {
     assert(!isValid() && "Cannot copy an initialized FileEntry");
   }
 
-  const char *getName() const { return Name; }
+  const char *getName() const { return Name.c_str(); }
   bool isValid() const { return IsValid; }
   off_t getSize() const { return Size; }
   unsigned getUID() const { return UID; }

diff --git a/include/clang/Basic/FileSystemStatCache.h b/include/clang/Basic/FileSystemStatCache.h
@@ -29,7 +29,9 @@ class File;
 class FileSystem;
 }
 
+// FIXME: should probably replace this with vfs::Status
 struct FileData {
+  std::string Name;
   uint64_t Size;
   time_t ModTime;
   llvm::sys::fs::UniqueID UniqueID;

diff --git a/include/clang/Basic/VirtualFileSystem.h b/include/clang/Basic/VirtualFileSystem.h
@@ -90,6 +90,8 @@ class File {
                                      bool RequiresNullTerminator = true) = 0;
   /// \brief Closes the file.
   virtual llvm::error_code close() = 0;
+  /// \brief Sets the name to use for this file.
+  virtual void setName(StringRef Name) = 0;
 };
 
 /// \brief The virtual file system interface.

diff --git a/lib/Basic/FileManager.cpp b/lib/Basic/FileManager.cpp
@@ -282,7 +282,7 @@ const FileEntry *FileManager::getFile(StringRef Filename, bool openFile,
   }
 
   // Otherwise, we don't have this file yet, add it.
-  UFE.Name    = InterndFileName;
+  UFE.Name    = Data.Name;
   UFE.Size = Data.Size;
   UFE.ModTime = Data.ModTime;
   UFE.Dir     = DirInfo;

diff --git a/lib/Basic/FileSystemStatCache.cpp b/lib/Basic/FileSystemStatCache.cpp
@@ -32,6 +32,7 @@ void FileSystemStatCache::anchor() { }
 
 static void copyStatusToFileData(const vfs::Status &Status,
                                  FileData &Data) {
+  Data.Name = Status.getName();
   Data.Size = Status.getSize();
   Data.ModTime = Status.getLastModificationTime().toEpochTime();
   Data.UniqueID = Status.getUniqueID();

diff --git a/lib/Basic/VirtualFileSystem.cpp b/lib/Basic/VirtualFileSystem.cpp
@@ -83,6 +83,7 @@ error_code FileSystem::getBufferForFile(const llvm::Twine &Name,
 /// \brief Wrapper around a raw file descriptor.
 class RealFile : public File {
   int FD;
+  Status S;
   friend class RealFileSystem;
   RealFile(int FD) : FD(FD) {
     assert(FD >= 0 && "Invalid or inactive file descriptor");
@@ -95,15 +96,21 @@ class RealFile : public File {
                        int64_t FileSize = -1,
                        bool RequiresNullTerminator = true) LLVM_OVERRIDE;
   error_code close() LLVM_OVERRIDE;
+  void setName(StringRef Name) LLVM_OVERRIDE;
 };
 RealFile::~RealFile() { close(); }
 
 ErrorOr<Status> RealFile::status() {
   assert(FD != -1 && "cannot stat closed file");
-  file_status RealStatus;
-  if (error_code EC = sys::fs::status(FD, RealStatus))
-    return EC;
-  return Status(RealStatus);
+  if (!S.isStatusKnown()) {
+    file_status RealStatus;
+    if (error_code EC = sys::fs::status(FD, RealStatus))
+      return EC;
+    Status NewS(RealStatus);
+    NewS.setName(S.getName());
+    S = llvm_move(NewS);
+  }
+  return S;
 }
 
 error_code RealFile::getBuffer(const Twine &Name,
@@ -131,6 +138,10 @@ error_code RealFile::close() {
   return error_code::success();
 }
 
+void RealFile::setName(StringRef Name) {
+  S.setName(Name);
+}
+
 /// \brief The file system according to your operating system.
 class RealFileSystem : public FileSystem {
 public:
@@ -154,6 +165,7 @@ error_code RealFileSystem::openFileForRead(const Twine &Name,
   if (error_code EC = sys::fs::openFileForRead(Name, FD))
     return EC;
   Result.reset(new RealFile(FD));
+  Result->setName(Name.str());
   return error_code::success();
 }
 
@@ -267,7 +279,10 @@ class FileEntry : public Entry {
         UseName(UseName) {}
   StringRef getExternalContentsPath() const { return ExternalContentsPath; }
   /// \brief whether to use the external path as the name for this file.
-  NameKind useName() const { return UseName; }
+  bool useExternalName(bool GlobalUseExternalName) const {
+    return UseName == NK_NotSet ? GlobalUseExternalName
+                                : (UseName == NK_External);
+  }
   static bool classof(const Entry *E) { return E->getKind() == EK_File; }
 };
 
@@ -770,8 +785,7 @@ ErrorOr<Status> VFSFromYAML::status(const Twine &Path) {
   if (FileEntry *F = dyn_cast<FileEntry>(*Result)) {
     ErrorOr<Status> S = ExternalFS->status(F->getExternalContentsPath());
     assert(!S || S->getName() == F->getExternalContentsPath());
-    if (S && (F->useName() == FileEntry::NK_Virtual ||
-              (F->useName() == FileEntry::NK_NotSet && !UseExternalNames)))
+    if (S && !F->useExternalName(UseExternalNames))
       S->setName(PathStr);
     return S;
   } else { // directory
@@ -792,7 +806,14 @@ error_code VFSFromYAML::openFileForRead(const Twine &Path,
   if (!F) // FIXME: errc::not_a_file?
     return error_code(errc::invalid_argument, system_category());
 
-  return ExternalFS->openFileForRead(F->getExternalContentsPath(), Result);
+  if (error_code EC = ExternalFS->openFileForRead(F->getExternalContentsPath(),
+                                                  Result))
+    return EC;
+
+  if (!F->useExternalName(UseExternalNames))
+    Result->setName(Path.str());
+
+  return error_code::success();
 }
 
 IntrusiveRefCntPtr<FileSystem>

diff --git a/lib/Frontend/CompilerInstance.cpp b/lib/Frontend/CompilerInstance.cpp
@@ -886,7 +886,7 @@ static void compileModule(CompilerInstance &ImportingInstance,
 
   // Note that this module is part of the module build stack, so that we
   // can detect cycles in the module graph.
-  Instance.createFileManager(); // FIXME: Adopt file manager from importer?
+  Instance.setFileManager(&ImportingInstance.getFileManager());
   Instance.createSourceManager(Instance.getFileManager());
   SourceManager &SourceMgr = Instance.getSourceManager();
   SourceMgr.setModuleBuildStack(

diff --git a/lib/Lex/PTHLexer.cpp b/lib/Lex/PTHLexer.cpp
@@ -688,6 +688,7 @@ class PTHStatCache : public FileSystemStatCache {
     if (!D.HasData)
       return CacheMissing;
 
+    Data.Name = Path;
     Data.Size = D.Size;
     Data.ModTime = D.ModTime;
     Data.UniqueID = D.UniqueID;

diff --git a/test/VFS/Inputs/external-names.h b/test/VFS/Inputs/external-names.h
@@ -0,0 +1,4 @@
+void foo(char **c) {
+  *c = __FILE__;
+  int x = c; // produce a diagnostic
+}
diff --git a/test/VFS/Inputs/use-external-names.yaml b/test/VFS/Inputs/use-external-names.yaml
@@ -0,0 +1,7 @@
+{
+  'version': 0,
+  'use-external-names': EXTERNAL_NAMES,
+  'roots': [{ 'type': 'file', 'name': 'OUT_DIR/external-names.h',
+             'external-contents': 'INPUT_DIR/external-names.h'
+           }]
+}
diff --git a/test/VFS/external-names.c b/test/VFS/external-names.c
@@ -0,0 +1,35 @@
+// RUN: sed -e "s:INPUT_DIR:%S/Inputs:g" -e "s:OUT_DIR:%t:g" -e "s:EXTERNAL_NAMES:true:" %S/Inputs/use-external-names.yaml > %t.external.yaml
+// RUN: sed -e "s:INPUT_DIR:%S/Inputs:g" -e "s:OUT_DIR:%t:g" -e "s:EXTERNAL_NAMES:false:" %S/Inputs/use-external-names.yaml > %t.yaml
+// REQUIRES: shell
+
+#include "external-names.h"
+
+////
+// Preprocessor (__FILE__ macro and # directives):
+
+// RUN: %clang_cc1 -I %t -ivfsoverlay %t.external.yaml -E %s | FileCheck -check-prefix=CHECK-PP-EXTERNAL %s
+// CHECK-PP-EXTERNAL: # {{[0-9]*}} "[[NAME:.*Inputs.external-names.h]]"
+// CHECK-PP-EXTERNAL-NEXT: void foo(char **c) {
+// CHECK-PP-EXTERNAL-NEXT: *c = "[[NAME]]";
+
+// RUN: %clang_cc1 -I %t -ivfsoverlay %t.yaml -E %s | FileCheck -check-prefix=CHECK-PP %s
+// CHECK-PP-NOT: Inputs
+
+////
+// Diagnostics:
+
+// RUN: %clang_cc1 -I %t -ivfsoverlay %t.external.yaml -fsyntax-only %s 2>&1 | FileCheck -check-prefix=CHECK-DIAG-EXTERNAL %s
+// CHECK-DIAG-EXTERNAL: {{.*}}Inputs{{.}}external-names.h:{{[0-9]*:[0-9]*}}: warning: incompatible pointer
+
+// RUN: %clang_cc1 -I %t -ivfsoverlay %t.yaml -fsyntax-only %s 2>&1 | FileCheck -check-prefix=CHECK-DIAG %s
+// CHECK-DIAG-NOT: Inputs
+
+////
+// Debug info
+
+// RUN: %clang_cc1 -I %t -ivfsoverlay %t.external.yaml -triple %itanium_abi_triple -g -emit-llvm %s -o - | FileCheck -check-prefix=CHECK-DEBUG-EXTERNAL %s
+// CHECK-DEBUG-EXTERNAL: ![[Num:[0-9]*]] = metadata !{metadata !"{{.*}}Inputs{{.}}external-names.h
+// CHECK-DEBUG-EXTERNAL: metadata !{i32 {{[0-9]*}}, metadata ![[Num]]{{.*}}DW_TAG_file_type
+
+// RUN: %clang_cc1 -I %t -ivfsoverlay %t.yaml -triple %itanium_abi_triple -g -emit-llvm %s -o - | FileCheck -check-prefix=CHECK-DEBUG %s
+// CHECK-DEBUG-NOT: Inputs
diff --git a/test/VFS/module-import.m b/test/VFS/module-import.m
@@ -1,3 +1,4 @@
+// RUN: rm -rf %t
 // RUN: sed -e "s:INPUT_DIR:%S/Inputs:g" -e "s:OUT_DIR:%t:g" %S/Inputs/vfsoverlay.yaml > %t.yaml
 // RUN: %clang_cc1 -Werror -fmodules -fmodules-cache-path=%t -ivfsoverlay %t.yaml -I %t -fsyntax-only %s
 // REQUIRES: shell

diff --git a/unittests/Basic/FileManagerTest.cpp b/unittests/Basic/FileManagerTest.cpp
@@ -28,10 +28,13 @@ class FakeStatCache : public FileSystemStatCache {
 
   void InjectFileOrDirectory(const char *Path, ino_t INode, bool IsFile) {
     FileData Data;
-    memset(&Data, 0, sizeof(FileData));
-    llvm::sys::fs::UniqueID ID(1, INode);
-    Data.UniqueID = ID;
+    Data.Name = Path;
+    Data.Size = 0;
+    Data.ModTime = 0;
+    Data.UniqueID = llvm::sys::fs::UniqueID(1, INode);
     Data.IsDirectory = !IsFile;
+    Data.IsNamedPipe = false;
+    Data.InPCH = false;
     StatCalls[Path] = Data;
   }