Revert "[lit][clang] Avoid realpath on Windows due to MAX_PATH limitations" #139739
+90
−151
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…tions" This reverts commit 05d613e.
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
llvmbot
added
cmake
|
@llvm/pr-subscribers-testing-tools @llvm/pr-subscribers-clang Author: None (guillem-bartrina-sonarsource) ChangesThis reverts commit 05d613e. Only one single test has been added after the reverted patch that makes use of the functionality it introduced.
Original discussion: https://reviews.llvm.org/D154130 Although it has been almost two years since the patch was merged and apparently no major problems associated with it have arisen, we still believe that the patch is unsound overall. Rationale: The whole point of canonical paths is to be unique for a given entity (directory or file), so that they can be directly compared to determine whether or not two entities are the same. This patch outright renders this fundamental property useless by allowing the same entity to have more than one canonical path on a Windows machine, depending on whether it is accessed via the real path or a substituted path. And the only motivation behind these deceptively innocuous changes seems to be to help circumvent a platform-dependent limitation (MAX_PATH on Windows) that apparently makes it difficult for some users (?) to run on lit tests. This limitation has workarounds (using directory junctions, enabling long paths or using UNC) that do not require changing llvm at all but simply some settings on the host machine. Furthermore, this limitation may no longer be relevant two years later. We also believe that the original patch was a bit incomplete. It indirectly affected the resolution of canonical paths used by certain parts of other llvm subsystems (module maps, frameworks, ...), and were not thoroughly tested for regressions. Also, some simplifications in the resolution of canonical paths in But the biggest concern, aside from breaking the fundamental property, is that the patch also inadvertently changes the way certain symbolic links are resolved in Windows, particularly those between different drives. [If the path contains a symbolic link to another drive, the absolute and real paths have different drive letters, even though they are not substituted drives... ] This behavior change is not documented in the patch description so it is definitely a regression. Finally, the last comment in the original discussion, which was posted after the merge, also raised concerns about the changes introduced by the patch. In summary, we believe that this patch is more detrimental than beneficial and should be reverted. As for the original patch, it is not easy to create any kind of test for this change in behavior. Since we are reverting a change, the need for tests is not so relevant. Patch is 20.43 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/139739.diff 16 Files Affected:
diff --git a/clang/include/clang/Basic/FileManager.h b/clang/include/clang/Basic/FileManager.h
index e83a61d6ff00c..769cfedae6018 100644
--- a/clang/include/clang/Basic/FileManager.h
+++ b/clang/include/clang/Basic/FileManager.h
@@ -319,13 +319,6 @@ class FileManager : public RefCountedBase<FileManager> {
/// required, which is (almost) never.
StringRef getCanonicalName(FileEntryRef File);
-private:
- /// Retrieve the canonical name for a given file or directory.
- ///
- /// The first param is a key in the CanonicalNames array.
- StringRef getCanonicalName(const void *Entry, StringRef Name);
-
-public:
void PrintStats() const;
/// Import statistics from a child FileManager and add them to this current
diff --git a/clang/lib/Basic/FileManager.cpp b/clang/lib/Basic/FileManager.cpp
index 86fe352df0461..f0285f56c9476 100644
--- a/clang/lib/Basic/FileManager.cpp
+++ b/clang/lib/Basic/FileManager.cpp
@@ -618,48 +618,33 @@ void FileManager::GetUniqueIDMapping(
}
StringRef FileManager::getCanonicalName(DirectoryEntryRef Dir) {
- return getCanonicalName(Dir, Dir.getName());
-}
+ auto Known = CanonicalNames.find(Dir);
+ if (Known != CanonicalNames.end())
+ return Known->second;
-StringRef FileManager::getCanonicalName(FileEntryRef File) {
- return getCanonicalName(File, File.getName());
+ StringRef CanonicalName(Dir.getName());
+
+ SmallString<4096> CanonicalNameBuf;
+ if (!FS->getRealPath(Dir.getName(), CanonicalNameBuf))
+ CanonicalName = CanonicalNameBuf.str().copy(CanonicalNameStorage);
+
+ CanonicalNames.insert({Dir, CanonicalName});
+ return CanonicalName;
}
-StringRef FileManager::getCanonicalName(const void *Entry, StringRef Name) {
+StringRef FileManager::getCanonicalName(FileEntryRef File) {
llvm::DenseMap<const void *, llvm::StringRef>::iterator Known =
- CanonicalNames.find(Entry);
+ CanonicalNames.find(File);
if (Known != CanonicalNames.end())
return Known->second;
- // Name comes from FileEntry/DirectoryEntry::getName(), so it is safe to
- // store it in the DenseMap below.
- StringRef CanonicalName(Name);
-
- SmallString<256> AbsPathBuf;
- SmallString<256> RealPathBuf;
- if (!FS->getRealPath(Name, RealPathBuf)) {
- if (is_style_windows(llvm::sys::path::Style::native)) {
- // For Windows paths, only use the real path if it doesn't resolve
- // a substitute drive, as those are used to avoid MAX_PATH issues.
- AbsPathBuf = Name;
- if (!FS->makeAbsolute(AbsPathBuf)) {
- if (llvm::sys::path::root_name(RealPathBuf) ==
- llvm::sys::path::root_name(AbsPathBuf)) {
- CanonicalName = RealPathBuf.str().copy(CanonicalNameStorage);
- } else {
- // Fallback to using the absolute path.
- // Simplifying /../ is semantically valid on Windows even in the
- // presence of symbolic links.
- llvm::sys::path::remove_dots(AbsPathBuf, /*remove_dot_dot=*/true);
- CanonicalName = AbsPathBuf.str().copy(CanonicalNameStorage);
- }
- }
- } else {
- CanonicalName = RealPathBuf.str().copy(CanonicalNameStorage);
- }
- }
+ StringRef CanonicalName(File.getName());
+
+ SmallString<4096> CanonicalNameBuf;
+ if (!FS->getRealPath(File.getName(), CanonicalNameBuf))
+ CanonicalName = CanonicalNameBuf.str().copy(CanonicalNameStorage);
- CanonicalNames.insert({Entry, CanonicalName});
+ CanonicalNames.insert({File, CanonicalName});
return CanonicalName;
}
diff --git a/clang/test/Lexer/case-insensitive-include-absolute.c b/clang/test/Lexer/case-insensitive-include-absolute.c
index ef9e131c45df5..4274d05045ecf 100644
--- a/clang/test/Lexer/case-insensitive-include-absolute.c
+++ b/clang/test/Lexer/case-insensitive-include-absolute.c
@@ -1,8 +1,8 @@
// REQUIRES: case-insensitive-filesystem
// RUN: rm -rf %t && split-file %s %t
-// RUN: sed "s|DIR|%{/t:real}|g" %t/tu.c.in > %t/tu.c
-// RUN: %clang_cc1 -fsyntax-only %t/tu.c 2>&1 | FileCheck %s -DDIR=%{/t:real}
+// RUN: sed "s|DIR|%/t|g" %t/tu.c.in > %t/tu.c
+// RUN: %clang_cc1 -fsyntax-only %t/tu.c 2>&1 | FileCheck %s -DDIR=%/t
//--- header.h
//--- tu.c.in
diff --git a/clang/test/Lexer/case-insensitive-include-win.c b/clang/test/Lexer/case-insensitive-include-win.c
index ed722feb3d994..6a17d0b552448 100644
--- a/clang/test/Lexer/case-insensitive-include-win.c
+++ b/clang/test/Lexer/case-insensitive-include-win.c
@@ -2,15 +2,9 @@
// This file should only include code that really needs a Windows host OS to
// run.
-// Note: We must use the real path here, because the logic to detect case
-// mismatches must resolve the real path to figure out the original casing.
-// If we use %t and we are on a substitute drive S: mapping to C:\subst,
-// then we will compare "S:\test.dir\FOO.h" to "C:\subst\test.dir\foo.h"
-// and avoid emitting the diagnostic because the structure is different.
-
// REQUIRES: system-windows
// RUN: mkdir -p %t.dir
// RUN: touch %t.dir/foo.h
-// RUN: not %clang_cl /FI \\?\%{t:real}.dir\FOO.h /WX -fsyntax-only %s 2>&1 | FileCheck %s
+// RUN: not %clang_cl /FI\\?\%t.dir\FOO.h /WX -fsyntax-only %s 2>&1 | FileCheck %s
// CHECK: non-portable path to file '"\\?\
diff --git a/llvm/cmake/modules/AddLLVM.cmake b/llvm/cmake/modules/AddLLVM.cmake
index 52bf7ca0906a3..2360f72e59d08 100644
--- a/llvm/cmake/modules/AddLLVM.cmake
+++ b/llvm/cmake/modules/AddLLVM.cmake
@@ -1887,15 +1887,10 @@ endfunction()
# use it and can't be in a lit module. Use with make_paths_relative().
string(CONCAT LLVM_LIT_PATH_FUNCTION
"# Allow generated file to be relocatable.\n"
- "import os\n"
- "import platform\n"
+ "from pathlib import Path\n"
"def path(p):\n"
" if not p: return ''\n"
- " # Follows lit.util.abs_path_preserve_drive, which cannot be imported here.\n"
- " if platform.system() == 'Windows':\n"
- " return os.path.abspath(os.path.join(os.path.dirname(__file__), p))\n"
- " else:\n"
- " return os.path.realpath(os.path.join(os.path.dirname(__file__), p))\n"
+ " return str((Path(__file__).parent / p).resolve())\n"
)
# This function provides an automatic way to 'configure'-like generate a file
diff --git a/llvm/docs/CommandGuide/lit.rst b/llvm/docs/CommandGuide/lit.rst
index 2a0ddd0ea04b4..46c5b9936239e 100644
--- a/llvm/docs/CommandGuide/lit.rst
+++ b/llvm/docs/CommandGuide/lit.rst
@@ -616,16 +616,6 @@ TestRunner.py:
%/T %T but ``\`` is replaced by ``/``
%{s:basename} The last path component of %s
%{t:stem} The last path component of %t but without the ``.tmp`` extension (alias for %basename_t)
- %{s:real} %s after expanding all symbolic links and substitute drives
- %{S:real} %S after expanding all symbolic links and substitute drives
- %{p:real} %p after expanding all symbolic links and substitute drives
- %{t:real} %t after expanding all symbolic links and substitute drives
- %{T:real} %T after expanding all symbolic links and substitute drives
- %{/s:real} %/s after expanding all symbolic links and substitute drives
- %{/S:real} %/S after expanding all symbolic links and substitute drives
- %{/p:real} %/p after expanding all symbolic links and substitute drives
- %{/t:real} %/t after expanding all symbolic links and substitute drives
- %{/T:real} %/T after expanding all symbolic links and substitute drives
%{/s:regex_replacement} %/s but escaped for use in the replacement of a ``s@@@`` command in sed
%{/S:regex_replacement} %/S but escaped for use in the replacement of a ``s@@@`` command in sed
%{/p:regex_replacement} %/p but escaped for use in the replacement of a ``s@@@`` command in sed
diff --git a/llvm/docs/TestingGuide.rst b/llvm/docs/TestingGuide.rst
index b6dda6a732405..3a555d6bea5aa 100644
--- a/llvm/docs/TestingGuide.rst
+++ b/llvm/docs/TestingGuide.rst
@@ -757,7 +757,7 @@ RUN lines:
``%{fs-sep}``
Expands to the file system separator, i.e. ``/`` or ``\`` on Windows.
-``%/s, %/S, %/t, %/T``
+``%/s, %/S, %/t, %/T:``
Act like the corresponding substitution above but replace any ``\``
character with a ``/``. This is useful to normalize path separators.
@@ -766,17 +766,7 @@ RUN lines:
Example: ``%/s: C:/Desktop Files/foo_test.s.tmp``
-``%{s:real}, %{S:real}, %{t:real}, %{T:real}``
-``%{/s:real}, %{/S:real}, %{/t:real}, %{/T:real}``
-
- Act like the corresponding substitution, including with ``/``, but use
- the real path by expanding all symbolic links and substitute drives.
-
- Example: ``%s: S:\foo_test.s.tmp``
-
- Example: ``%{/s:real}: C:/SDrive/foo_test.s.tmp``
-
-``%:s, %:S, %:t, %:T``
+``%:s, %:S, %:t, %:T:``
Act like the corresponding substitution above but remove colons at
the beginning of Windows paths. This is useful to allow concatenation
diff --git a/llvm/utils/lit/lit/TestRunner.py b/llvm/utils/lit/lit/TestRunner.py
index 16e9c7fbf45c5..fbef1aa6ec9dc 100644
--- a/llvm/utils/lit/lit/TestRunner.py
+++ b/llvm/utils/lit/lit/TestRunner.py
@@ -96,7 +96,7 @@ def change_dir(self, newdir):
if os.path.isabs(newdir):
self.cwd = newdir
else:
- self.cwd = lit.util.abs_path_preserve_drive(os.path.join(self.cwd, newdir))
+ self.cwd = os.path.realpath(os.path.join(self.cwd, newdir))
class TimeoutHelper(object):
@@ -455,7 +455,7 @@ def executeBuiltinMkdir(cmd, cmd_shenv):
dir = to_unicode(dir) if kIsWindows else to_bytes(dir)
cwd = to_unicode(cwd) if kIsWindows else to_bytes(cwd)
if not os.path.isabs(dir):
- dir = lit.util.abs_path_preserve_drive(os.path.join(cwd, dir))
+ dir = os.path.realpath(os.path.join(cwd, dir))
if parent:
lit.util.mkdir_p(dir)
else:
@@ -501,7 +501,7 @@ def on_rm_error(func, path, exc_info):
path = to_unicode(path) if kIsWindows else to_bytes(path)
cwd = to_unicode(cwd) if kIsWindows else to_bytes(cwd)
if not os.path.isabs(path):
- path = lit.util.abs_path_preserve_drive(os.path.join(cwd, path))
+ path = os.path.realpath(os.path.join(cwd, path))
if force and not os.path.exists(path):
continue
try:
@@ -1399,11 +1399,19 @@ def getDefaultSubstitutions(test, tmpDir, tmpBase, normalize_slashes=False):
tmpBaseName = os.path.basename(tmpBase)
sourceBaseName = os.path.basename(sourcepath)
- substitutions.append(("%{pathsep}", os.pathsep))
- substitutions.append(("%basename_t", tmpBaseName))
-
- substitutions.append(("%{s:basename}", sourceBaseName))
- substitutions.append(("%{t:stem}", tmpBaseName))
+ substitutions.extend(
+ [
+ ("%s", sourcepath),
+ ("%S", sourcedir),
+ ("%p", sourcedir),
+ ("%{pathsep}", os.pathsep),
+ ("%t", tmpName),
+ ("%basename_t", tmpBaseName),
+ ("%T", tmpDir),
+ ("%{s:basename}", sourceBaseName),
+ ("%{t:stem}", tmpBaseName)
+ ]
+ )
substitutions.extend(
[
@@ -1413,47 +1421,49 @@ def getDefaultSubstitutions(test, tmpDir, tmpBase, normalize_slashes=False):
]
)
- substitutions.append(("%/et", tmpName.replace("\\", "\\\\\\\\\\\\\\\\")))
+ # "%/[STpst]" should be normalized.
+ substitutions.extend(
+ [
+ ("%/s", sourcepath.replace("\\", "/")),
+ ("%/S", sourcedir.replace("\\", "/")),
+ ("%/p", sourcedir.replace("\\", "/")),
+ ("%/t", tmpBase.replace("\\", "/") + ".tmp"),
+ ("%/T", tmpDir.replace("\\", "/")),
+ ("%/et", tmpName.replace("\\", "\\\\\\\\\\\\\\\\")),
+ ]
+ )
+ # "%{/[STpst]:regex_replacement}" should be normalized like "%/[STpst]" but we're
+ # also in a regex replacement context of a s@@@ regex.
def regex_escape(s):
s = s.replace("@", r"\@")
s = s.replace("&", r"\&")
return s
- path_substitutions = [
- ("s", sourcepath), ("S", sourcedir), ("p", sourcedir),
- ("t", tmpName), ("T", tmpDir)
- ]
- for path_substitution in path_substitutions:
- letter = path_substitution[0]
- path = path_substitution[1]
-
- # Original path variant
- substitutions.append(("%" + letter, path))
-
- # Normalized path separator variant
- substitutions.append(("%/" + letter, path.replace("\\", "/")))
-
- # realpath variants
- # Windows paths with substitute drives are not expanded by default
- # as they are used to avoid MAX_PATH issues, but sometimes we do
- # need the fully expanded path.
- real_path = os.path.realpath(path)
- substitutions.append(("%{" + letter + ":real}", real_path))
- substitutions.append(("%{/" + letter + ":real}",
- real_path.replace("\\", "/")))
-
- # "%{/[STpst]:regex_replacement}" should be normalized like
- # "%/[STpst]" but we're also in a regex replacement context
- # of a s@@@ regex.
- substitutions.append(
- ("%{/" + letter + ":regex_replacement}",
- regex_escape(path.replace("\\", "/"))))
-
- # "%:[STpst]" are normalized paths without colons and without
- # a leading slash.
- substitutions.append(("%:" + letter, colonNormalizePath(path)))
+ substitutions.extend(
+ [
+ ("%{/s:regex_replacement}", regex_escape(sourcepath.replace("\\", "/"))),
+ ("%{/S:regex_replacement}", regex_escape(sourcedir.replace("\\", "/"))),
+ ("%{/p:regex_replacement}", regex_escape(sourcedir.replace("\\", "/"))),
+ (
+ "%{/t:regex_replacement}",
+ regex_escape(tmpBase.replace("\\", "/")) + ".tmp",
+ ),
+ ("%{/T:regex_replacement}", regex_escape(tmpDir.replace("\\", "/"))),
+ ]
+ )
+ # "%:[STpst]" are normalized paths without colons and without a leading
+ # slash.
+ substitutions.extend(
+ [
+ ("%:s", colonNormalizePath(sourcepath)),
+ ("%:S", colonNormalizePath(sourcedir)),
+ ("%:p", colonNormalizePath(sourcedir)),
+ ("%:t", colonNormalizePath(tmpBase + ".tmp")),
+ ("%:T", colonNormalizePath(tmpDir)),
+ ]
+ )
return substitutions
diff --git a/llvm/utils/lit/lit/builtin_commands/diff.py b/llvm/utils/lit/lit/builtin_commands/diff.py
index fbf4eb0e137b3..3a91920f9b5ed 100644
--- a/llvm/utils/lit/lit/builtin_commands/diff.py
+++ b/llvm/utils/lit/lit/builtin_commands/diff.py
@@ -281,7 +281,7 @@ def main(argv):
try:
for file in args:
if file != "-" and not os.path.isabs(file):
- file = util.abs_path_preserve_drive(file)
+ file = os.path.realpath(os.path.join(os.getcwd(), file))
if flags.recursive_diff:
if file == "-":
diff --git a/llvm/utils/lit/lit/discovery.py b/llvm/utils/lit/lit/discovery.py
index 2e7f90c6bb0c9..a0a2549d8df99 100644
--- a/llvm/utils/lit/lit/discovery.py
+++ b/llvm/utils/lit/lit/discovery.py
@@ -7,7 +7,7 @@
import sys
from lit.TestingConfig import TestingConfig
-from lit import LitConfig, Test, util
+from lit import LitConfig, Test
def chooseConfigFileFromDir(dir, config_names):
@@ -56,7 +56,7 @@ def search1(path):
# configuration to load instead.
config_map = litConfig.params.get("config_map")
if config_map:
- cfgpath = util.abs_path_preserve_drive(cfgpath)
+ cfgpath = os.path.realpath(cfgpath)
target = config_map.get(os.path.normcase(cfgpath))
if target:
cfgpath = target
@@ -67,13 +67,13 @@ def search1(path):
cfg = TestingConfig.fromdefaults(litConfig)
cfg.load_from_path(cfgpath, litConfig)
- source_root = util.abs_path_preserve_drive(cfg.test_source_root or path)
- exec_root = util.abs_path_preserve_drive(cfg.test_exec_root or path)
+ source_root = os.path.realpath(cfg.test_source_root or path)
+ exec_root = os.path.realpath(cfg.test_exec_root or path)
return Test.TestSuite(cfg.name, source_root, exec_root, cfg), ()
def search(path):
# Check for an already instantiated test suite.
- real_path = util.abs_path_preserve_drive(path)
+ real_path = os.path.realpath(path)
res = cache.get(real_path)
if res is None:
cache[real_path] = res = search1(path)
diff --git a/llvm/utils/lit/lit/util.py b/llvm/utils/lit/lit/util.py
index b03fd8bc22693..bd32d760f8ff1 100644
--- a/llvm/utils/lit/lit/util.py
+++ b/llvm/utils/lit/lit/util.py
@@ -128,23 +128,6 @@ def usable_core_count():
return n
-def abs_path_preserve_drive(path):
- """Return the absolute path without resolving drive mappings on Windows.
-
- """
- if platform.system() == "Windows":
- # Windows has limitations on path length (MAX_PATH) that
- # can be worked around using substitute drives, which map
- # a drive letter to a longer path on another drive.
- # Since Python 3.8, os.path.realpath resolves sustitute drives,
- # so we should not use it. In Python 3.7, os.path.realpath
- # was implemented as os.path.abspath.
- return os.path.abspath(path)
- else:
- # On UNIX, the current directory always has symbolic links resolved,
- # so any program accepting relative paths cannot preserve symbolic
- # links in paths and we should always use os.path.realpath.
- return os.path.realpath(path)
def mkdir(path):
try:
diff --git a/llvm/utils/lit/tests/Inputs/config-map-discovery/driver.py b/llvm/utils/lit/tests/Inputs/config-map-discovery/driver.py
index 50ad2cd65cb1b..d22b84d6a15cf 100644
--- a/llvm/utils/lit/tests/Inputs/config-map-discovery/driver.py
+++ b/llvm/utils/lit/tests/Inputs/config-map-discovery/driver.py
@@ -2,7 +2,8 @@
import os
import sys
-main_config = lit.util.abs_path_preserve_drive(sys.argv[1])
+main_config = sys.argv[1]
+main_config = os.path.realpath(main_config)
main_config = os.path.normcase(main_config)
config_map = {main_config: sys.argv[2]}
diff --git a/llvm/utils/lit/tests/Inputs/config-map-discovery/lit.alt.cfg b/llvm/utils/lit/tests/Inputs/config-map-discovery/lit.alt.cfg
index 27860e192fdc1..c7b303f50a05c 100644
--- a/llvm/utils/lit/tests/Inputs/config-map-discovery/lit.alt.cfg
+++ b/llvm/utils/lit/tests/Inputs/config-map-discovery/lit.alt.cfg
@@ -5,5 +5,5 @@ config.suffixes = ['.txt']
config.test_format = lit.formats.ShTest()
import os
-config.test_exec_root = os.path.dirname(lit.util.abs_path_preserve_drive(__file__))
+config.test_exec_root = os.path.realpath(os.path.dirname(__file__))
config.test_source_root = os.path.join(config.test_exec_root, "tests")
diff --git a/llvm/utils/lit/tests/Inputs/use-llvm-tool-required/lit.cfg b/llvm/utils/lit/tests/Inputs/use-llvm-tool-required/lit.cfg
index 5062e38c82f14..e41207bc2f05d 100644
--- a/llvm/utils/lit/tests/Inputs/use-llvm-tool-required/lit.cfg
+++ b/llvm/utils/lit/tests/Inputs/use-llvm-tool-required/lit.cfg
@@ -1,5 +1,4 @@
import lit.formats
-import lit.util
config.name = "use-llvm-tool-required"
config.suffixes = [".txt"]
@@ -8,7 +7,7 @@ config.test_source_root = None
config.test_exec_root = None
import os.path
-config.llvm_tools_dir = os.path.dirname(lit.util.abs_path_preserve_drive(__file__))
+config.llvm_tools_dir = os.path.realpath(os.path.dirname(__file__))
import lit.llvm
lit.llvm.initialize(lit_config, config)
diff --git a/llvm/utils/lit/tests/Inputs/use-llvm-tool/lit.cfg b/llvm/utils/lit/tests/Inputs/use-llvm-tool/lit.cfg
index 2a52db2183edf..8fe62d98c1349 100644
--- a/llvm/utils/lit/tests/Inputs/use-llvm-tool/lit.cfg
+++ b/llvm/utils/lit/tests/Inputs/use-llvm-tool/lit.cfg
@@ -1,5 +1,4 @@
import lit.formats
-import lit.util
config.name = "use-llvm-tool"
config.suffixes = [".txt"]
@@ -8,7 +7,7 @@ config.test_source_root = None
config.test_exec_root = None
import os.path
-this_dir = os.path.dirname(lit.util.abs_path_preserve_drive(__file__))
+this_dir = os.path.realpath(os.path.dirname(__file__))
config.llvm_tools_dir = os.path.join(this_dir, "build")
import lit.llvm
diff --git a/llvm/utils/llvm-lit/llvm-lit.in b/llvm/utils/llvm-lit/llvm-lit.in
index 0b6683...
[truncated]
|
steakhal
requested review from
compnerd,
benlangmuir,
AaronBallman and
jansvoboda11
You can test this locally with the following command:darker --check --diff -r HEAD~1...HEAD llvm/utils/lit/lit/TestRunner.py llvm/utils/lit/lit/builtin_commands/diff.py llvm/utils/lit/lit/discovery.py llvm/utils/lit/lit/util.py llvm/utils/lit/tests/Inputs/config-map-discovery/driver.pyView the diff from darker here.--- lit/TestRunner.py 2025-05-13 12:54:35.000000 +0000
+++ lit/TestRunner.py 2025-05-13 15:00:30.579509 +0000
@@ -1407,11 +1407,11 @@
("%{pathsep}", os.pathsep),
("%t", tmpName),
("%basename_t", tmpBaseName),
("%T", tmpDir),
("%{s:basename}", sourceBaseName),
- ("%{t:stem}", tmpBaseName)
+ ("%{t:stem}", tmpBaseName),
]
)
substitutions.extend(
[
|
|
The original patch is needed and is actively in use. The intention is to be able to support testing of the LLVM (and Swift) test suites where the paths are substituted away. The tiny path limit in Win32 is a problem, and creating a drive substitution is a reasonable solution here. Without this change, a file on a substituted drive will have two separate paths. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This reverts commit 05d613e.
Only one single test has been added after the reverted patch that makes use of the functionality it introduced.
%{/t:real}incase-insensitive-include-absolute.chas been replaced by the good ol'%/t.Original discussion: https://reviews.llvm.org/D154130
Although it has been almost two years since the patch was merged and apparently no major problems associated with it have arisen, we still believe that the patch is unsound overall.
Rationale:
The whole point of canonical paths is to be unique for a given entity (directory or file), so that they can be directly compared to determine whether or not two entities are the same.
This patch outright renders this fundamental property useless by allowing the same entity to have more than one canonical path on a Windows machine, depending on whether it is accessed via the real path or a substituted path.
And the only motivation behind these deceptively innocuous changes seems to be to help circumvent a platform-dependent limitation (MAX_PATH on Windows) that apparently makes it difficult for some users (?) to run lit tests. This limitation has workarounds (using directory junctions, enabling long paths or using UNC) that do not require changing llvm at all but simply some settings on the host machine. Furthermore, this limitation may no longer be relevant two years later.
We also believe that the original patch was a bit incomplete. It indirectly affected the resolution of canonical paths used by certain parts of other llvm subsystems (module maps, frameworks, ...), and were not thoroughly tested for regressions.
Also, some simplifications in the resolution of canonical paths in
(Text|SARIF)Diagnostic.cppmay have been overlooked.But the biggest concern, aside from breaking the fundamental property, is that the patch also inadvertently changes the way certain symbolic links are resolved in Windows, particularly those between different drives. [If the path contains a symbolic link to another drive, the absolute and real paths have different drive letters, even though they are not substituted drives... ] This behavior change is not documented in the patch description so it is definitely a regression.
Finally, the last comment in the original discussion, which was posted after the merge, also raised concerns about the changes introduced by the patch.
In summary, we believe that this patch is more detrimental than beneficial and should be reverted.
Similar to the original patch, it is not easy to create any kind of test for this behavior change. Since we are reversing a change, the need for tests is not as relevant.