https://github.com/inaki-amatria updated
https://github.com/llvm/llvm-project/pull/94544
From ed167da84dd9ad59a5b2c61bd237d041efa9d4fc Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?I=C3=B1aki=20Amatria=20Barral?= <inaki.amat...@appentra.com>
Date: Thu, 6 Jun 2024 00:52:30 +0200
Subject: [PATCH 1/2] [llvm][Support] Stop using PWD in current_path
In a filesystem structured as follows:
```
r/
|- a/
`- b -> a/
```
If current directory is set to `r/b` (a symbolic link to `r/a`) and then
changed to `r/a` using `set_current_path()`, subsequent calls to
`current_path()` incorrectly return `r/b`. This is because
`current_path()` reads the `PWD` environment variable, which still
points to the symbolic link `r/b` instead of the actual directory `r/a`.
This issue leads to inaccurate path reporting, causing confusion and
potential errors in applications that rely on `current_path()`.
This issue is reproducible with bind mounts too.
---
.../llvm/Testing/Support/SupportHelpers.h | 24 ++++++++++++++
llvm/lib/Support/Unix/Path.inc | 10 ------
llvm/test/MC/ELF/comp-dir.s | 8 -----
llvm/unittests/Support/Path.cpp | 32 +++----------------
.../Support/VirtualFileSystemTest.cpp | 17 ++++++++++
5 files changed, 46 insertions(+), 45 deletions(-)
diff --git a/llvm/include/llvm/Testing/Support/SupportHelpers.h
b/llvm/include/llvm/Testing/Support/SupportHelpers.h
index 95c2765027ba0..49e44e42a1dc1 100644
--- a/llvm/include/llvm/Testing/Support/SupportHelpers.h
+++ b/llvm/include/llvm/Testing/Support/SupportHelpers.h
@@ -248,6 +248,30 @@ class TempFile {
StringRef path() const { return Path; }
};
+#ifndef _WIN32
+// RAII helper to set and restore an environment variable.
+class WithEnv {
+ const char *Var;
+ std::optional<std::string> OriginalValue;
+
+public:
+ WithEnv(const char *Var, const char *Value) : Var(Var) {
+ if (const char *V = ::getenv(Var))
+ OriginalValue.emplace(V);
+ if (Value)
+ ::setenv(Var, Value, 1);
+ else
+ ::unsetenv(Var);
+ }
+ ~WithEnv() {
+ if (OriginalValue)
+ ::setenv(Var, OriginalValue->c_str(), 1);
+ else
+ ::unsetenv(Var);
+ }
+};
+#endif
+
} // namespace unittest
} // namespace llvm
diff --git a/llvm/lib/Support/Unix/Path.inc b/llvm/lib/Support/Unix/Path.inc
index 6e679f74869f0..01af15ebfa52b 100644
--- a/llvm/lib/Support/Unix/Path.inc
+++ b/llvm/lib/Support/Unix/Path.inc
@@ -369,16 +369,6 @@ ErrorOr<space_info> disk_space(const Twine &Path) {
std::error_code current_path(SmallVectorImpl<char> &result) {
result.clear();
- const char *pwd = ::getenv("PWD");
- llvm::sys::fs::file_status PWDStatus, DotStatus;
- if (pwd && llvm::sys::path::is_absolute(pwd) &&
- !llvm::sys::fs::status(pwd, PWDStatus) &&
- !llvm::sys::fs::status(".", DotStatus) &&
- PWDStatus.getUniqueID() == DotStatus.getUniqueID()) {
- result.append(pwd, pwd + strlen(pwd));
- return std::error_code();
- }
-
result.resize_for_overwrite(PATH_MAX);
while (true) {
diff --git a/llvm/test/MC/ELF/comp-dir.s b/llvm/test/MC/ELF/comp-dir.s
index 59edce1ca5452..729f41e872781 100644
--- a/llvm/test/MC/ELF/comp-dir.s
+++ b/llvm/test/MC/ELF/comp-dir.s
@@ -6,13 +6,5 @@
// CHECK: DW_AT_comp_dir [DW_FORM_string] ("{{([A-Za-z]:.*)?}}/test/comp/dir")
-// RUN: mkdir -p %t.foo
-// RUN: ln -sf %t.foo %t.bar
-// RUN: cd %t.foo
-// RUN: env PWD=%t.bar llvm-mc -triple=x86_64-linux-unknown -g %s
-filetype=obj -o %t.o
-// RUN: llvm-dwarfdump -v -debug-info %t.o | FileCheck --check-prefix=PWD %s
-// PWD: DW_AT_comp_dir [DW_FORM_string] ("{{.*}}.bar")
-
-
f:
nop
diff --git a/llvm/unittests/Support/Path.cpp b/llvm/unittests/Support/Path.cpp
index bdae7a8ee4b55..02c34ea632aa4 100644
--- a/llvm/unittests/Support/Path.cpp
+++ b/llvm/unittests/Support/Path.cpp
@@ -451,28 +451,6 @@ std::string getEnvWin(const wchar_t *Var) {
}
return expected;
}
-#else
-// RAII helper to set and restore an environment variable.
-class WithEnv {
- const char *Var;
- std::optional<std::string> OriginalValue;
-
-public:
- WithEnv(const char *Var, const char *Value) : Var(Var) {
- if (const char *V = ::getenv(Var))
- OriginalValue.emplace(V);
- if (Value)
- ::setenv(Var, Value, 1);
- else
- ::unsetenv(Var);
- }
- ~WithEnv() {
- if (OriginalValue)
- ::setenv(Var, OriginalValue->c_str(), 1);
- else
- ::unsetenv(Var);
- }
-};
#endif
TEST(Support, HomeDirectory) {
@@ -496,7 +474,7 @@ TEST(Support, HomeDirectory) {
// Apple has their own solution for this.
#if defined(LLVM_ON_UNIX) && !defined(__APPLE__)
TEST(Support, HomeDirectoryWithNoEnv) {
- WithEnv Env("HOME", nullptr);
+ unittest::WithEnv Env("HOME", nullptr);
// Don't run the test if we have nothing to compare against.
struct passwd *pw = getpwuid(getuid());
@@ -510,7 +488,7 @@ TEST(Support, HomeDirectoryWithNoEnv) {
}
TEST(Support, ConfigDirectoryWithEnv) {
- WithEnv Env("XDG_CONFIG_HOME", "/xdg/config");
+ unittest::WithEnv Env("XDG_CONFIG_HOME", "/xdg/config");
SmallString<128> ConfigDir;
EXPECT_TRUE(path::user_config_directory(ConfigDir));
@@ -518,7 +496,7 @@ TEST(Support, ConfigDirectoryWithEnv) {
}
TEST(Support, ConfigDirectoryNoEnv) {
- WithEnv Env("XDG_CONFIG_HOME", nullptr);
+ unittest::WithEnv Env("XDG_CONFIG_HOME", nullptr);
SmallString<128> Fallback;
ASSERT_TRUE(path::home_directory(Fallback));
@@ -530,7 +508,7 @@ TEST(Support, ConfigDirectoryNoEnv) {
}
TEST(Support, CacheDirectoryWithEnv) {
- WithEnv Env("XDG_CACHE_HOME", "/xdg/cache");
+ unittest::WithEnv Env("XDG_CACHE_HOME", "/xdg/cache");
SmallString<128> CacheDir;
EXPECT_TRUE(path::cache_directory(CacheDir));
@@ -538,7 +516,7 @@ TEST(Support, CacheDirectoryWithEnv) {
}
TEST(Support, CacheDirectoryNoEnv) {
- WithEnv Env("XDG_CACHE_HOME", nullptr);
+ unittest::WithEnv Env("XDG_CACHE_HOME", nullptr);
SmallString<128> Fallback;
ASSERT_TRUE(path::home_directory(Fallback));
diff --git a/llvm/unittests/Support/VirtualFileSystemTest.cpp
b/llvm/unittests/Support/VirtualFileSystemTest.cpp
index e9fd9671ea6ab..5d9386ba347dd 100644
--- a/llvm/unittests/Support/VirtualFileSystemTest.cpp
+++ b/llvm/unittests/Support/VirtualFileSystemTest.cpp
@@ -479,6 +479,23 @@ TEST(VirtualFileSystemTest, BasicRealFSIteration) {
}
#ifdef LLVM_ON_UNIX
+TEST(VirtualFileSystemTest, ChangeCWDToDirFromASymbolicLinkToDir) {
+ // r/
+ // |- a/
+ // `- b -> a/
+ TempDir Root("r", /*Unique=*/true);
+ TempDir ADir(Root.path("a"));
+ TempLink B(ADir.path(), Root.path("b"));
+
+ // PWD points to `/r/b`
+ unittest::WithEnv Env("PWD", B.path().str().c_str());
+
+ // Navigate to `/r/a` from `/r/b`
+ IntrusiveRefCntPtr<vfs::FileSystem> FS = vfs::getRealFileSystem();
+ ASSERT_FALSE(FS->setCurrentWorkingDirectory(ADir.path()));
+ EXPECT_EQ(ADir.path(), *FS->getCurrentWorkingDirectory());
+}
+
TEST(VirtualFileSystemTest, MultipleWorkingDirs) {
// Our root contains a/aa, b/bb, c, where c is a link to a/.
// Run tests both in root/b/ and root/c/ (to test "normal" and symlink dirs).
From a2898cda42f658394c94a06226fa6bb89da863be Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?I=C3=B1aki=20Amatria=20Barral?= <inaki.amat...@appentra.com>
Date: Tue, 11 Jun 2024 18:16:13 +0200
Subject: [PATCH 2/2] fixup! [llvm][Support] Stop using PWD in current_path
---
clang/test/Driver/coverage.c | 9 ---------
clang/test/Tooling/clang-check-pwd.cpp | 14 --------------
2 files changed, 23 deletions(-)
delete mode 100644 clang/test/Tooling/clang-check-pwd.cpp
diff --git a/clang/test/Driver/coverage.c b/clang/test/Driver/coverage.c
index e5ed064aab457..ccafbdc19529e 100644
--- a/clang/test/Driver/coverage.c
+++ b/clang/test/Driver/coverage.c
@@ -17,10 +17,6 @@
// GCNO-LOCATION: "-coverage-notes-file={{.*}}/foo/bar.gcno"
// GCNO-LOCATION-REL: "-coverage-notes-file={{.*}}{{/|\\\\}}foo/bar.gcno"
-/// GCC allows PWD to change the paths.
-// RUN: %if system-linux %{ PWD=/proc/self/cwd %clang -### -c --coverage %s -o
foo/bar.o 2>&1 | FileCheck --check-prefix=PWD %s %}
-// PWD: "-coverage-notes-file=/proc/self/cwd/foo/bar.gcno"
"-coverage-data-file=/proc/self/cwd/foo/bar.gcda"
-
/// Don't warn -Wunused-command-line-argument.
// RUN: %clang -E -Werror --coverage -ftest-coverage -fprofile-arcs %s
@@ -48,8 +44,3 @@
// RUN: %clang -### --coverage d/a.c d/b.c -o e/x -dumpdir f/g 2>&1 |
FileCheck %s --check-prefix=LINK2
// LINK2: -cc1{{.*}} "-coverage-notes-file={{.*}}{{/|\\\\}}f/ga.gcno"
"-coverage-data-file={{.*}}{{/|\\\\}}f/ga.gcda"
// LINK2: -cc1{{.*}} "-coverage-notes-file={{.*}}{{/|\\\\}}f/gb.gcno"
"-coverage-data-file={{.*}}{{/|\\\\}}f/gb.gcda"
-
-/// GCC allows PWD to change the paths.
-// RUN: %if system-linux %{ PWD=/proc/self/cwd %clang -### --coverage d/a.c
d/b.c -o e/x -fprofile-dir=f 2>&1 | FileCheck %s --check-prefix=LINK3 %}
-// LINK3: -cc1{{.*}} "-coverage-notes-file=/proc/self/cwd/e/x-a.gcno"
"-coverage-data-file=f/proc/self/cwd/e/x-a.gcda"
-// LINK3: -cc1{{.*}} "-coverage-notes-file=/proc/self/cwd/e/x-b.gcno"
"-coverage-data-file=f/proc/self/cwd/e/x-b.gcda"
diff --git a/clang/test/Tooling/clang-check-pwd.cpp
b/clang/test/Tooling/clang-check-pwd.cpp
deleted file mode 100644
index 2e8d4a3fe12b6..0000000000000
--- a/clang/test/Tooling/clang-check-pwd.cpp
+++ /dev/null
@@ -1,14 +0,0 @@
-// RUN: rm -rf %t
-// RUN: mkdir %t
-// RUN: echo "[{\"directory\":\".\",\"command\":\"clang++ -c
%t/test.cpp\",\"file\":\"%t/test.cpp\"}]" | sed -e 's/\\/\\\\/g' >
%t/compile_commands.json
-// RUN: cp "%s" "%t/test.cpp"
-// RUN: ln -sf %t %t.foobar
-// RUN: cd %t
-// RUN: env PWD="%t.foobar" not clang-check -p "%t" "test.cpp" 2>&1|FileCheck
%s
-// FIXME: Make the above easier.
-
-// CHECK: a type specifier is required
-// CHECK: .foobar/test.cpp
-invalid;
-
-// REQUIRES: shell
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
