Skip to content

Support: Do not check if a file exists before executing #128821

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

Merged

Conversation

arsenm
Copy link
Contributor

@arsenm arsenm commented Feb 26, 2025

Let the actual syscall error if the file doesn't exist. This produces
a more standard "no such file or directory" phrasing of the error message,
and avoids an extra step.

The same antipattern appears in the windows code, we should probably
fix that one too.

Let the actual syscall error if the file doesn't exist. This produces
a more standard "no such file or directory" phrasing of the error message,
and avoids an extra step.

The same antipattern appears in the windows code, we should probably
fix that one too.
Copy link
Contributor Author

arsenm commented Feb 26, 2025

This stack of pull requests is managed by Graphite. Learn more about stacking.

@llvmbot
Copy link
Member

llvmbot commented Feb 26, 2025

@llvm/pr-subscribers-llvm-support

Author: Matt Arsenault (arsenm)

Changes

Let the actual syscall error if the file doesn't exist. This produces
a more standard "no such file or directory" phrasing of the error message,
and avoids an extra step.

The same antipattern appears in the windows code, we should probably
fix that one too.


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

2 Files Affected:

  • (modified) llvm/lib/Support/Unix/Program.inc (-7)
  • (modified) llvm/test/tools/llvm-rc/windres-preproc.test (+1-1)
diff --git a/llvm/lib/Support/Unix/Program.inc b/llvm/lib/Support/Unix/Program.inc
index 0708df1eed0a3..6d68369ad191c 100644
--- a/llvm/lib/Support/Unix/Program.inc
+++ b/llvm/lib/Support/Unix/Program.inc
@@ -168,13 +168,6 @@ static bool Execute(ProcessInfo &PI, StringRef Program,
                     ArrayRef<std::optional<StringRef>> Redirects,
                     unsigned MemoryLimit, std::string *ErrMsg,
                     BitVector *AffinityMask, bool DetachProcess) {
-  if (!llvm::sys::fs::exists(Program)) {
-    if (ErrMsg)
-      *ErrMsg = std::string("Executable \"") + Program.str() +
-                std::string("\" doesn't exist!");
-    return false;
-  }
-
   assert(!AffinityMask && "Starting a process with an affinity mask is "
                           "currently not supported on Unix!");
 
diff --git a/llvm/test/tools/llvm-rc/windres-preproc.test b/llvm/test/tools/llvm-rc/windres-preproc.test
index 52427862e760b..ad558c9a0c927 100644
--- a/llvm/test/tools/llvm-rc/windres-preproc.test
+++ b/llvm/test/tools/llvm-rc/windres-preproc.test
@@ -20,7 +20,7 @@
 ;; Test error messages when unable to execute the preprocessor.
 
 ; RUN: not llvm-windres --preprocessor intentionally-missing-executable %p/Inputs/empty.rc %t.res 2>&1 | FileCheck %s --check-prefix=CHECK4
-; CHECK4: llvm-rc: Preprocessing failed: Executable "intentionally-missing-executable" doesn't exist!
+; CHECK4: llvm-rc: Preprocessing failed: posix_spawn failed: No such file or directory
 
 ;; Test --preprocessor with an argument with spaces.
 

@arsenm arsenm marked this pull request as ready for review February 26, 2025 05:31
@arsenm arsenm merged commit 8dd6095 into main Feb 26, 2025
11 checks passed
@arsenm arsenm deleted the users/arsenm/support/stop-checking-file-exists-before-exec branch February 26, 2025 06:51
@mark-sed
Copy link

@arsenm The message check should probably be more general as our machine generates a different one. I have created an issue for this: #129208

@daltenty
Copy link
Member

This seems to be encountering problems on AIX as well:
https://lab.llvm.org/buildbot/#/builders/64/builds/2371

@arsenm
Copy link
Contributor Author

arsenm commented Mar 1, 2025

This seems to be encountering problems on AIX as well: https://lab.llvm.org/buildbot/#/builders/64/builds/2371

Should also be fixed by #129252

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.

5 participants