diff --git a/clang-tools-extra/test/clang-tidy/infrastructure/file-filter-symlinks.cpp b/clang-tools-extra/test/clang-tidy/infrastructure/file-filter-symlinks.cpp index 7efa7d070f69f..30bcdc3f87743 100644 --- a/clang-tools-extra/test/clang-tidy/infrastructure/file-filter-symlinks.cpp +++ b/clang-tools-extra/test/clang-tidy/infrastructure/file-filter-symlinks.cpp @@ -12,8 +12,9 @@ // RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='header\.h' %s -- -I %t 2>&1 | FileCheck --check-prefix=CHECK_HEADER %s // RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='header\.h' -quiet %s -- -I %t 2>&1 | FileCheck --check-prefix=CHECK_HEADER %s -// Check that `-header-filter` operates on the same file paths as paths in -// diagnostics printed by ClangTidy. +// `-header-filter` operates on the actual file path that the user provided in +// the #include directive; however, Clang's path name simplification causes the +// path to be printed in canonicalised form here. #include "dir1/dir2/../header_alias.h" -// CHECK_HEADER_ALIAS: dir1/dir2/../header_alias.h:1:11: warning: single-argument constructors +// CHECK_HEADER_ALIAS: dir1/header.h:1:11: warning: single-argument constructors // CHECK_HEADER-NOT: warning: diff --git a/clang/include/clang/Basic/SourceManager.h b/clang/include/clang/Basic/SourceManager.h index ed967fd47dc83..00da67fba669c 100644 --- a/clang/include/clang/Basic/SourceManager.h +++ b/clang/include/clang/Basic/SourceManager.h @@ -824,6 +824,12 @@ class SourceManager : public RefCountedBase { mutable std::unique_ptr FakeSLocEntryForRecovery; + /// Cache for filenames used in diagnostics. See 'getNameForDiagnostic()'. + mutable llvm::StringMap DiagNames; + + /// Allocator for absolute/short names. + mutable llvm::BumpPtrAllocator DiagNameAlloc; + /// Lazily computed map of macro argument chunks to their expanded /// source location. using MacroArgsMap = std::map; @@ -1848,6 +1854,16 @@ class SourceManager : public RefCountedBase { /// \return Location of the top-level macro caller. SourceLocation getTopMacroCallerLoc(SourceLocation Loc) const; + /// Retrieve the name of a file suitable for diagnostics. + // FIXME: Passing in the DiagnosticOptions here is a workaround for the + // fact that installapi does some weird things with DiagnosticsEngines, + // which causes the 'Diag' member of SourceManager (or at least the + // DiagnosticsOptions member thereof) to be a dangling reference + // sometimes. We should probably fix that or decouple the two classes + // to avoid this issue entirely. + StringRef getNameForDiagnostic(StringRef Filename, + const DiagnosticOptions &Opts) const; + private: friend class ASTReader; friend class ASTWriter; diff --git a/clang/lib/Basic/SourceManager.cpp b/clang/lib/Basic/SourceManager.cpp index b2b1488f9dc8e..e6b61302cc294 100644 --- a/clang/lib/Basic/SourceManager.cpp +++ b/clang/lib/Basic/SourceManager.cpp @@ -2390,3 +2390,75 @@ SourceManagerForFile::SourceManagerForFile(StringRef FileName, assert(ID.isValid()); SourceMgr->setMainFileID(ID); } + +StringRef +SourceManager::getNameForDiagnostic(StringRef Filename, + const DiagnosticOptions &Opts) const { + OptionalFileEntryRef File = getFileManager().getOptionalFileRef(Filename); + if (!File) + return Filename; + + bool SimplifyPath = [&] { + if (Opts.AbsolutePath) + return true; + + // Try to simplify paths that contain '..' in any case since paths to + // standard library headers especially tend to get quite long otherwise. + // Only do that for local filesystems though to avoid slowing down + // compilation too much. + if (!File->getName().contains("..")) + return false; + + // If we're not on Windows, check if we're on a network file system and + // avoid simplifying the path in that case since that can be slow. On + // Windows, the check for a local filesystem is already slow, so skip it. +#ifndef _WIN32 + if (!llvm::sys::fs::is_local(File->getName())) + return false; +#endif + + return true; + }(); + + if (!SimplifyPath) + return Filename; + + // This may involve computing canonical names, so cache the result. + StringRef &CacheEntry = DiagNames[Filename]; + if (!CacheEntry.empty()) + return CacheEntry; + + // We want to print a simplified absolute path, i. e. without "dots". + // + // The hardest part here are the paths like "//../". + // On Unix-like systems, we cannot just collapse "/..", because + // paths are resolved sequentially, and, thereby, the path + // "/" 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. + SmallString<256> TempBuf; +#ifdef _WIN32 + TempBuf = File->getName(); + llvm::sys::fs::make_absolute(TempBuf); + llvm::sys::path::native(TempBuf); + llvm::sys::path::remove_dots(TempBuf, /* remove_dot_dot */ true); +#else + TempBuf = getFileManager().getCanonicalName(*File); +#endif + + // In some cases, the resolved path may actually end up being longer (e.g. + // if it was originally a relative path), so just retain whichever one + // ends up being shorter. + if (!Opts.AbsolutePath && TempBuf.size() > Filename.size()) + CacheEntry = Filename; + else + CacheEntry = TempBuf.str().copy(DiagNameAlloc); + + return CacheEntry; +} diff --git a/clang/lib/Frontend/SARIFDiagnostic.cpp b/clang/lib/Frontend/SARIFDiagnostic.cpp index ac27d7480de3e..f9714018f7b46 100644 --- a/clang/lib/Frontend/SARIFDiagnostic.cpp +++ b/clang/lib/Frontend/SARIFDiagnostic.cpp @@ -163,36 +163,7 @@ SARIFDiagnostic::addDiagnosticLevelToRule(SarifRule Rule, llvm::StringRef SARIFDiagnostic::emitFilename(StringRef Filename, const SourceManager &SM) { - if (DiagOpts.AbsolutePath) { - auto File = SM.getFileManager().getOptionalFileRef(Filename); - if (File) { - // We want to print a simplified absolute path, i. e. without "dots". - // - // The hardest part here are the paths like "//../". - // On Unix-like systems, we cannot just collapse "/..", because - // paths are resolved sequentially, and, thereby, the path - // "/" 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 is 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<256> 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 - Filename = SM.getFileManager().getCanonicalName(*File); -#endif - } - } - - return Filename; + return SM.getNameForDiagnostic(Filename, DiagOpts); } /// Print out the file/line/column information and include trace. diff --git a/clang/lib/Frontend/TextDiagnostic.cpp b/clang/lib/Frontend/TextDiagnostic.cpp index ccdd59da89bd1..e5bf86791c44a 100644 --- a/clang/lib/Frontend/TextDiagnostic.cpp +++ b/clang/lib/Frontend/TextDiagnostic.cpp @@ -738,39 +738,7 @@ void TextDiagnostic::printDiagnosticMessage(raw_ostream &OS, } void TextDiagnostic::emitFilename(StringRef Filename, const SourceManager &SM) { -#ifdef _WIN32 - SmallString<4096> TmpFilename; -#endif - if (DiagOpts.AbsolutePath) { - auto File = SM.getFileManager().getOptionalFileRef(Filename); - if (File) { - // We want to print a simplified absolute path, i. e. without "dots". - // - // The hardest part here are the paths like "//../". - // On Unix-like systems, we cannot just collapse "/..", because - // paths are resolved sequentially, and, thereby, the path - // "/" 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 - 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 - Filename = SM.getFileManager().getCanonicalName(*File); -#endif - } - } - - OS << Filename; + OS << SM.getNameForDiagnostic(Filename, DiagOpts); } /// Print out the file/line/column information and include trace. diff --git a/clang/test/Frontend/absolute-paths.c b/clang/test/Frontend/absolute-paths.c index e06cf262dd8e2..e507f0d4c34b3 100644 --- a/clang/test/Frontend/absolute-paths.c +++ b/clang/test/Frontend/absolute-paths.c @@ -8,9 +8,9 @@ #include "absolute-paths.h" -// Check whether the diagnostic from the header above includes the dummy -// directory in the path. -// NORMAL: SystemHeaderPrefix +// Check that the bogus prefix we added is stripped out even if absolute paths +// are disabled. +// NORMAL-NOT: SystemHeaderPrefix // ABSOLUTE-NOT: SystemHeaderPrefix // CHECK: warning: non-void function does not return a value diff --git a/clang/test/Frontend/simplify-paths.c b/clang/test/Frontend/simplify-paths.c new file mode 100644 index 0000000000000..90af7c4ee3836 --- /dev/null +++ b/clang/test/Frontend/simplify-paths.c @@ -0,0 +1,18 @@ +// REQUIRES: shell + +// RUN: rm -rf %t +// RUN: mkdir -p %t/a/b/ +// RUN: echo 'void x;' > %t/test.h +// RUN: echo 'const void x;' > %t/header_with_a_really_long_name.h +// RUN: ln -s %t/header_with_a_really_long_name.h %t/a/shorter_name.h +// +// RUN: %clang_cc1 -fsyntax-only -I %t %s 2> %t/output.txt || true +// RUN: cat %t/output.txt | FileCheck %s + +// Check that we strip '..' by canonicalising the path... +#include "a/b/../../test.h" +// CHECK: simplify-paths.c.tmp/test.h:1:6: error: variable has incomplete type 'void' + +// ... but only if the resulting path is actually shorter. +#include "a/b/../shorter_name.h" +// CHECK: simplify-paths.c.tmp/a/b/../shorter_name.h:1:12: error: variable has incomplete type 'const void'