[clang] e8efac4 - [clang] Fix the canonicalization of paths in -fdiagnostics-absolute-paths

[Karl-Johan Karlsson via cfe-commits]

Author: Karl-Johan Karlsson
Date: 2019-12-20T09:16:33+01:00
New Revision: e8efac4b15303932581c128dc3976f4359388338

URL: https://github.com/llvm/llvm-project/commit/e8efac4b15303932581c128dc3976f4359388338
DIFF: https://github.com/llvm/llvm-project/commit/e8efac4b15303932581c128dc3976f4359388338.diff

LOG: [clang] Fix the canonicalization of paths in -fdiagnostics-absolute-paths

In the current implementation of clang the canonicalization of paths in
diagnostic messages (when using -fdiagnostics-absolute-paths) only works
if the symbolic link is in the directory part of the filename, not if
the file itself is a symbolic link to another file.

This patch adds support to canonicalize the complete path including the
file.

Reviewers: rsmith, hans, rnk, ikudrin

Reviewed By: rnk

Differential Revision: https://reviews.llvm.org/D70527

Added: 
    clang/test/Frontend/absolute-paths-symlinks.c

Modified: 
    clang/include/clang/Basic/FileManager.h
    clang/lib/Basic/FileManager.cpp
    clang/lib/Frontend/TextDiagnostic.cpp

Removed: 
    


################################################################################
diff  --git a/clang/include/clang/Basic/FileManager.h b/clang/include/clang/Basic/FileManager.h
index 28cd05818087..fed43786d410 100644
--- a/clang/include/clang/Basic/FileManager.h
+++ b/clang/include/clang/Basic/FileManager.h
@@ -222,8 +222,8 @@ class FileManager : public RefCountedBase<FileManager> {
                   llvm::BumpPtrAllocator>
       SeenFileEntries;
 
-  /// The canonical names of directories.
-  llvm::DenseMap<const DirectoryEntry *, llvm::StringRef> CanonicalDirNames;
+  /// The canonical names of files and directories .
+  llvm::DenseMap<const void *, llvm::StringRef> CanonicalNames;
 
   /// Storage for canonical names that we have computed.
   llvm::BumpPtrAllocator CanonicalNameStorage;
@@ -421,6 +421,13 @@ class FileManager : public RefCountedBase<FileManager> {
   /// required, which is (almost) never.
   StringRef getCanonicalName(const DirectoryEntry *Dir);
 
+  /// Retrieve the canonical name for a given file.
+  ///
+  /// This is a very expensive operation, despite its results being cached,
+  /// and should only be used when the physical layout of the file system is
+  /// required, which is (almost) never.
+  StringRef getCanonicalName(const FileEntry *File);
+
   void PrintStats() const;
 };
 

diff  --git a/clang/lib/Basic/FileManager.cpp b/clang/lib/Basic/FileManager.cpp
index b4f0a35e0d09..079a4bbfc82f 100644
--- a/clang/lib/Basic/FileManager.cpp
+++ b/clang/lib/Basic/FileManager.cpp
@@ -548,10 +548,9 @@ void FileManager::GetUniqueIDMapping(
 }
 
 StringRef FileManager::getCanonicalName(const DirectoryEntry *Dir) {
-  // FIXME: use llvm::sys::fs::canonical() when it gets implemented
-  llvm::DenseMap<const DirectoryEntry *, llvm::StringRef>::iterator Known
-    = CanonicalDirNames.find(Dir);
-  if (Known != CanonicalDirNames.end())
+  llvm::DenseMap<const void *, llvm::StringRef>::iterator Known
+    = CanonicalNames.find(Dir);
+  if (Known != CanonicalNames.end())
     return Known->second;
 
   StringRef CanonicalName(Dir->getName());
@@ -560,7 +559,23 @@ StringRef FileManager::getCanonicalName(const DirectoryEntry *Dir) {
   if (!FS->getRealPath(Dir->getName(), CanonicalNameBuf))
     CanonicalName = StringRef(CanonicalNameBuf).copy(CanonicalNameStorage);
 
-  CanonicalDirNames.insert({Dir, CanonicalName});
+  CanonicalNames.insert({Dir, CanonicalName});
+  return CanonicalName;
+}
+
+StringRef FileManager::getCanonicalName(const FileEntry *File) {
+  llvm::DenseMap<const void *, llvm::StringRef>::iterator Known
+    = CanonicalNames.find(File);
+  if (Known != CanonicalNames.end())
+    return Known->second;
+
+  StringRef CanonicalName(File->getName());
+
+  SmallString<4096> CanonicalNameBuf;
+  if (!FS->getRealPath(File->getName(), CanonicalNameBuf))
+    CanonicalName = StringRef(CanonicalNameBuf).copy(CanonicalNameStorage);
+
+  CanonicalNames.insert({File, CanonicalName});
   return CanonicalName;
 }
 

diff  --git a/clang/lib/Frontend/TextDiagnostic.cpp b/clang/lib/Frontend/TextDiagnostic.cpp
index 7bb6c5b74d5f..78acaaf9f96e 100644
--- a/clang/lib/Frontend/TextDiagnostic.cpp
+++ b/clang/lib/Frontend/TextDiagnostic.cpp
@@ -761,11 +761,12 @@ void TextDiagnostic::printDiagnosticMessage(raw_ostream &OS,
 }
 
 void TextDiagnostic::emitFilename(StringRef Filename, const SourceManager &SM) {
-  SmallVector<char, 128> AbsoluteFilename;
+#ifdef _WIN32
+  SmallString<4096> TmpFilename;
+#endif
   if (DiagOpts->AbsolutePath) {
-    auto Dir = SM.getFileManager().getDirectory(
-        llvm::sys::path::parent_path(Filename));
-    if (Dir) {
+    auto File = SM.getFileManager().getFile(Filename);
+    if (File) {
       // We want to print a simplified absolute path, i. e. without "dots".
       //
       // The hardest part here are the paths like "<part1>/<link>/../<part2>".
@@ -781,16 +782,14 @@ void TextDiagnostic::emitFilename(StringRef Filename, const SourceManager &SM) {
       // on Windows we can just use llvm::sys::path::remove_dots(), because,
       // on that system, both aforementioned paths point to the same place.
 #ifdef _WIN32
-      SmallString<4096> DirName = (*Dir)->getName();
-      llvm::sys::fs::make_absolute(DirName);
-      llvm::sys::path::native(DirName);
-      llvm::sys::path::remove_dots(DirName, /* remove_dot_dot */ true);
+      TmpFilename = (*File)->getName();
+      llvm::sys::fs::make_absolute(TmpFilename);
+      llvm::sys::path::native(TmpFilename);
+      llvm::sys::path::remove_dots(TmpFilename, /* remove_dot_dot */ true);
+      Filename = StringRef(TmpFilename.data(), TmpFilename.size());
 #else
-      StringRef DirName = SM.getFileManager().getCanonicalName(*Dir);
+      Filename = SM.getFileManager().getCanonicalName(*File);
 #endif
-      llvm::sys::path::append(AbsoluteFilename, DirName,
-                              llvm::sys::path::filename(Filename));
-      Filename = StringRef(AbsoluteFilename.data(), AbsoluteFilename.size());
     }
   }
 

diff  --git a/clang/test/Frontend/absolute-paths-symlinks.c b/clang/test/Frontend/absolute-paths-symlinks.c
new file mode 100644
index 000000000000..e91a96fc0922
--- /dev/null
+++ b/clang/test/Frontend/absolute-paths-symlinks.c
@@ -0,0 +1,15 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: cd %t
+// RUN: cp "%s" "test.c"
+// RUN: ln -sf "test.c" "link.c"
+// RUN: not %clang_cc1 -fsyntax-only -fdiagnostics-absolute-paths "link.c" 2>&1|FileCheck %s
+
+// Verify that -fdiagnostics-absolute-paths resolve symbolic links in
+// diagnostics messages.
+
+// CHECK: test.c
+// CHECK-SAME: error: unknown type name
+This do not compile
+
+// REQUIRES: shell