Skip to content

Conversation

@slydiman
Copy link
Contributor

@slydiman slydiman commented Apr 5, 2024

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.

…() 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 slydiman added the bug Indicates an unexpected problem or unintended behavior label Apr 5, 2024
@slydiman slydiman requested a review from MaskRay April 5, 2024 07:16
@llvmbot
Copy link
Member

llvmbot commented Apr 5, 2024

@llvm/pr-subscribers-llvm-support

@llvm/pr-subscribers-platform-windows

Author: Dmitry Vasilyev (slydiman)

Changes

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.


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

1 Files Affected:

  • (modified) llvm/lib/Support/Windows/Path.inc (+3-1)
diff --git a/llvm/lib/Support/Windows/Path.inc b/llvm/lib/Support/Windows/Path.inc
index 854d531ab371fc..4f0336a85daaa5 100644
--- a/llvm/lib/Support/Windows/Path.inc
+++ b/llvm/lib/Support/Windows/Path.inc
@@ -157,7 +157,9 @@ std::string getMainExecutable(const char *argv0, void *MainExecAddr) {
 
   SmallString<256> RealPath;
   sys::fs::real_path(PathNameUTF8, RealPath);
-  return std::string(RealPath);
+  if (RealPath.size())
+    return std::string(RealPath);
+  return std::string(PathNameUTF8.data());
 }
 
 UniqueID file_status::getUniqueID() const {

@mstorsjo mstorsjo requested a review from aganea April 5, 2024 07:30
Copy link
Member

@mstorsjo mstorsjo left a comment

Choose a reason for hiding this comment

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

This change looks reasonable to me, but I want to loop in @aganea before landing it.

@aganea
Copy link
Member

aganea commented Apr 5, 2024

LGTM, thanks for the fix!

If you want to avoid resolution with network drives or virtuals substs, you can also use clang … -no-canonical-paths I think, that should avoid calling this function.

@slydiman slydiman merged commit 225e14e into llvm:main Apr 5, 2024
@slydiman slydiman deleted the fix-empty-real_path branch April 5, 2024 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Indicates an unexpected problem or unintended behavior llvm:support platform:windows

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants