[llvm-branch-commits] [clang-tools-extra] 1c995e2 - Revert "[Clang] [Diagnostics] Simplify filenames that contain '..' (#143520)"
via llvm-branch-commits
llvm-branch-commits at lists.llvm.org
Sat Jul 12 06:10:16 PDT 2025
Author: Sirraide
Date: 2025-07-12T15:10:13+02:00
New Revision: 1c995e203fad211cf7bcebb26ecc8bf783caf3b0
URL: https://github.com/llvm/llvm-project/commit/1c995e203fad211cf7bcebb26ecc8bf783caf3b0
DIFF: https://github.com/llvm/llvm-project/commit/1c995e203fad211cf7bcebb26ecc8bf783caf3b0.diff
LOG: Revert "[Clang] [Diagnostics] Simplify filenames that contain '..' (#143520)"
This reverts commit e3e7393c4681f31cd3820fdda2831a3e004a48f5.
Added:
Modified:
clang-tools-extra/test/clang-tidy/infrastructure/file-filter-symlinks.cpp
clang/include/clang/Basic/SourceManager.h
clang/lib/Basic/SourceManager.cpp
clang/lib/Frontend/SARIFDiagnostic.cpp
clang/lib/Frontend/TextDiagnostic.cpp
clang/test/Frontend/absolute-paths.c
Removed:
clang/test/Frontend/simplify-paths.c
################################################################################
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 30bcdc3f87743..7efa7d070f69f 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,9 +12,8 @@
// 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
-// `-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.
+// Check that `-header-filter` operates on the same file paths as paths in
+// diagnostics printed by ClangTidy.
#include "dir1/dir2/../header_alias.h"
-// CHECK_HEADER_ALIAS: dir1/header.h:1:11: warning: single-argument constructors
+// CHECK_HEADER_ALIAS: dir1/dir2/../header_alias.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 00da67fba669c..ed967fd47dc83 100644
--- a/clang/include/clang/Basic/SourceManager.h
+++ b/clang/include/clang/Basic/SourceManager.h
@@ -824,12 +824,6 @@ class SourceManager : public RefCountedBase<SourceManager> {
mutable std::unique_ptr<SrcMgr::SLocEntry> FakeSLocEntryForRecovery;
- /// Cache for filenames used in diagnostics. See 'getNameForDiagnostic()'.
- mutable llvm::StringMap<StringRef> 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<unsigned, SourceLocation>;
@@ -1854,16 +1848,6 @@ class SourceManager : public RefCountedBase<SourceManager> {
/// \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 e6b61302cc294..b2b1488f9dc8e 100644
--- a/clang/lib/Basic/SourceManager.cpp
+++ b/clang/lib/Basic/SourceManager.cpp
@@ -2390,75 +2390,3 @@ 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 "<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
diff erent 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 f9714018f7b46..ac27d7480de3e 100644
--- a/clang/lib/Frontend/SARIFDiagnostic.cpp
+++ b/clang/lib/Frontend/SARIFDiagnostic.cpp
@@ -163,7 +163,36 @@ SARIFDiagnostic::addDiagnosticLevelToRule(SarifRule Rule,
llvm::StringRef SARIFDiagnostic::emitFilename(StringRef Filename,
const SourceManager &SM) {
- return SM.getNameForDiagnostic(Filename, DiagOpts);
+ 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 "<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
diff erent 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;
}
/// 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 e5bf86791c44a..ccdd59da89bd1 100644
--- a/clang/lib/Frontend/TextDiagnostic.cpp
+++ b/clang/lib/Frontend/TextDiagnostic.cpp
@@ -738,7 +738,39 @@ void TextDiagnostic::printDiagnosticMessage(raw_ostream &OS,
}
void TextDiagnostic::emitFilename(StringRef Filename, const SourceManager &SM) {
- OS << SM.getNameForDiagnostic(Filename, DiagOpts);
+#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 "<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
diff erent 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;
}
/// 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 e507f0d4c34b3..e06cf262dd8e2 100644
--- a/clang/test/Frontend/absolute-paths.c
+++ b/clang/test/Frontend/absolute-paths.c
@@ -8,9 +8,9 @@
#include "absolute-paths.h"
-// Check that the bogus prefix we added is stripped out even if absolute paths
-// are disabled.
-// NORMAL-NOT: SystemHeaderPrefix
+// Check whether the diagnostic from the header above includes the dummy
+// directory in the path.
+// NORMAL: 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
deleted file mode 100644
index 90af7c4ee3836..0000000000000
--- a/clang/test/Frontend/simplify-paths.c
+++ /dev/null
@@ -1,18 +0,0 @@
-// 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'
More information about the llvm-branch-commits
mailing list