libvips / build-win64-mxe

73 stars 15 forks source link

Symbols in 32-bit 8.12.0-rc1-static libvips.lib are missing underscore prefix #37

Closed lovell closed 2 years ago

lovell commented 2 years ago

vips-dev-w32-web-8.12.0-rc1-static.zip

$ ar p libvips.lib | strings | grep vips_image_set_int
vips_image_set_int

vips-dev-w32-web-8.11.4-static.zip

$ ar p libvips.lib | strings | grep vips_image_set_int
_vips_image_set_int

Discovered via the build failure at https://ci.appveyor.com/project/lovell/sharp/builds/41548233/job/dbmqq9c1s0a9hlem

kleisauke commented 2 years ago

Indeed, looks like a regression which affects re-linking with MSVC. Since commit https://github.com/libvips/build-win64-mxe/commit/87185888391a13c4d22e4becf4b10e6ff55cce5a (which updates LLVM to 13), llvm-dlltool is used to build these .lib files, instead of the dlltool wrapper. Perhaps these 2 comments are relevant here: https://github.com/llvm/llvm-project/blob/e356027016c6365b3d8924f54c33e2c63d931492/llvm/lib/Object/COFFImportFile.cpp#L94-L99 https://github.com/llvm/llvm-project/blob/6c48f6aafe69017065ab424dde11909d9cd5cb44/llvm/lib/Object/COFFModuleDefinition.cpp#L60-L69

I'll investigate this further.

kleisauke commented 2 years ago

It looks like llvm-ddltool was always targeting the default i386:x86-64 machine, see:

$ docker run --rm -it -u $(id -u):$(id -g) -v $PWD/build:/data --entrypoint "bash" libvips-build-win-mxe
$ export PATH=/data/mxe/usr/bin:/data/mxe/usr/x86_64-pc-linux-gnu/bin:/data/mxe/usr/i686-w64-mingw32.static.posix.web/bin:$PATH
$ cd /data/mxe/usr/i686-w64-mingw32.static.posix.web
$ gendef - bin/libvips-42.dll > lib/libvips.def
$ i686-w64-mingw32.static.posix.web-dlltool -d lib/libvips.def -l lib/libvips.lib -D bin/libvips-42.dll
$ llvm-readobj --coff-exports lib/libvips.lib | grep vips_image_set_int
Symbol: __imp_vips_image_set_int
Symbol: vips_image_set_int
$ llvm-readobj lib/libvips.lib  | grep -B2 -A1 "Arch:"
File: lib/libvips.lib(libvips-42.dll)
Format: COFF-x86-64
Arch: x86_64
AddressSize: 64bit
--
File: lib/libvips.lib(libvips-42.dll)
Format: COFF-x86-64
Arch: x86_64
AddressSize: 64bit
--
File: lib/libvips.lib(libvips-42.dll)
Format: COFF-x86-64
Arch: x86_64
AddressSize: 64bit

For the i386 target it should print this:

$ llvm-dlltool -m i386 -d lib/libvips.def -l lib/libvips-fixed.lib -D bin/libvips-42.dll
$ llvm-readobj --coff-exports lib/libvips-fixed.lib | grep vips_image_set_int
Symbol: __imp__vips_image_set_int
Symbol: _vips_image_set_int
$ llvm-readobj lib/libvips-fixed.lib  | grep -B2 -A1 "Arch:"
File: lib/libvips-fixed.lib(libvips-42.dll)
Format: COFF-i386
Arch: i386
AddressSize: 32bit
--
File: lib/libvips-fixed.lib(libvips-42.dll)
Format: COFF-i386
Arch: i386
AddressSize: 32bit
--
File: lib/libvips-fixed.lib(libvips-42.dll)
Format: COFF-i386
Arch: i386
AddressSize: 32bit

This is due to the fact that MXE targets can contain dots to indicate shared/static linking (e.g., x86_64-w64-mingw32.static or x86_64-w64-mingw32.shared), which fails when LLVM tries to detect its architecture. Commit https://github.com/libvips/build-win64-mxe/commit/f5512e9c964a67c7b9d3fe133957d9e1a61365cd should fix this. This bug was introduced in commit 87185888391a13c4d22e4becf4b10e6ff55cce5a. I'm rebuilding and preparing a new release now.

@mstorsjo Do you think it makes sense for LLVM to support these (uncommon) prefixed wrappers? See these comments and this patch for more details. I think NixOS is doing something similar (which I think was the reason for this PR).

mstorsjo commented 2 years ago

@mstorsjo Do you think it makes sense for LLVM to support these (uncommon) prefixed wrappers? See these comments and this patch for more details. I think NixOS is doing something similar (which I think was the reason for this PR).

Hmm, I'm not sure what the most straightforward way to parse it would be.

Given that the parsed argv[0] can be something like aarch64-w64-mingw32-llvm-dlltool-10.exe, I'm not sure how to easily trim away .exe, without losing the whole end in aarch64-w64-mingw32.static-dlltool. I guess it'd need to be a trim like s/\.[a-z]*$// for a regex or something like that. Ideally it'd be a straightforward operation with plain C99 string.h or llvm::sys::path operations. Or maybe just if (endswith_case_insensitive(".exe")) str[strlen()-4] = '\0';?

kleisauke commented 2 years ago

I'm not sure what the best approach is either. If only the .exe extension needs to be trimmed away, then it can be easily fixed as you said.

FWIW, here is an example of what the MXE directory structure looks like (when using both GCC and LLVM).

Directory structure ```bash $ tree /data/mxe/usr -L 3 --dirsfirst ├── bin │   ├── aarch64-w64-mingw32.static-clang -> /data/mxe/usr/aarch64-w64-mingw32.static/bin/clang-target-wrapper.sh │   ├── aarch64-w64-mingw32.static-clang++ -> /data/mxe/usr/aarch64-w64-mingw32.static/bin/clang-target-wrapper.sh │   ├── aarch64-w64-mingw32.static-ld -> /data/mxe/usr/aarch64-w64-mingw32.static/bin/ld-wrapper.sh │   ├── i686-w64-mingw32.shared.win32.dw2-g++ -> /data/mxe/usr/i686-w64-mingw32.shared.win32.dw2/bin/g++ │   ├── i686-w64-mingw32.shared.win32.dw2-gcc -> /data/mxe/usr/i686-w64-mingw32.shared.win32.dw2/bin/gcc │   ├── i686-w64-mingw32.shared.win32.dw2-ld -> /data/mxe/usr/i686-w64-mingw32.shared.win32.dw2/bin/ld │   ├── x86_64-w64-mingw32.shared.posix-g++ -> /data/mxe/usr/x86_64-w64-mingw32.shared.posix/bin/g++ │   ├── x86_64-w64-mingw32.shared.posix-gcc -> /data/mxe/usr/x86_64-w64-mingw32.shared.posix/bin/gcc │   └── x86_64-w64-mingw32.shared.posix-ld -> /data/mxe/usr/x86_64-w64-mingw32.shared.posix/bin/ld ├── aarch64-w64-mingw32.static │   ├── bin │   │   ├── clang -> /data/mxe/usr/x86_64-pc-linux-gnu/bin/clang │   │   ├── clang++ -> /data/mxe/usr/x86_64-pc-linux-gnu/bin/clang++ │   │   ├── clang-target-wrapper.sh │   │   ├── ld.lld -> /data/mxe/usr/x86_64-pc-linux-gnu/bin/ld.lld │   │   └── ld-wrapper.sh │   ├── etc │   ├── include │   ├── lib │   ├── mingw │   └── share ├── i686-w64-mingw32.shared.win32.dw2 │   ├── bin │   │   ├── g++ │   │   ├── gcc │   │   └── ld │   ├── etc │   ├── include │   ├── lib │   ├── mingw │   └── share ├── x86_64-w64-mingw32.shared.posix │   ├── bin │   │   ├── g++ │   │   ├── gcc │   │   └── ld │   ├── etc │   ├── include │   ├── lib │   ├── mingw │   └── share └── x86_64-pc-linux-gnu ├── bin │   ├── clang -> clang-13 │   ├── clang++ -> clang │   ├── clang-13 │   ├── ld.lld -> lld │   └── lld ├── include ├── lib └── share ```
kleisauke commented 2 years ago

The libvips v8.12.0-rc1-build2 Windows binaries are now available with this fix included. I'll close.

@mstorsjo Let me know if you'd rather track this in a new issue.

mstorsjo commented 2 years ago

@mstorsjo Let me know if you'd rather track this in a new issue.

Yeah I won't keep track of it from here at least, I'd rather have an issue in llvm-mingw then (and possibly one in upstream llvm, as there's some cases of similar handling there, although I don't keep as good track of longstanding issues there).

lovell commented 2 years ago

@kleisauke Dankjewel!

kleisauke commented 1 year ago

FWIW, I simplified this logic by introducing an intermediate bash wrapper (see e.g. work-in-progress commit 76a0a0b35bc87e78c2c4177240c3dc4aeb81a6d1).

Details ```console $ cat /data/mxe/usr/bin/aarch64-w64-mingw32.static-dlltool #!/bin/sh exec "/data/mxe/usr/aarch64-w64-mingw32.static/bin/x86_64-w64-mingw32-dlltool" "$@" $ cat /data/mxe/usr/bin/aarch64-w64-mingw32.static-windres #!/bin/sh exec "/data/mxe/usr/aarch64-w64-mingw32.static/bin/x86_64-w64-mingw32-windres" "$@" $ tree /data/mxe/usr -L 3 --dirsfirst ├── bin │   ├── aarch64-w64-mingw32.static-dlltool │   └── aarch64-w64-mingw32.static-windres ├── aarch64-w64-mingw32.static │   ├── aarch64-w64-mingw32 │   ├── bin │   │   ├── aarch64-w64-mingw32-dlltool -> /data/mxe/usr/x86_64-pc-linux-gnu/bin/llvm-dlltool │   │   └── aarch64-w64-mingw32-windres -> /data/mxe/usr/x86_64-pc-linux-gnu/bin/llvm-windres │   ├── etc │   ├── include │   ├── lib │   └── share └── x86_64-pc-linux-gnu ├── bin │   ├── llvm-ar │   ├── llvm-dlltool -> llvm-ar │   ├── llvm-rc │   ├── llvm-windres -> llvm-rc ├── include ├── lib └── share ```

I also looked into supporting this in LLVM, but it seemed like that would require a lot of changes. Many LLVM tools do the basename detection of the running tool based on the result of the llvm::sys::path::stem function, which would strip the . and everything after that:

sys::path::stem("aarch64-w64-mingw32.static-dlltool") -> x86_64-w64-mingw32
sys::path::stem("aarch64-w64-mingw32-dlltool.exe") -> x86_64-w64-mingw32-dlltool

Replacing the dots with a hyphen (i.e. s/./-/g) would also require a lot of changes, especially since Autotools-based projects fails to canonicalize --host=x86_64-w64-mingw32-static-posix. https://git.savannah.gnu.org/cgit/config.git/tree/config.sub?id=63acb96f92473ceb5e21d873d7c0aee266b3d6d3#n133