[llvm-windres] Resolve the --preprocessor executable in $PATH

[mstorsjo]

The llvm::sys::ExecuteAndWait function doesn't resolve the file to be executed from $PATH - i.e. it is similar to execv(), not execvp().

Due to this, specifying a --preprocessor argument to llvm-windres only worked if it specified an absolute path to the preprocessor executable. This was observed as one of the issues in msys2/MINGW-packages#19157.

Before d2fa6b6, this usage of --preprocessor seemed to work, because the first argument of Args[] was ignored and llvm-windres just executed the autodetected clang executable regardless.

Also improve the error messages printed if preprocessing failed. (If the preprocessor executable was started but itself returned an error, we don't get any error string.)

[llvmbot]

@llvm/pr-subscribers-platform-windows

Author: Martin Storsjö (mstorsjo)

Changes

The llvm::sys::ExecuteAndWait function doesn't resolve the file to be executed from $PATH - i.e. it is similar to execv(), not execvp().

Due to this, specifying a --preprocessor argument to llvm-windres only worked if it specified an absolute path to the preprocessor executable. This was observed as one of the issues in msys2/MINGW-packages#19157.

Before d2fa6b6, this usage of --preprocessor seemed to work, because the first argument of Args[] was ignored and llvm-windres just executed the autodetected clang executable regardless.

Also improve the error messages printed if preprocessing failed. (If the preprocessor executable was started but itself returned an error, we don't get any error string.)

---

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

3 Files Affected:

• (modified) llvm/test/tools/llvm-rc/preproc.test (+1-1)
• (modified) llvm/test/tools/llvm-rc/windres-preproc.test (+16-2)
• (modified) llvm/tools/llvm-rc/llvm-rc.cpp (+17-4)

diff --git a/llvm/test/tools/llvm-rc/preproc.test b/llvm/test/tools/llvm-rc/preproc.test
index f55e5434dce360..7a6d8b3db03662 100644
--- a/llvm/test/tools/llvm-rc/preproc.test
+++ b/llvm/test/tools/llvm-rc/preproc.test
@@ -1,3 +1,3 @@
 ; RUN: llvm-rc -### -i%p "-DFOO1=\"foo bar\"" -UFOO2 -D FOO3 -- %p/Inputs/empty.rc | FileCheck %s
 
-; CHECK: {{^}} "clang" "--driver-mode=gcc" "-target" "{{.*}}-pc-windows-msvc-coff" "-E" "-xc" "-DRC_INVOKED" "{{.*}}empty.rc" "-o" "{{.*}}preproc-{{.*}}.rc" "-I" "{{.*}}" "-D" "FOO1=\"foo bar\"" "-U" "FOO2" "-D" "FOO3"{{$}}
+; CHECK: {{^}} "clang" "--driver-mode=gcc" "-target" "{{.*}}-pc-windows-msvc-coff" "-E" "-xc" "-DRC_INVOKED" "-I" "{{.*}}" "-D" "FOO1=\"foo bar\"" "-U" "FOO2" "-D" "FOO3" "{{.*}}empty.rc" "-o" "{{.*}}preproc-{{.*}}.rc"{{$}}
diff --git a/llvm/test/tools/llvm-rc/windres-preproc.test b/llvm/test/tools/llvm-rc/windres-preproc.test
index e55195b3a4d280..74e888614aa2b9 100644
--- a/llvm/test/tools/llvm-rc/windres-preproc.test
+++ b/llvm/test/tools/llvm-rc/windres-preproc.test
@@ -5,6 +5,20 @@
 
 ; RUN: llvm-windres -### --include-dir %p/incdir1 --include %p/incdir2 "-DFOO1=\\\"foo bar\\\"" -UFOO2 -D FOO3 --preprocessor-arg "-DFOO4=\\\"baz baz\\\"" -DFOO5=\"bar\" %p/Inputs/empty.rc %t.res | FileCheck %s --check-prefix=CHECK1
 ; RUN: llvm-windres -### --include-dir %p/incdir1 --include %p/incdir2 "-DFOO1=\"foo bar\"" -UFOO2 -D FOO3 --preprocessor-arg "-DFOO4=\"baz baz\"" "-DFOO5=bar" %p/Inputs/empty.rc %t.res --use-temp-file | FileCheck %s --check-prefix=CHECK1
-; CHECK1: {{^}} "clang" "--driver-mode=gcc" "-target" "{{.*}}-{{.*}}{{mingw32|windows-gnu}}" "-E" "-xc" "-DRC_INVOKED" "{{.*}}empty.rc" "-o" "{{.*}}preproc-{{.*}}.rc" "-I" "{{.*}}incdir1" "-I" "{{.*}}incdir2" "-D" "FOO1=\"foo bar\"" "-U" "FOO2" "-D" "FOO3" "-DFOO4=\"baz baz\"" "-D" "FOO5=bar"{{$}}
+; CHECK1: {{^}} "clang" "--driver-mode=gcc" "-target" "{{.*}}-{{.*}}{{mingw32|windows-gnu}}" "-E" "-xc" "-DRC_INVOKED" "-I" "{{.*}}incdir1" "-I" "{{.*}}incdir2" "-D" "FOO1=\"foo bar\"" "-U" "FOO2" "-D" "FOO3" "-DFOO4=\"baz baz\"" "-D" "FOO5=bar" "{{.*}}empty.rc" "-o" "{{.*}}preproc-{{.*}}.rc"{{$}}
 ; RUN: llvm-windres -### --preprocessor "i686-w64-mingw32-gcc -E -DFOO=\\\"foo\\ bar\\\"" %p/Inputs/empty.rc %t.res | FileCheck %s --check-prefix=CHECK2
-; CHECK2: {{^}} "i686-w64-mingw32-gcc" "-E" "-DFOO=\"foo bar\"" "{{.*}}empty.rc" "-o" "{{.*}}preproc-{{.*}}.rc"{{$}}
+; CHECK2: {{^}} "{{.*}}i686-w64-mingw32-gcc" "-E" "-DFOO=\"foo bar\"" "{{.*}}empty.rc" "-o" "{{.*}}preproc-{{.*}}.rc"{{$}}
+
+;; Test resolving the --preprocessor executable from PATH
+
+; RUN: rm -rf %t-bin/testbin
+; RUN: mkdir -p %t-bin/testbin
+; RUN: ln -s llvm-windres %t-bin/testbin/i686-w64-mingw32-gcc
+; RUN: env PATH=%t-bin/testbin llvm-windres -### --preprocessor i686-w64-mingw32-gcc --preprocessor-arg -E --preprocessor-arg -xc -DRC_INVOKED %p/Inputs/empty.rc %t.res | FileCheck %s --check-prefix=CHECK3
+; CHECK3: {{^}} "{{.*}}/testbin/i686-w64-mingw32-gcc" "-E" "-xc" "-D" "RC_INVOKED" "{{.*}}empty.rc" "-o" "{{.*}}preproc-{{.*}}.rc"{{$}}
+
+
+;; 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!
diff --git a/llvm/tools/llvm-rc/llvm-rc.cpp b/llvm/tools/llvm-rc/llvm-rc.cpp
index b955347f2a8646..27fb0309e0ee54 100644
--- a/llvm/tools/llvm-rc/llvm-rc.cpp
+++ b/llvm/tools/llvm-rc/llvm-rc.cpp
@@ -248,16 +248,23 @@ void preprocess(StringRef Src, StringRef Dst, const RcOptions &Opts,
   SmallVector<StringRef, 8> Args = {
       Clang, "--driver-mode=gcc", "-target", Opts.Triple, "-E",
       "-xc", "-DRC_INVOKED"};
+  std::string PreprocessorExecutable;
   if (!Opts.PreprocessCmd.empty()) {
     Args.clear();
     for (const auto &S : Opts.PreprocessCmd)
       Args.push_back(S);
+    if (!sys::fs::can_execute(Args[0])) {
+      if (auto P = sys::findProgramByName(Args[0])) {
+        PreprocessorExecutable = *P;
+        Args[0] = PreprocessorExecutable;
+      }
+    }
   }
+  for (const auto &S : Opts.PreprocessArgs)
+    Args.push_back(S);
   Args.push_back(Src);
   Args.push_back("-o");
   Args.push_back(Dst);
-  for (const auto &S : Opts.PreprocessArgs)
-    Args.push_back(S);
   if (Opts.PrintCmdAndExit || Opts.BeVerbose) {
     for (const auto &A : Args) {
       outs() << " ";
@@ -269,9 +276,15 @@ void preprocess(StringRef Src, StringRef Dst, const RcOptions &Opts,
   }
   // The llvm Support classes don't handle reading from stdout of a child
   // process; otherwise we could avoid using a temp file.
-  int Res = sys::ExecuteAndWait(Args[0], Args);
+  std::string ErrMsg;
+  int Res =
+      sys::ExecuteAndWait(Args[0], Args, /*Env=*/std::nullopt, /*Redirects=*/{},
+                          /*SecondsToWait=*/0, /*MemoryLimit=*/0, &ErrMsg);
   if (Res) {
-    fatalError("llvm-rc: Preprocessing failed.");
+    if (!ErrMsg.empty())
+      fatalError("llvm-rc: Preprocessing failed: " + ErrMsg);
+    else
+      fatalError("llvm-rc: Preprocessing failed.");
   }
 }
 

[mstorsjo]

This goes on top of #75389 - only the topmost commmit should be reviewed in this PR.

CC @mati865 @lazka @pbo-linaro