Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Support] Resolve symlinks in getMainExecutable() on Windows #76304

Merged
merged 3 commits into from
Dec 26, 2023

Conversation

aganea
Copy link
Member

@aganea aganea commented Dec 24, 2023

This makes the Windows implementation for getMainExecutable() behave the same as its Linux counterpart, in regards to symlinks. Previously, when using cmake ... -DLLVM_USE_SYMLINKS=ON, calling this function wouldn't resolve to the "real", non-symlinked path.

This makes the Windows implementation for `getMainExecutable()` behave the same as its Linux counterpart, in regards to symlinks. Previously, when using `cmake ... -DLLVM_USE_SYMLINKS=ON`, calling this function wouldn't resolve to the "real", non-symlinked path.
@llvmbot
Copy link
Collaborator

llvmbot commented Dec 24, 2023

@llvm/pr-subscribers-platform-windows

Author: Alexandre Ganea (aganea)

Changes

This makes the Windows implementation for getMainExecutable() behave the same as its Linux counterpart, in regards to symlinks. Previously, when using cmake ... -DLLVM_USE_SYMLINKS=ON, calling this function wouldn't resolve to the "real", non-symlinked path.


Full diff: https://github.com/llvm/llvm-project/pull/76304.diff

1 Files Affected:

  • (modified) llvm/lib/Support/Windows/Path.inc (+4-1)
diff --git a/llvm/lib/Support/Windows/Path.inc b/llvm/lib/Support/Windows/Path.inc
index 168a63bb2d969d..6b50309be94d77 100644
--- a/llvm/lib/Support/Windows/Path.inc
+++ b/llvm/lib/Support/Windows/Path.inc
@@ -154,7 +154,10 @@ std::string getMainExecutable(const char *argv0, void *MainExecAddr) {
     return "";
 
   llvm::sys::path::make_preferred(PathNameUTF8);
-  return std::string(PathNameUTF8.data());
+
+  SmallString<256> RealPath;
+  sys::fs::real_path(PathNameUTF8, RealPath);
+  return (std::string)RealPath;
 }
 
 UniqueID file_status::getUniqueID() const {

@llvmbot
Copy link
Collaborator

llvmbot commented Dec 24, 2023

@llvm/pr-subscribers-llvm-support

Author: Alexandre Ganea (aganea)

Changes

This makes the Windows implementation for getMainExecutable() behave the same as its Linux counterpart, in regards to symlinks. Previously, when using cmake ... -DLLVM_USE_SYMLINKS=ON, calling this function wouldn't resolve to the "real", non-symlinked path.


Full diff: https://github.com/llvm/llvm-project/pull/76304.diff

1 Files Affected:

  • (modified) llvm/lib/Support/Windows/Path.inc (+4-1)
diff --git a/llvm/lib/Support/Windows/Path.inc b/llvm/lib/Support/Windows/Path.inc
index 168a63bb2d969d..6b50309be94d77 100644
--- a/llvm/lib/Support/Windows/Path.inc
+++ b/llvm/lib/Support/Windows/Path.inc
@@ -154,7 +154,10 @@ std::string getMainExecutable(const char *argv0, void *MainExecAddr) {
     return "";
 
   llvm::sys::path::make_preferred(PathNameUTF8);
-  return std::string(PathNameUTF8.data());
+
+  SmallString<256> RealPath;
+  sys::fs::real_path(PathNameUTF8, RealPath);
+  return (std::string)RealPath;
 }
 
 UniqueID file_status::getUniqueID() const {

@aganea aganea requested a review from abrachet December 24, 2023 00:27
@compnerd
Copy link
Member

Seems reasonable at first glance. How should this behave with subst drives?

llvm/lib/Support/Windows/Path.inc Outdated Show resolved Hide resolved
@aganea
Copy link
Member Author

aganea commented Dec 24, 2023

Seems reasonable at first glance. How should this behave with subst drives?

Since we're using GetFinalPathNameByHandleW it will always resolve to the canonical, non-subst path. My understanding is that getMainExecutable() does that on Linux as well. On Windows it wasn't the case, since mostly likely nobody uses -DLLVM_USE_SYMLINKS=ON because of the admin rights.

C:\git\llvm-project>subst Y: stage1_msvc_debug

C:\git\llvm-project>Y:

Y:\>bin\clang-cl.exe /c C:\git\a.cpp -###
clang version 18.0.0git (https://github.com/aganea/llvm-project.git 51d89243cdc1239e79a2829d2735c3ebdade0fed)
Target: x86_64-pc-windows-msvc
Thread model: posix
InstalledDir: Y:\bin
 (in-process)
 "C:\\git\\llvm-project\\stage1_msvc_debug\\bin\\llvm.exe" "clang-cl.exe" "-cc1" "-triple" "x86_64-pc-windows-msvc19.38.33133" "-emit-obj" "-mrelax-all" "-mincremental-linker-compatible" "-disable-free" "-clear-ast-before-backend" "-main-file-name" "a.cpp" "-mrelocation-model" (rest omitted)

Copy link
Member Author

@aganea aganea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As suggested.

@aganea aganea requested a review from compnerd December 24, 2023 21:02
@aganea aganea requested a review from MaskRay December 25, 2023 13:55
@aganea
Copy link
Member Author

aganea commented Dec 26, 2023

@compnerd @MaskRay Thanks for the review!

@aganea aganea merged commit f11b056 into llvm:main Dec 26, 2023
3 of 4 checks passed
@aganea aganea deleted the fix_get_main_executable_win branch December 26, 2023 15:33
slydiman added a commit to slydiman/llvm-project that referenced this pull request Apr 5, 2024
…() failed

The commit f11b056 (llvm#76304) breaks `clang` and other tools if they are used from a RAMDrive.
`GetFinalPathNameByHandleW()` may return 0 and GetLastError 0x28. This patch fixes that issue.
Note `real_path()` uses `openFileForRead()` but it reports the error only if failed to open a file. Getting `RealPath` is optional functionality.

BTW, `sys::fs::real_path()` resolves not only symlinks, but also network drives and virtual drives created by the `subst` tool. It may break an automation. It is better to detect symlinks and resolve only symlinks.
slydiman added a commit that referenced this pull request Apr 5, 2024
…() failed (#87749)

The commit f11b056 (#76304) breaks `clang` and other tools if they are
used from a RAMDrive. `GetFinalPathNameByHandleW()` may return 0 and
GetLastError 0x28. This patch fixes that issue. Note `real_path()` uses
`openFileForRead()` but it reports the error only if failed to open a
file. Getting `RealPath` is optional functionality.

BTW, `sys::fs::real_path()` resolves not only symlinks, but also network
drives and virtual drives created by the `subst` tool. It may break an
automation. It is better to detect symlinks and resolve only symlinks.
@glandium
Copy link
Contributor

This has the unfortunate consequence that if you have llvm-config in a path with spaces and call it with the short path, the paths it will emit will contain spaces, when they didn't before.

That is, before:

$ mkdir -p "FOO BAR/bin"
$ cp llvm-config.exe "FOO BAR/bin"
$ ./FOOBAR~1/llvm-config.exe --bindir
(...)\FOOBAR~1\bin

But now:

$ ./FOOBAR~1/llvm-config.exe --bindir
(...)\FOO BAR\bin

It's not too much of a problem with --bindir, but with e.g. --libs, that means we now get a spaces-separated list of files separated by spaces, which doesn't quite work (which using the shortname before worked around).

@aganea
Copy link
Member Author

aganea commented Jun 11, 2024

@glandium It is a bit unfortunate but it seems the previous behavior was "by chance". Do you happen to know what happens on Linux under the same conditions? (path with spaces or symlink without spaces pointing to a path with spaces) I could send a PR to add sys::printArg() where needed, if that sounds good to you?

@glandium
Copy link
Contributor

It probably has the "contains spaces" problem on Linux. I guess sys::printArg would make sense for everything llvm-config prints out, but there's no saying whether things that consume its output will actually deal with it correctly (although, obviously, they're probably not handling things correctly anyways for the case sys::printArg would print something different).

@aganea
Copy link
Member Author

aganea commented Jun 11, 2024

How did you actually found this? What was your usage of llvm-config?

@glandium
Copy link
Contributor

It's not really relevant, but it's in the Firefox build system, using llvm-config --cxxflags and llvm-config --ldflags, and it wouldn't handle the quoting sys::printArg adds, but it's entirely our problem. I'm sure there are plenty of other scripts using llvm-config and not handling quoting properly (and not handling spaces properly today).

@glandium
Copy link
Contributor

llvm-config output not handling spaces is actually #28117

@aganea
Copy link
Member Author

aganea commented Jul 1, 2024

@glandium Please see #97305

aganea added a commit that referenced this pull request Jul 3, 2024
If any of the printed paths by `llvm-config` contains quotes, spaces,
backslashes or dollar sign characters, these paths will be quoted and
the corresponding characters will be escaped.

Following discussion in #76304

Fixes #28117
kirillpyasecky pushed a commit to kirillpyasecky/llvm-project that referenced this pull request Jul 3, 2024
If any of the printed paths by `llvm-config` contains quotes, spaces,
backslashes or dollar sign characters, these paths will be quoted and
the corresponding characters will be escaped.

Following discussion in llvm#76304

Fixes llvm#28117
kbluck pushed a commit to kbluck/llvm-project that referenced this pull request Jul 6, 2024
If any of the printed paths by `llvm-config` contains quotes, spaces,
backslashes or dollar sign characters, these paths will be quoted and
the corresponding characters will be escaped.

Following discussion in llvm#76304

Fixes llvm#28117
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants