[PATCH] D159223: [llvm-windres] Implement the windres flag --use-temp-file

Wed Aug 30 13:39:47 PDT 2023

mstorsjo created this revision.
mstorsjo added reviewers: thieta, mati865.
Herald added a project: All.
mstorsjo requested review of this revision.
Herald added a project: LLVM.

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/rG0f4c6b120f21d582ab7c5c4f2b2a475086c34938> /
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 <https://reviews.llvm.org/D146848> fixed one case but broke another
(a 17.x regression), and implementing this allows fixing the broken
case.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D159223

Files:
  llvm/test/tools/llvm-rc/windres-preproc.test
  llvm/tools/llvm-rc/WindresOpts.td
  llvm/tools/llvm-rc/llvm-rc.cpp


Index: llvm/tools/llvm-rc/llvm-rc.cpp
===================================================================

--- llvm/tools/llvm-rc/llvm-rc.cpp
+++ llvm/tools/llvm-rc/llvm-rc.cpp
@@ -461,7 +461,14 @@
     // 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
@@ -475,17 +482,19 @@
       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));
Index: llvm/tools/llvm-rc/WindresOpts.td
===================================================================
--- llvm/tools/llvm-rc/WindresOpts.td
+++ llvm/tools/llvm-rc/WindresOpts.td
@@ -48,6 +48,10 @@
 
 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">;
 
@@ -57,6 +61,3 @@
 def _HASH_HASH_HASH : Flag<["-"], "###">;
 
 def no_preprocess : Flag<["--"], "no-preprocess">;
-
-// Unimplemented options for compatibility
-def use_temp_file: Flag<["--"], "use-temp-file">;
Index: llvm/test/tools/llvm-rc/windres-preproc.test
===================================================================
--- llvm/test/tools/llvm-rc/windres-preproc.test
+++ llvm/test/tools/llvm-rc/windres-preproc.test
@@ -4,6 +4,7 @@
 ; 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"{{$}}


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D159223.554819.patch
Type: text/x-patch
Size: 3917 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20230830/41aca237/attachment.bin>
