[PATCH] D100755: [llvm-rc] [3/4] Run clang to preprocess input files

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added a comment.

In D100755#2701842 <https://reviews.llvm.org/D100755#2701842>, @aganea wrote:

> I agree, a new option `--preprocessor-arg` could help there?

Yeah, that'd allow for more specific preprocessing customization. But I'd rather defer adding more new nonstandard options to a later patch.



================
Comment at: llvm/tools/llvm-rc/Opts.td:43
 
-def codepage : JS<"C", "Set the codepage used for input strings.">;
+def no_cpp : F<"no-cpp", "Don't try to preprocess the input file.">;
+
----------------
aganea wrote:
> I know `-no-cpp` is short, but in my mind 'cpp' sounds like C++ not C preprocessor. I'm wondering if `-no-preprocess`  wouldn't be clearer?
That might be a good idea.


================
Comment at: llvm/tools/llvm-rc/llvm-rc.cpp:98
+  if (!Parent.empty())
+    Path = sys::findProgramByName("clang", Parent);
+  if (!Path)
----------------
thakis wrote:
> if there's no clang but only clang-cl, it should try to find that. (pass --driver-mode=gcc to use only one set of options – but see below)
> 
> (we don't bundle clang on windows in chromium, only clang-cl. we don't currently use llvm-rc, but we might one day)
Ah, trying both sounds good. I'm in the opposite situation - I only bundle clang, not clang-cl, and I currently use llvm-rc. (But I probably wouldn't use this preprocessing logic as is anyway, as I need to use a mingw clang driver for finding the sysroot etc.)

What do you @thakis think - should we invoke it with `--driver-mode=cl` instead? If we allow passing more options through, it might matter, even if it doesn't right now.

(If running in that mode, we'd need to use `-Fi` instead of `-o` for the output though, and that complicates the following patch a bit.)


================
Comment at: llvm/tools/llvm-rc/llvm-rc.cpp:154
+      Args.push_back("-U");
+      break;
+    }
----------------
thakis wrote:
> Here's our chromium wrapper: https://source.chromium.org/chromium/chromium/src/+/master:build/toolchain/win/rc/rc.py
> 
> On Windows, /winsysroot support and possibly -imsvc support would be nice too.
Those sound useful - but I think I'd prefer to defer adding support for more nonstandard preprocessing options to a later patch.

What do you think of a generic `--preprocessor-arg` like in the windres frontend, which might be useful for @aganea?


================
Comment at: llvm/tools/llvm-rc/llvm-rc.cpp:206
+  std::string PreprocessedFile = InArgsInfo[0];
+  if (!InputArgs.hasArg(OPT_no_cpp) && !getenv("LLVM_RC_NO_CPP")) {
+    std::string OutFile = createTempFile("preproc", "rc");
----------------
thakis wrote:
> Do we need the env var?
We don't strictly need the env var; I intended it as a soft upgrade path for cases where we already preprocess outside of llvm-rc and might not find clang-cl (if we'd only look for that one). But if we look for both clang and clang-cl, that might be ok.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100755/new/

https://reviews.llvm.org/D100755