[llvm-windres] Implement the windres flag --use-temp-file

[mstorsjo]

Summary

Whether a temp file or a pipe is used for preprocessing is an
internal detail, this flag has a notable effect on the preprocessing
in GNU windres. Without this flag, GNU windres passes command
arguments as-is to popen(), which means they get evaluated by a
shell without being re-escaped for this case. To mimic this,
llvm-windres has manually tried to unescape arguments.

When GNU windres is given the --use-temp-file flag, it uses a
different API for invoking the preprocessor, and this API takes care
of preserving special characters in the command line arguments.
For users of GNU windres, this means that by using --use-temp-file,
they don't need to do the (quite terrible) double escaping of
quotes/spaces etc.

The xz project uses the --use-temp-file flag when invoking
GNU windres, see
https://github.com/tukaani-project/xz/commit/6b117d3b1fe91eb26d533ab16a2e552f84148d47.
However as llvm-windres didn't implement this flag and just
assumed the GNU windres popen() behaviour, they had to use a
different codepath for llvm-windres.

That separate codepath for llvm-windres broke later when llvm-windres
got slightly more accurate unescaping of lone quotes in
0f4c6b120f21d582ab7c5c4f2b2a475086c34938 /
https://reviews.llvm.org/D146848 (fixing a discrepancy to GNU
windres as found in https://github.com/llvm/llvm-project/issues/57334),
and this was reported in
https://github.com/mstorsjo/llvm-mingw/issues/363.

Not touching the implementation of the --preprocessor option
with respect to the --use-temp-file flag; that option is doubly
tricky as GNU windres changed its behaviour in a backwards incompatible
way recently (and llvm-windres currently matches the old behaviour).

TL;DR, previously we ignored this flag, but it turned out it has an
observeable effect and it allows entirely avoiding one of the more
insane parts of the windres command line interface.
Plus the earlier change from D146848 fixed one case but broke another
(a 17.x regression), and implementing this allows fixing the broken
case.

[alvinhochun]

The code looks fine to me, and I trust that you have tested it.

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

496–497

I presume you plan to resolve this soon? Since you mentioned that GNU windres changed its behaviour in a backwards incompatible
way do you have a reference to any upstream bug reports or commits regarding this?

[mstorsjo]

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

496–497

I'm kinda unsure about what to do about it.

It broke, unintentionally, in this binutils commit: https://sourceware.org/git/?p=binutils-gdb.git;a=commitdiff;h=21c33bcbe36377abf01614fb1b9be439a3b6de20

There were bug reports about it filed here: https://github.com/msys2/MSYS2-packages/issues/2379 and here: https://sourceware.org/bugzilla/show_bug.cgi?id=27594

But there was no acknowledgement that this indeed broke the existing uses of the option according to the documentation, instead the documentation was updated to document how the option works now: https://sourceware.org/git/?p=binutils-gdb.git;a=commitdiff;h=5edb8e3f5ad8d74a83fc0df7f6e4514eed0aa77f;hp=3abbafc2aacc6706fea3e3e326e2f08d107c3672

Before this, llvm-windres was implemented mimicing GNU windres behaviour from this change - to make llvm-windres work with projects that were using it according to the old behaviour, e.g. in ffmpeg. Due to this breakage, it's hard for third party projects to sensibly use this option at all - so in ffmpeg I had to opt to change it to use another option instead: https://github.com/ffmpeg/ffmpeg/commit/f9626d1065c43f1d51afe66bdf988b9f33729440 Not sure how other projects that were affected dealt with it.

As 2 years has passed since, and I guess most projects that were using the option have had to adapt (i.e. stop using it? or switch to the new form and not work with llvm-windres), I guess we could/should update. I feel we didn't really get proper closure on those bug reports though - that why I haven't taken action on matching this behaviour yet.

[mati865]

I don't have any experience with --use-temp-file so I'll trust your judgement.
Let's get this regression fixed but I'm also curious about the plan for future `--preprocessor changes.

[mstorsjo]

In D159223#4634441, @mati865 wrote:

Let's get this regression fixed

Technically this bit isn't a regression - I just hadn't realized that --use-temp-file inhibits the messier parts of the windres interface (and hadn't run into projects using it before). But for other reasons, a project that wanted to use --use-temp-file but couldn't use it with llvm-windres, had used a different parameter passing which was broken by 0f4c6b120f21d582ab7c5c4f2b2a475086c34938 / D146848 - where this option made llvm-windres match GNU windres better.

So no regression, but for this usecase we placed that particular user of the tool in a tricky spot - correctly implementing this option gives a better way forward. (This issue has normally been solved by double-escaping parameters to windres elsewhere - but the use in xz where the same defines are passed both to windres and to the C compiler breaks if using that double-escaping fix.)

but I'm also curious about the plan for future `--preprocessor changes.

I guess I could look into changing it to the new behaviour form at some point. But as the referenced bugzilla thread shows, it was all left in a bit of undefined state (attempts to revert something to appease some users, but actually reverting the wrong thing, etc...)

[mati865]

So no regression, but for this usecase we placed that particular user of the tool in a tricky spot

Oh, that's tricky situation indeed

I guess I could look into changing it to the new behaviour form at some point.

Phabricator didn't refresh after you replied to Alvin, that comment answers my question.

llvm/test/tools/llvm-rc/windres-preproc.test

 ;; Some quoted arguments below don't work properly on Windows when llvm-lit
 ;; invokes the cmd shell to run the commands. Just require running in a
 ;; posix shell, to keep being able to test this corner case on Unix at least.
 ; REQUIRES: shell
 
 ; 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"{{$}}
 ; 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"{{$}}

llvm/tools/llvm-rc/WindresOpts.td

 defm define : LongShort<"D", "define", "Define to pass to the preprocessor">;
 
 defm undef : LongShort<"U", "undefine", "Undefine to pass to the preprocessor">;
 
 defm codepage : LongShort<"c", "codepage", "Default codepage to use">;
 
 defm language : LongShort<"l", "language", "Default language to use (0x0-0xffff)">;
 
+def use_temp_file: Flag<["--"], "use-temp-file">,
+                   HelpText<"Mimic GNU windres preprocessor option handling "
+                            "(don't unescape preprocessor options)">;
+
 defm verbose : F<"v", "verbose", "Enable verbose output">;
 defm version : F<"V", "version", "Display version">;
 
 defm help : F<"h", "help", "Display this message and exit">;
 
 // Print (but do not run) the commands to run for preprocessing
 def _HASH_HASH_HASH : Flag<["-"], "###">;
 
 def no_preprocess : Flag<["--"], "no-preprocess">;
-
-// Unimplemented options for compatibility
-def use_temp_file: Flag<["--"], "use-temp-file">;

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

for (const auto *Arg :
     // when calling GNU windres, callers need to double escape chars like
     // quotes, e.g. as -DSTRING=\\\"1.2.3\\\".
     //
     // Exactly how the arguments are interpreted depends on the platform
     // though - but the cases where this matters (where callers would have
     // done this double escaping) probably is confined to cases like these
     // quoted string defines, and those happen to work the same across unix
     // and windows.
-    std::string Unescaped = unescape(Arg->getValue());
+    //
+    // If GNU windres is executed with --use-temp-file, it doesn't use
+    // popen() to invoke the preprocessor, but uses another function which
+    // actually preserves tricky characters better. To mimic this behaviour,
+    // don't unescape arguments here.
+    std::string Value = Arg->getValue();
+    if (!InputArgs.hasArg(WINDRES_use_temp_file))
+      Value = unescape(Value);
     switch (Arg->getOption().getID()) {
     case WINDRES_include_dir:
       // Technically, these are handled the same way as e.g. defines, but
       // the way we consistently unescape the unix way breaks windows paths
       // with single backslashes. Alternatively, our unescape function would
       // need to mimic the platform specific command line parsing/unescaping
       // logic.
       Opts.Params.Include.push_back(Arg->getValue());
       Opts.PreprocessArgs.push_back("-I");
       Opts.PreprocessArgs.push_back(Arg->getValue());
       break;
     case WINDRES_define:
       Opts.PreprocessArgs.push_back("-D");
-      Opts.PreprocessArgs.push_back(Unescaped);
+      Opts.PreprocessArgs.push_back(Value);
       break;
     case WINDRES_undef:
       Opts.PreprocessArgs.push_back("-U");
-      Opts.PreprocessArgs.push_back(Unescaped);
+      Opts.PreprocessArgs.push_back(Value);
       break;
     case WINDRES_preprocessor_arg:
-      Opts.PreprocessArgs.push_back(Unescaped);
+      Opts.PreprocessArgs.push_back(Value);
       break;
     }
   }
+  // TODO: If --use-temp-file is set, we shouldn't be unescaping
+  // the --preprocessor argument either, only splitting it.
   if (InputArgs.hasArg(WINDRES_preprocessor))
     Opts.PreprocessCmd =
         unescapeSplit(InputArgs.getLastArgValue(WINDRES_preprocessor));
 
   Opts.Params.CodePage = CpWin1252; // Different default
   if (InputArgs.hasArg(WINDRES_codepage)) {
     if (InputArgs.getLastArgValue(WINDRES_codepage)
             .getAsInteger(0, Opts.Params.CodePage))