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

[Ka-Ka]

Summary

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.

[Ka-Ka]

Ping

[ikudrin]

Personally, I would prefer to see the file name and path to be changed as little as possible because that would help to recognize the files better. We cannot use remove_dots() on POSIX OSes to simplify paths, because it may return an invalid path; thus we have to use getRealPath(). If I understand it right, there is no similar problem with the file name itself.

So, which issues this patch is going to solve?

[rnk]

From auditing the call sites, it seems that almost all of them could be simplified by using the new API:
http://llvm-cs.pcc.me.uk/tools/clang/lib/Basic/FileManager.cpp/rgetCanonicalName

clang/include/clang/Basic/FileManager.h

225–226

You could make the key void* and save a DenseMap here. Nobody ever iterates CanonicalDirNames to look at the keys.

[rnk]

In D70527#1785552, @ikudrin wrote:

Personally, I would prefer to see the file name and path to be changed as little as possible because that would help to recognize the files better. We cannot use remove_dots() on POSIX OSes to simplify paths, because it may return an invalid path; thus we have to use getRealPath(). If I understand it right, there is no similar problem with the file name itself.

So, which issues this patch is going to solve?

It seems clear to me, the filename could be an absolute symlink to a real file somewhere far removed from the realpath of the parent directory. It seems reasonable that -fdiagnostics-absolute-paths would look through symlinks in this case.

[Ka-Ka]

In D70527#1786718, @rnk wrote:

In D70527#1785552, @ikudrin wrote:

Personally, I would prefer to see the file name and path to be changed as little as possible because that would help to recognize the files better. We cannot use remove_dots() on POSIX OSes to simplify paths, because it may return an invalid path; thus we have to use getRealPath(). If I understand it right, there is no similar problem with the file name itself.

So, which issues this patch is going to solve?

It seems clear to me, the filename could be an absolute symlink to a real file somewhere far removed from the realpath of the parent directory. It seems reasonable that -fdiagnostics-absolute-paths would look through symlinks in this case.

This fix is important when building with Bazel (google build system) as Bazel set up a temporary sandbox (with a series of symbolic links) to be be able to control the dependencies and keep a valid build cache. Currently when using the clang option -fdiagnostics-absolute-paths it will only resolve the sybolic names in the dir-path of the filename which end up with warning- and error-messages to files inside the sandbox instead of the correct source inside the repo you are working in.

In D70527#1786698, @rnk wrote:

From auditing the call sites, it seems that almost all of them could be simplified by using the new API:
http://llvm-cs.pcc.me.uk/tools/clang/lib/Basic/FileManager.cpp/rgetCanonicalName

I agree, that they can be simplified, but I guess it is easier to do those changes in separate patches.

clang/include/clang/Basic/FileManager.h

225–226

Sure, I will update the patch ...

[Ka-Ka]

Updated patch according to review comments.

[rnk]

lgtm

clang/lib/Basic/FileManager.cpp

550–551

These FIXMEs look stale. We are already using VFS getRealPath which ultimately probably boils down to whatever the original author thought sys::fs::canonical would be.

567

I don't think we need to carry this FIXME here.

569

uber nit: use the .insert get-or-create pattern

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

16

I wish we had a more precise feature to indicate symlink support, but this is what we do elsewhere, so there's nothing to do here.

[MaskRay]

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

16

I wrote:

## Don't make symlinks on Windows.
# UNSUPPORTED: system-windows

in D71302. Yeah, please tell me which precise feature I should use :)

[MaskRay]

clang/lib/Basic/FileManager.cpp

562

There is no need to change now, but I think try_emplace may be slightly more idiomatic as it saves a copy-list-initialization.

572

There is no need to change now. StringRef ... = may be more common.

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

4

No need to change now. Quotes are not needed.

6

No need to change now. Spaces around |

[Ka-Ka]

I have now updated the testcase according to comments by @MaskRay in commit 073cdb239044
Thanks for post-commit review comments.

clang/include/clang/Basic/FileManager.h

class FileManager : public RefCountedBase<FileManager> {
   /// virtual) we have looked up, or an error that occurred when we looked up
   /// the file.
   ///
   /// \see SeenDirEntries
   llvm::StringMap<llvm::ErrorOr<SeenFileEntryOrRedirect>,
                   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;
 
   /// Each FileEntry we create is assigned a unique ID #.
   ///
   unsigned NextFileUID;
 
public:
 
   /// Retrieve the canonical name for a given directory.
   ///
   /// 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 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;
 };
 
 } // end namespace clang
 
 #endif // LLVM_CLANG_BASIC_FILEMANAGER_H

clang/lib/Basic/FileManager.cpp

if (llvm::ErrorOr<SeenFileEntryOrRedirect> Entry = FE->getValue()) {
         UIDToFiles[FE->getUID()] = FE;
     }
 
   // Map virtual file entries
   for (const auto &VFE : VirtualFileEntries)
     UIDToFiles[VFE->getUID()] = VFE.get();
 }
 
 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());
 
   SmallString<4096> CanonicalNameBuf;
   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;
 }
 
 void FileManager::PrintStats() const {
   llvm::errs() << "\n*** File Manager Stats:\n";
   llvm::errs() << UniqueRealFiles.size() << " real files found, "
                << UniqueRealDirs.size() << " real dirs found.\n";
   llvm::errs() << VirtualFileEntries.size() << " virtual files found, "
                << VirtualDirectoryEntries.size() << " virtual dirs found.\n";
   llvm::errs() << NumDirLookups << " dir lookups, "
                << NumDirCacheMisses << " dir cache misses.\n";
   llvm::errs() << NumFileLookups << " file lookups, "
                << NumFileCacheMisses << " file cache misses.\n";
 
   //llvm::errs() << PagesMapped << BytesOfPagesMapped << FSLookups;
 }

clang/lib/Frontend/TextDiagnostic.cpp

void TextDiagnostic::printDiagnosticMessage(raw_ostream &OS,
   }
 
   if (ShowColors)
     OS.resetColor();
   OS << '\n';
 }
 
 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>".
       // On Unix-like systems, we cannot just collapse "<link>/..", because
       // paths are resolved sequentially, and, thereby, the path
       // "<part1>/<part2>" may point to a different location. That is why
       // we use FileManager::getCanonicalName(), which expands all indirections
       // with llvm::sys::fs::real_path() and caches the result.
       //
       // On the other hand, it would be better to preserve as much of the
       // original path as possible, because that helps a user to recognize it.
       // real_path() expands all links, which sometimes too much. Luckily,
       // 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());
     }
   }
 
   OS << Filename;
 }
 
 /// Print out the file/line/column information and include trace.
 ///

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

+// 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