skeeto / w64devkit

Portable C and C++ Development Kit for x64 (and x86) Windows
The Unlicense
2.7k stars 185 forks source link

Issue of `x86_64-w64-mingw32-windres' #35

Closed HuangShumin closed 1 year ago

HuangShumin commented 1 year ago

I used w64devkit to compile latest busybox-w32 of master branch. And I got an error when I execute this command as below:

x86_64-w64-mingw32-windres -D"KBUILD_STR(s)=#s" -D"BB_VER=KBUILD_STR(1.36.0.git)" -D"BB_VERSION=1" -D"BB_PATCHLEVEL=36" -D"BB_SUBLEVEL=0" -D"BB_EXTRAVERSION=0" --include-dir=/busybox-w32-master/include --include-dir=/busybox-w32-master/win32/resources win32/resources/resources.rc win32/resources/resources.o

I got error as below:

<command-line>: warning: ISO C99 requires whitespace after the macro name x86_64-w64-mingw32-windres: win32/resources/resources.rc:21: syntax error

When I substitue x86_64-w64-mingw32-windres.exe(3KB) with msys2 windres.exe(2.3MB), and execute again, it works without any error.

So I think there's some issue about windres in w64devkit. If this issue is solved, w64devkit is able to build latest busybox-w32.

skeeto commented 1 year ago

Quick summary: This is a bug in Binutils' windres.c:quot, or maybe in pex-win32.c:pex_win32_exec_child depending on your point of view. It assumes the preprocessor will be run through a POSIX shell and quotes accordingly, but this doubly wrong on Windows.

How are you getting this far? I didn't know it was possible. When I try on Windows, GNU Make can't even successfully parse the busybox-w32 Makefile, let alone actually start building anything. After generating an autoconf.h on Linux and copying it over, I could reproduce the issue.

My main suspicion was that the command line string is damaged somewhere along the way. That's a lot of shell metacharacters, and there are a dozen incompatible conventions for parsing command line strings on Windows. The 3kB windres is just an alias for the real 1MB windres.exe, but this also fails, meaning it's not a problem in the alias. However, substituting cpp.exe works just fine, indicating it's happening somewhere between windres.exe and cpp.exe.

I wrote a C program that prints its arguments to a log, one per line, then used the windres --preprocessor option to use it as the preprocessor. The log showed that the preprocessor gets backslashes before each parenthesis, as though they were put there for the shell but not removed. This is necessary on unix (including MSYS2) where the preprocessor is run through popen().

On Windows obviously there's no POSIX shell so the quoting is wrong, but even worse there's no shell involved at all. It invokes CreateProcess directly (pex-win32.c), including a argv_to_cmdline which is essentially the role intended by winres.c:quot. Either windres shouldn't be quoting metacharacters on Windows, or pex_win32_exec_child should be unquoting in the manner of a POSIX shell since its callers assume this behavior. (There are probably more Binutils bugs lurking around this interface.)

The most expedient fix would be to replace quot() with xstrdup() when targeting Windows, eliminating the unnecessary quoting. Until this is fixed upstream, I'm considering patching that locally.

skeeto commented 1 year ago

Possible correction: It may actually be using (via macro on Windows) msvcrt's _popen by default, without pex_win32_exec_child. It depends on --use-temp-file. So there may be a shell ("command processor"), but it doesn't handle quoting like a POSIX shell. Regardless, this should be reflected in windres.c:quot, and that's probably what needs fixing.

skeeto commented 1 year ago

Feeling more confident now: Try using --use-temp-file and you'll see the bug on all platforms (MSYS, Linux, etc.). In this mode, after constructing the command string windres parses it back apart incorrectly. I hoped this would be a workaround for Windows, but when it didn't work I realized it also must be a problem everywhere else. That makes this easier to report upstream, too.

HuangShumin commented 1 year ago

thank you for your analysis, answer draws near now. I tried yesterday and managed to build busybox with workaround as below:

  1. extract latest busybox-w32-master.zip to C:\ and cd C:/busybox-w32-master in w64devkit console
  2. sed -i 's/find \*/find ./' scripts/basic/split-include.c
  3. modify scripts/embedded_scripts and applets/usage_compressed last command mv -- "$target.$$" "$target" to cat "$target.$$" > "$target"
  4. modify applets/usage_compressed test -x "$loc/usage" || test -x "$loc/usage.exe" || exit 1
  5. substitue x86_64-w64-mingw32-windres with msys2 mingw64 windres.exe 2.4MB
  6. link unreference to 'ffs' in gzip.o issue sed -i 's/int ffs(int i);/#define ffs __builtin_ffs/' include/mingw.h
  7. make CURDIR=/busybox-w32-master mingw64-defconfig without drive letter and colon
  8. make CURDIR=/busybox-w32-master #-d
  9. ls -lh busybox.exe #about 1.1MB larger than msys2 and Linux posix building

And I tested to build busybox with custom script embedded, it also works.

cd /busybox-w32-master

mkdir embed

echo "echo hello world" > embed/hello

make CURDIR=/busybox-w32-master

./busybox hello

It's convenient to embed some small shell applets to busybox-w32 now.

HuangShumin commented 1 year ago

shall we keep this open before we report to upstream

skeeto commented 1 year ago

Thanks for the info! Closed is fine for now. Just need to take some time to report it upstream...