Page MenuHomePhabricator

[llvm-rc] [3/4] Run clang to preprocess input files
ClosedPublic

Authored by mstorsjo on Apr 19 2021, 5:21 AM.

Details

Summary

Allow opting out from preprocessing with a command line argument and
an environment variable. (Not all existing users of llvm-rc might have
clang available, theoretically at least.)

Update tests to pass -no-cpp (and one case of LLVM_RC_NO_CPP) to
make it not try to use clang (which isn't a build level dependency
of llvm-rc).

Update a few options to allow them both joined (as -DFOO) and separate
(-D BR), as rc.exe allows both forms of them.

With the verbose flag set, this prints the preprocessing command
used (which differs from what rc.exe does).

This uses 'clang' for preprocessing, instead of 'clang-cl'; for this
particular case, 'clang-cl' might be marginally easier, but using
'clang' simplifies the following patch. The 'clang' executable is
probably also available in a few more cases than 'clang-cl'.

As we can't rely on clang to exist when running these tests, this only
tests what kind of command lines are constructed.

Diff Detail

Event Timeline

mstorsjo requested review of this revision.Apr 19 2021, 5:21 AM
mstorsjo created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptApr 19 2021, 5:21 AM
rnk resigned from this revision.Apr 19 2021, 1:49 PM
rnk added a subscriber: epastor.

I think I need to get out of the review loop for RC changes, but this is the direction we agreed on. @epastor has been working on llvm-ml. If he has time and interest in reviewing changes to llvm-rc, I would encourage you to use him as a reviewer.

mstorsjo added inline comments.Apr 19 2021, 2:05 PM
llvm/tools/llvm-rc/llvm-rc.cpp
90

I think I'll rewrite this to not use the llvm::sys::TempFile class, but instead just use llvm::sys::createUniqueFile() instead, as the persistent open TempFile seems to be causing issues when running on actual windows, see the CI failures on D100756.

I'm not quite clear on the motivation. It's hard for me to imagine the resource compiler being useful if you cannot use the C preprocessor. I guess for simple tests you don't need it, is that worth adding an option?

On the technical side, yeah, keeping the temp file open could be problematic. Once you fix that, I don't see anything wrong with this patch.

I'm not quite clear on the motivation. It's hard for me to imagine the resource compiler being useful if you cannot use the C preprocessor. I guess for simple tests you don't need it, is that worth adding an option?

It's mostly due to the layering issue; when running check-llvm one doesn't necessarily have clang available (and I guess it's not ok to add such a dependency). That unfortunately makes the actual invocation of the subprocess untested by the unit tests.

Secondly, as llvm-rc currently doesn't rely on clang, one could concievably have setups that use llvm-rc without clang available, and would want to keep it that way at least initially. (E.g. my current windres wrapper preprocesses externally and then calls llvm-rc. It does have 'clang' available, but not e.g. 'clang-cl'. With patch 4/4 in place I don't need my external wrapper - but it'd still be used while transitioning to this setup.)

Ah, I get it. Thanks for the clarification.

mstorsjo updated this revision to Diff 338782.Apr 20 2021, 2:38 AM

Using createTemporaryFile instead of the TempFile class, avoiding keeping the temp file open twice while using it.

Thanks for adding this Martin!

I'd really like to see a simple test running from the Clang side as well! Like calling llvm-rc with a file that requires preprocessing:

// foo.rc
#include "resource.h"
IDI_MY_ICON       ICON           "my.ico"

with

// resource.h
#define IDI_MY_ICON               101

Also just for reference purposes, I'm pasting the little script I'm currently using to replace rc.exe on Linux. I've put it in the same folder as clang-cl and llvm-rc and now I'm able to use the same cmd-line as for rc.exe (it goes through a temp file which isn't ideal):

diff --git a/llvm/tools/llvm-rc/rc.sh b/llvm/tools/llvm-rc/rc.sh
new file mode 100755
index 000000000000..4a16f020aad8
--- /dev/null
+++ b/llvm/tools/llvm-rc/rc.sh
@@ -0,0 +1,34 @@
+#!/bin/bash
+
+INCLUDES=
+for flag in "$@" 
+do
+        if [[ $flag =~ /Fo(.+) ]] ;
+	then
+		OUTFILE=${BASH_REMATCH[1]}
+		OUTFLAG=$flag
+	fi
+	if [[ $flag =~ .+\.rc ]] ;
+	then
+		RCFILE=$flag
+		RCPATH=$(dirname "${flag}")
+	fi
+	if [[ $flag =~ /I ]] ;
+	then
+		INCLUDES="$INCLUDES $flag"
+	fi
+	if [[ $flag == /nologo ]] ;
+	then
+		NOLOGO=$flag
+	fi
+done
+if [[ $* =~ (/l[[:space:]]0x[[:alnum:]]+) ]] ;
+then
+	LANGUAGE="${BASH_REMATCH[1]}"
+fi
+
+SCRIPTPATH=$(dirname "$0")
+
+$SCRIPTPATH/clang-cl /E ${RCFILE} ${INCLUDES} -nostdinc -Wno-nonportable-include-path > "${OUTFILE}.i"
+$SCRIPTPATH/llvm-rc "${OUTFILE}.i" ${OUTFLAG} ${LANGUAGE} ${NOLOGO} /I${RCPATH}
+
llvm/tools/llvm-rc/Opts.td
43–46

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?

llvm/tools/llvm-rc/llvm-rc.cpp
133

I'm wondering if you could add --driver-mode=cl ? We're using platform includes and/or VC includes in our .res files. How would that work out?

For example, we're including these:

C:\.nuget\dd\vs2019_buildtools.16.9.1\tools\VC\Tools\MSVC\14.28.29910\atlmfc\include\afxres.h
C:\.nuget\dd\win-sdk.10.19041.1\tools\10\Include\10.0.19041.0\um\winresrc.h

In our case they are consumed from nuget packages and thus we're adding -nostdinc as well to the clang-cl invocation, before calling llvm-rc with the output. But I'm more concerned about any options or #define(s) that the CL driver defines, that stock clang.exe does not? You wouldn't have to fiddle with the triple if you did that I think.

167

Couldn't we fold the child process with -fintegrated-cc1? Life would be much better without temp files.

Thanks for adding this Martin!

I'd really like to see a simple test running from the Clang side as well! Like calling llvm-rc with a file that requires preprocessing:

That sounds like a good idea, as there's indeed a gap in testing at that spot right now.

What would be the most appropriate place under clang/test for it?


Also just for reference purposes, I'm pasting the little script I'm currently using to replace rc.exe on Linux. I've put it in the same folder as clang-cl and llvm-rc and now I'm able to use the same cmd-line as for rc.exe (it goes through a temp file which isn't ideal):

diff --git a/llvm/tools/llvm-rc/rc.sh b/llvm/tools/llvm-rc/rc.sh
new file mode 100755
index 000000000000..4a16f020aad8
--- /dev/null
+++ b/llvm/tools/llvm-rc/rc.sh
@@ -0,0 +1,34 @@
+#!/bin/bash
+
+INCLUDES=
+for flag in "$@" 
+do
+        if [[ $flag =~ /Fo(.+) ]] ;
+	then
+		OUTFILE=${BASH_REMATCH[1]}
+		OUTFLAG=$flag
+	fi
+	if [[ $flag =~ .+\.rc ]] ;
+	then
+		RCFILE=$flag
+		RCPATH=$(dirname "${flag}")
+	fi
+	if [[ $flag =~ /I ]] ;
+	then
+		INCLUDES="$INCLUDES $flag"
+	fi
+	if [[ $flag == /nologo ]] ;
+	then
+		NOLOGO=$flag
+	fi
+done
+if [[ $* =~ (/l[[:space:]]0x[[:alnum:]]+) ]] ;
+then
+	LANGUAGE="${BASH_REMATCH[1]}"
+fi
+
+SCRIPTPATH=$(dirname "$0")
+
+$SCRIPTPATH/clang-cl /E ${RCFILE} ${INCLUDES} -nostdinc -Wno-nonportable-include-path > "${OUTFILE}.i"
+$SCRIPTPATH/llvm-rc "${OUTFILE}.i" ${OUTFLAG} ${LANGUAGE} ${NOLOGO} /I${RCPATH}
+

Can the same things be emulated with the plain rc.exe tool too? I guess -nostdinc maps to the /X option (i.e. ignoring the INCLUDE variable), but I'm not sure if there's an official way of passing arbitrary preprocessor args with rc.exe (as rc.exe doesn't launch an out of process preprocessor, but it uses a built-in one). (With the windres interface, there's --preprocessor-arg though.)

llvm/tools/llvm-rc/llvm-rc.cpp
133

I did indeed consider calling literally clang-cl first, but to reduce differences with D100756, I went with plain clang. Running clang -target *-windows-msvc does retain most aspects of the clang-cl specific behaviours (including using the INCLUDE env var for picking up include directories) - off hand I don't think of anything in this use case where the distinction between clang -target *-windows-msvc and --driver-mode=cl matters. But I guess one could add --driver-mode=cl if you really want to, it shouldn't make things much harder for D100756.

167

It's not about child processes within clang, it's simply that the LLVM Support library's Execute* functions don't support redirecting stdin/stdout/stderr to a pipe and reading it at all.

thakis accepted this revision.Apr 20 2021, 8:35 AM

This looks basically good to me.

llvm/tools/llvm-rc/llvm-rc.cpp
98

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)

154

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.

217

Do we need the env var?

This revision is now accepted and ready to land.Apr 20 2021, 8:35 AM

Thanks for adding this Martin!

I'd really like to see a simple test running from the Clang side as well! Like calling llvm-rc with a file that requires preprocessing:

That sounds like a good idea, as there's indeed a gap in testing at that spot right now.

What would be the most appropriate place under clang/test for it?

Maybe under clang\test\Preprocessor ?


Also just for reference purposes, I'm pasting the little script I'm currently using to replace rc.exe on Linux. I've put it in the same folder as clang-cl and llvm-rc and now I'm able to use the same cmd-line as for rc.exe (it goes through a temp file which isn't ideal):

diff --git a/llvm/tools/llvm-rc/rc.sh b/llvm/tools/llvm-rc/rc.sh
new file mode 100755
index 000000000000..4a16f020aad8
--- /dev/null
+++ b/llvm/tools/llvm-rc/rc.sh
@@ -0,0 +1,34 @@
+#!/bin/bash
+
+INCLUDES=
+for flag in "$@" 
+do
+        if [[ $flag =~ /Fo(.+) ]] ;
+	then
+		OUTFILE=${BASH_REMATCH[1]}
+		OUTFLAG=$flag
+	fi
+	if [[ $flag =~ .+\.rc ]] ;
+	then
+		RCFILE=$flag
+		RCPATH=$(dirname "${flag}")
+	fi
+	if [[ $flag =~ /I ]] ;
+	then
+		INCLUDES="$INCLUDES $flag"
+	fi
+	if [[ $flag == /nologo ]] ;
+	then
+		NOLOGO=$flag
+	fi
+done
+if [[ $* =~ (/l[[:space:]]0x[[:alnum:]]+) ]] ;
+then
+	LANGUAGE="${BASH_REMATCH[1]}"
+fi
+
+SCRIPTPATH=$(dirname "$0")
+
+$SCRIPTPATH/clang-cl /E ${RCFILE} ${INCLUDES} -nostdinc -Wno-nonportable-include-path > "${OUTFILE}.i"
+$SCRIPTPATH/llvm-rc "${OUTFILE}.i" ${OUTFLAG} ${LANGUAGE} ${NOLOGO} /I${RCPATH}
+

Can the same things be emulated with the plain rc.exe tool too? I guess -nostdinc maps to the /X option (i.e. ignoring the INCLUDE variable), but I'm not sure if there's an official way of passing arbitrary preprocessor args with rc.exe (as rc.exe doesn't launch an out of process preprocessor, but it uses a built-in one). (With the windres interface, there's --preprocessor-arg though.)

/X is not exactly like -nostdinc, at least in Clang. -nostdinc has precedence over everything and it seemed the simplest way to be completly hermetic: https://github.com/llvm/llvm-project/blob/d480f968ad8b56d3ee4a6b6df5532d485b0ad01e/clang/lib/Driver/ToolChains/MSVC.cpp#L1234
(but we need to use /clang:-isystem instead of -imsvc for system headers)

As for the behavior of rc.exe on Windows, we start with clean env.vars and launch vcvars64.bat from the nuget package and that solves it. But on Linux we can't really do that. I agree, a new option --preprocessor-arg could help there?

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.

llvm/tools/llvm-rc/Opts.td
43–46

That might be a good idea.

llvm/tools/llvm-rc/llvm-rc.cpp
98

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.)

154

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?

217

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.

thakis added inline comments.Apr 20 2021, 11:28 AM
llvm/tools/llvm-rc/llvm-rc.cpp
98

Seems fine too, no strong opinion either way.

thakis added inline comments.Apr 20 2021, 11:33 AM
llvm/tools/llvm-rc/llvm-rc.cpp
154

We don't need a general --preprocessor-arg as long as common args work.

Oh, on that note: https://source.chromium.org/chromium/chromium/src/+/master:build/toolchain/win/rc/rc.py also has /showIncludes support which is necessary to make ninja re-build .rc files if either

  • an included .h file is changed (needs preprocessor output)
  • an included resource file (.ico or similar) is changed (needs llvm-rc support)

Don't know if llvm-rc has support for the latter part. If it doesn't, that'd be nice to add, and some test that checks that both parts work would be nice. (Neither in this patch; just thinking about what we'd need to switch.)

epastor added inline comments.Apr 20 2021, 12:18 PM
llvm/tools/llvm-rc/Opts.td
43–46

+1 for this; I think -no-preprocess would be much clearer.

mstorsjo updated this revision to Diff 338969.Apr 20 2021, 12:54 PM
  • Added a testcase under clang/test/Preprocessor
  • Changed the option to -no-preprocess
  • Removed the env var for disabling preprocessing
  • Made it look for both clang and clang-cl

This still uses --driver-mode=gcc; it currently doesn't allow setting options where --driver-mode=cl would matter, and allowing that does complicate things a little. So I'd leave that as a future improvement for when/if it becomes possible to set such options where it matters.

I'm also not adding any more convenience options like -winsysroot, -imsvc, -showincludes for now; leaving those to future patches (also possibly for people who actually use them who can verify that their use makes sense.

Herald added a project: Restricted Project. · View Herald TranscriptApr 20 2021, 12:54 PM
aganea added inline comments.Apr 20 2021, 2:05 PM
llvm/tools/llvm-rc/llvm-rc.cpp
94

Since you're not using the std::error_code below in the call site, should this return Expected<...>? In that case, the variable Path shouldn't be needed?

154

We don't need a general --preprocessor-arg as long as common args work.

+1 with Nico. Native arguments in llvm-rc would be better (later).

mstorsjo added inline comments.Apr 20 2021, 2:23 PM
llvm/tools/llvm-rc/llvm-rc.cpp
94

With Expected<> I'd need to craft an Error at the end if I don't have a path to return, but do you mean Optional<>? I guess that'd work - but as findProgramByName() returns ErrorOr<std::string> I kept the same signature.

Even if returning Optional<>, we need a local variable to receive the ErrorOr<std::string> from findProgramByName() and inspect it.

In the future (after a release or so) I'd intend for this to be a hard error, so at that point, the returned error code actually would be printed.

aganea accepted this revision.Apr 20 2021, 2:28 PM

LGTM.

llvm/tools/llvm-rc/llvm-rc.cpp
94

I see, thanks for the explanation!

This revision was landed with ongoing or failed builds.Apr 21 2021, 1:50 AM
This revision was automatically updated to reflect the committed changes.

Purely FYI: the -- in the test isn't needed just on macOS but also on Linux if your checkout is eg under /usr: http://45.33.8.238/linux/44652/step_7.txt

(clang-cl has some nice diag if the /U flag is an existing file, since that happens fairly often. Maybe llvm-rc could have a nicer diag in that case too and suggest --)