Skip to content

Commit 05d613e

Browse files
compnerdcompnerd
committed
[lit][clang] Avoid realpath on Windows due to MAX_PATH limitations
Running lit tests on Windows can fail because its use of `os.path.realpath` expands substitute drives, which are used to keep paths short and avoid hitting MAX_PATH limitations. Changes lit logic to: Use `os.path.abspath` on Windows, where `MAX_PATH` is a concern that we can work around using substitute drives, which `os.path.realpath` would resolve. Use `os.path.realpath` on Unix, where the current directory always has symlinks resolved, so it is impossible to preserve symlinks in the presence of relative paths, and so we must make sure that all code paths use real paths. Also updates clang's `FileManager::getCanonicalName` and `ExtractAPI` code to avoid resolving substitute drives (i.e. resolving to a path under a different root). How tested: built with `-DLLVM_ENABLE_PROJECTS=clang` and built `check-all` on both Windows Differential Revision: https://reviews.llvm.org/D154130 Reviewed By: @benlangmuir Patch by Tristan Labelle <tristan@thebrowser.company>!
1 parent cdb7d57 commit 05d613e

File tree

16 files changed

+150
-92
lines changed

16 files changed

+150
-92
lines changed

‎clang/include/clang/Basic/FileManager.h‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -329,6 +329,13 @@ class FileManager : public RefCountedBase<FileManager> {
329329
/// required, which is (almost) never.
330330
StringRef getCanonicalName(const FileEntry *File);
331331

332+
private:
333+
/// Retrieve the canonical name for a given file or directory.
334+
///
335+
/// The first param is a key in the CanonicalNames array.
336+
StringRef getCanonicalName(const void *Entry, StringRef Name);
337+
338+
public:
332339
void PrintStats() const;
333340
};
334341

‎clang/lib/Basic/FileManager.cpp‎

Lines changed: 35 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -632,33 +632,48 @@ void FileManager::GetUniqueIDMapping(
632632
}
633633

634634
StringRef FileManager::getCanonicalName(DirectoryEntryRef Dir) {
635-
auto Known = CanonicalNames.find(Dir);
636-
if (Known != CanonicalNames.end())
637-
return Known->second;
638-
639-
StringRef CanonicalName(Dir.getName());
640-
641-
SmallString<4096> CanonicalNameBuf;
642-
if (!FS->getRealPath(Dir.getName(), CanonicalNameBuf))
643-
CanonicalName = CanonicalNameBuf.str().copy(CanonicalNameStorage);
644-
645-
CanonicalNames.insert({Dir, CanonicalName});
646-
return CanonicalName;
635+
return getCanonicalName(Dir, Dir.getName());
647636
}
648637

649638
StringRef FileManager::getCanonicalName(const FileEntry *File) {
650-
llvm::DenseMap<const void *, llvm::StringRef>::iterator Known
651-
= CanonicalNames.find(File);
639+
return getCanonicalName(File, File->getName());
640+
}
641+
642+
StringRef FileManager::getCanonicalName(const void *Entry, StringRef Name) {
643+
llvm::DenseMap<const void *, llvm::StringRef>::iterator Known =
644+
CanonicalNames.find(Entry);
652645
if (Known != CanonicalNames.end())
653646
return Known->second;
654647

655-
StringRef CanonicalName(File->getName());
656-
657-
SmallString<4096> CanonicalNameBuf;
658-
if (!FS->getRealPath(File->getName(), CanonicalNameBuf))
659-
CanonicalName = CanonicalNameBuf.str().copy(CanonicalNameStorage);
648+
// Name comes from FileEntry/DirectoryEntry::getName(), so it is safe to
649+
// store it in the DenseMap below.
650+
StringRef CanonicalName(Name);
651+
652+
SmallString<256> AbsPathBuf;
653+
SmallString<256> RealPathBuf;
654+
if (!FS->getRealPath(Name, RealPathBuf)) {
655+
if (is_style_windows(llvm::sys::path::Style::native)) {
656+
// For Windows paths, only use the real path if it doesn't resolve
657+
// a substitute drive, as those are used to avoid MAX_PATH issues.
658+
AbsPathBuf = Name;
659+
if (!FS->makeAbsolute(AbsPathBuf)) {
660+
if (llvm::sys::path::root_name(RealPathBuf) ==
661+
llvm::sys::path::root_name(AbsPathBuf)) {
662+
CanonicalName = RealPathBuf.str().copy(CanonicalNameStorage);
663+
} else {
664+
// Fallback to using the absolute path.
665+
// Simplifying /../ is semantically valid on Windows even in the
666+
// presence of symbolic links.
667+
llvm::sys::path::remove_dots(AbsPathBuf, /*remove_dot_dot=*/true);
668+
CanonicalName = AbsPathBuf.str().copy(CanonicalNameStorage);
669+
}
670+
}
671+
} else {
672+
CanonicalName = RealPathBuf.str().copy(CanonicalNameStorage);
673+
}
674+
}
660675

661-
CanonicalNames.insert({File, CanonicalName});
676+
CanonicalNames.insert({Entry, CanonicalName});
662677
return CanonicalName;
663678
}
664679

‎clang/lib/ExtractAPI/ExtractAPIConsumer.cpp‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -187,9 +187,7 @@ struct LocationFileChecker {
187187
if (ExternalFileEntries.count(File))
188188
return false;
189189

190-
StringRef FileName = File->tryGetRealPathName().empty()
191-
? File->getName()
192-
: File->tryGetRealPathName();
190+
StringRef FileName = SM.getFileManager().getCanonicalName(File);
193191

194192
// Try to reduce the include name the same way we tried to include it.
195193
bool IsQuoted = false;

‎clang/test/Lexer/case-insensitive-include-win.c‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,15 @@
22
// This file should only include code that really needs a Windows host OS to
33
// run.
44

5+
// Note: We must use the real path here, because the logic to detect case
6+
// mismatches must resolve the real path to figure out the original casing.
7+
// If we use %t and we are on a substitute drive S: mapping to C:\subst,
8+
// then we will compare "S:\test.dir\FOO.h" to "C:\subst\test.dir\foo.h"
9+
// and avoid emitting the diagnostic because the structure is different.
10+
511
// REQUIRES: system-windows
612
// RUN: mkdir -p %t.dir
713
// RUN: touch %t.dir/foo.h
8-
// RUN: not %clang_cl /FI\\?\%t.dir\FOO.h /WX -fsyntax-only %s 2>&1 | FileCheck %s
14+
// RUN: not %clang_cl /FI \\?\%{t:real}.dir\FOO.h /WX -fsyntax-only %s 2>&1 | FileCheck %s
915

1016
// CHECK: non-portable path to file '"\\?\

‎llvm/cmake/modules/AddLLVM.cmake‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1711,10 +1711,15 @@ endfunction()
17111711
# use it and can't be in a lit module. Use with make_paths_relative().
17121712
string(CONCAT LLVM_LIT_PATH_FUNCTION
17131713
"# Allow generated file to be relocatable.\n"
1714-
"from pathlib import Path\n"
1714+
"import os\n"
1715+
"import platform\n"
17151716
"def path(p):\n"
17161717
" if not p: return ''\n"
1717-
" return str((Path(__file__).parent / p).resolve())\n"
1718+
" # Follows lit.util.abs_path_preserve_drive, which cannot be imported here.\n"
1719+
" if platform.system() == 'Windows':\n"
1720+
" return os.path.abspath(os.path.join(os.path.dirname(__file__), p))\n"
1721+
" else:\n"
1722+
" return os.path.realpath(os.path.join(os.path.dirname(__file__), p))\n"
17181723
)
17191724

17201725
# This function provides an automatic way to 'configure'-like generate a file

‎llvm/docs/CommandGuide/lit.rst‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -541,6 +541,16 @@ TestRunner.py:
541541
%/p %p but ``\`` is replaced by ``/``
542542
%/t %t but ``\`` is replaced by ``/``
543543
%/T %T but ``\`` is replaced by ``/``
544+
%{s:real} %s after expanding all symbolic links and substitute drives
545+
%{S:real} %S after expanding all symbolic links and substitute drives
546+
%{p:real} %p after expanding all symbolic links and substitute drives
547+
%{t:real} %t after expanding all symbolic links and substitute drives
548+
%{T:real} %T after expanding all symbolic links and substitute drives
549+
%{/s:real} %/s after expanding all symbolic links and substitute drives
550+
%{/S:real} %/S after expanding all symbolic links and substitute drives
551+
%{/p:real} %/p after expanding all symbolic links and substitute drives
552+
%{/t:real} %/t after expanding all symbolic links and substitute drives
553+
%{/T:real} %/T after expanding all symbolic links and substitute drives
544554
%{/s:regex_replacement} %/s but escaped for use in the replacement of a ``s@@@`` command in sed
545555
%{/S:regex_replacement} %/S but escaped for use in the replacement of a ``s@@@`` command in sed
546556
%{/p:regex_replacement} %/p but escaped for use in the replacement of a ``s@@@`` command in sed

‎llvm/docs/TestingGuide.rst‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -671,7 +671,7 @@ RUN lines:
671671
``${fs-sep}``
672672
Expands to the file system separator, i.e. ``/`` or ``\`` on Windows.
673673

674-
``%/s, %/S, %/t, %/T:``
674+
``%/s, %/S, %/t, %/T``
675675

676676
Act like the corresponding substitution above but replace any ``\``
677677
character with a ``/``. This is useful to normalize path separators.
@@ -680,7 +680,17 @@ RUN lines:
680680

681681
Example: ``%/s: C:/Desktop Files/foo_test.s.tmp``
682682

683-
``%:s, %:S, %:t, %:T:``
683+
``%{s:real}, %{S:real}, %{t:real}, %{T:real}``
684+
``%{/s:real}, %{/S:real}, %{/t:real}, %{/T:real}``
685+
686+
Act like the corresponding substitution, including with ``/``, but use
687+
the real path by expanding all symbolic links and substitute drives.
688+
689+
Example: ``%s: S:\foo_test.s.tmp``
690+
691+
Example: ``%{/s:real}: C:/SDrive/foo_test.s.tmp``
692+
693+
``%:s, %:S, %:t, %:T``
684694

685695
Act like the corresponding substitution above but remove colons at
686696
the beginning of Windows paths. This is useful to allow concatenation

‎llvm/utils/lit/lit/TestRunner.py‎

Lines changed: 40 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,7 @@ def change_dir(self, newdir):
7474
if os.path.isabs(newdir):
7575
self.cwd = newdir
7676
else:
77-
self.cwd = os.path.realpath(os.path.join(self.cwd, newdir))
77+
self.cwd = lit.util.abs_path_preserve_drive(os.path.join(self.cwd, newdir))
7878

7979

8080
class TimeoutHelper(object):
@@ -427,7 +427,7 @@ def executeBuiltinMkdir(cmd, cmd_shenv):
427427
dir = to_unicode(dir) if kIsWindows else to_bytes(dir)
428428
cwd = to_unicode(cwd) if kIsWindows else to_bytes(cwd)
429429
if not os.path.isabs(dir):
430-
dir = os.path.realpath(os.path.join(cwd, dir))
430+
dir = lit.util.abs_path_preserve_drive(os.path.join(cwd, dir))
431431
if parent:
432432
lit.util.mkdir_p(dir)
433433
else:
@@ -473,7 +473,7 @@ def on_rm_error(func, path, exc_info):
473473
path = to_unicode(path) if kIsWindows else to_bytes(path)
474474
cwd = to_unicode(cwd) if kIsWindows else to_bytes(cwd)
475475
if not os.path.isabs(path):
476-
path = os.path.realpath(os.path.join(cwd, path))
476+
path = lit.util.abs_path_preserve_drive(os.path.join(cwd, path))
477477
if force and not os.path.exists(path):
478478
continue
479479
try:
@@ -1243,18 +1243,10 @@ def getDefaultSubstitutions(test, tmpDir, tmpBase, normalize_slashes=False):
12431243
substitutions.extend(test.config.substitutions)
12441244
tmpName = tmpBase + ".tmp"
12451245
baseName = os.path.basename(tmpBase)
1246-
substitutions.extend(
1247-
[
1248-
("%s", sourcepath),
1249-
("%S", sourcedir),
1250-
("%p", sourcedir),
1251-
("%{pathsep}", os.pathsep),
1252-
("%t", tmpName),
1253-
("%basename_t", baseName),
1254-
("%T", tmpDir),
1255-
]
1256-
)
12571246

1247+
substitutions.append(("%{pathsep}", os.pathsep))
1248+
substitutions.append(("%basename_t", baseName))
1249+
12581250
substitutions.extend(
12591251
[
12601252
("%{fs-src-root}", pathlib.Path(sourcedir).anchor),
@@ -1263,49 +1255,47 @@ def getDefaultSubstitutions(test, tmpDir, tmpBase, normalize_slashes=False):
12631255
]
12641256
)
12651257

1266-
# "%/[STpst]" should be normalized.
1267-
substitutions.extend(
1268-
[
1269-
("%/s", sourcepath.replace("\\", "/")),
1270-
("%/S", sourcedir.replace("\\", "/")),
1271-
("%/p", sourcedir.replace("\\", "/")),
1272-
("%/t", tmpBase.replace("\\", "/") + ".tmp"),
1273-
("%/T", tmpDir.replace("\\", "/")),
1274-
("%/et", tmpName.replace("\\", "\\\\\\\\\\\\\\\\")),
1275-
]
1276-
)
1258+
substitutions.append(("%/et", tmpName.replace("\\", "\\\\\\\\\\\\\\\\")))
12771259

1278-
# "%{/[STpst]:regex_replacement}" should be normalized like "%/[STpst]" but we're
1279-
# also in a regex replacement context of a s@@@ regex.
12801260
def regex_escape(s):
12811261
s = s.replace("@", r"\@")
12821262
s = s.replace("&", r"\&")
12831263
return s
12841264

1285-
substitutions.extend(
1286-
[
1287-
("%{/s:regex_replacement}", regex_escape(sourcepath.replace("\\", "/"))),
1288-
("%{/S:regex_replacement}", regex_escape(sourcedir.replace("\\", "/"))),
1289-
("%{/p:regex_replacement}", regex_escape(sourcedir.replace("\\", "/"))),
1290-
(
1291-
"%{/t:regex_replacement}",
1292-
regex_escape(tmpBase.replace("\\", "/")) + ".tmp",
1293-
),
1294-
("%{/T:regex_replacement}", regex_escape(tmpDir.replace("\\", "/"))),
1295-
]
1296-
)
1265+
path_substitutions = [
1266+
("s", sourcepath), ("S", sourcedir), ("p", sourcedir),
1267+
("t", tmpName), ("T", tmpDir)
1268+
]
1269+
for path_substitution in path_substitutions:
1270+
letter = path_substitution[0]
1271+
path = path_substitution[1]
1272+
1273+
# Original path variant
1274+
substitutions.append(("%" + letter, path))
1275+
1276+
# Normalized path separator variant
1277+
substitutions.append(("%/" + letter, path.replace("\\", "/")))
1278+
1279+
# realpath variants
1280+
# Windows paths with substitute drives are not expanded by default
1281+
# as they are used to avoid MAX_PATH issues, but sometimes we do
1282+
# need the fully expanded path.
1283+
real_path = os.path.realpath(path)
1284+
substitutions.append(("%{" + letter + ":real}", real_path))
1285+
substitutions.append(("%{/" + letter + ":real}",
1286+
real_path.replace("\\", "/")))
1287+
1288+
# "%{/[STpst]:regex_replacement}" should be normalized like
1289+
# "%/[STpst]" but we're also in a regex replacement context
1290+
# of a s@@@ regex.
1291+
substitutions.append(
1292+
("%{/" + letter + ":regex_replacement}",
1293+
regex_escape(path.replace("\\", "/"))))
1294+
1295+
# "%:[STpst]" are normalized paths without colons and without
1296+
# a leading slash.
1297+
substitutions.append(("%:" + letter, colonNormalizePath(path)))
12971298

1298-
# "%:[STpst]" are normalized paths without colons and without a leading
1299-
# slash.
1300-
substitutions.extend(
1301-
[
1302-
("%:s", colonNormalizePath(sourcepath)),
1303-
("%:S", colonNormalizePath(sourcedir)),
1304-
("%:p", colonNormalizePath(sourcedir)),
1305-
("%:t", colonNormalizePath(tmpBase + ".tmp")),
1306-
("%:T", colonNormalizePath(tmpDir)),
1307-
]
1308-
)
13091299
return substitutions
13101300

13111301

‎llvm/utils/lit/lit/builtin_commands/diff.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -281,7 +281,7 @@ def main(argv):
281281
try:
282282
for file in args:
283283
if file != "-" and not os.path.isabs(file):
284-
file = os.path.realpath(os.path.join(os.getcwd(), file))
284+
file = util.abs_path_preserve_drive(file)
285285

286286
if flags.recursive_diff:
287287
if file == "-":

‎llvm/utils/lit/lit/discovery.py‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
import sys
88

99
from lit.TestingConfig import TestingConfig
10-
from lit import LitConfig, Test
10+
from lit import LitConfig, Test, util
1111

1212

1313
def chooseConfigFileFromDir(dir, config_names):
@@ -56,8 +56,8 @@ def search1(path):
5656
# configuration to load instead.
5757
config_map = litConfig.params.get("config_map")
5858
if config_map:
59-
cfgpath = os.path.realpath(cfgpath)
60-
target = config_map.get(os.path.normcase(cfgpath))
59+
cfgpath = util.abs_path_preserve_drive(cfgpath)
60+
target = config_map.get(cfgpath)
6161
if target:
6262
cfgpath = target
6363

@@ -67,13 +67,13 @@ def search1(path):
6767

6868
cfg = TestingConfig.fromdefaults(litConfig)
6969
cfg.load_from_path(cfgpath, litConfig)
70-
source_root = os.path.realpath(cfg.test_source_root or path)
71-
exec_root = os.path.realpath(cfg.test_exec_root or path)
70+
source_root = util.abs_path_preserve_drive(cfg.test_source_root or path)
71+
exec_root = util.abs_path_preserve_drive(cfg.test_exec_root or path)
7272
return Test.TestSuite(cfg.name, source_root, exec_root, cfg), ()
7373

7474
def search(path):
7575
# Check for an already instantiated test suite.
76-
real_path = os.path.realpath(path)
76+
real_path = util.abs_path_preserve_drive(path)
7777
res = cache.get(real_path)
7878
if res is None:
7979
cache[real_path] = res = search1(path)

0 commit comments

Comments
 (0)