search

mstorsjo / llvm-mingw

An LLVM/Clang/LLD based mingw-w64 toolchain
Other
1.75k stars 176 forks source link

llvm-mingw-20240130-ucrt-ubuntu-20.04-x86_64 fails to build Wine 9.x (ld.lld: error: duplicate symbol: __chkstk) #397

Closed rmi1974 closed 4 months ago

rmi1974 commented 5 months ago

Hello folks,

just switched out llvm-mingw-20231128-ucrt-ubuntu-20.04-x86_64 against the new release llvm-mingw-20240130-ucrt-ubuntu-20.04-x86_64.

Not that it really matters but I built Wine current HEAD: https://source.winehq.org/git/wine.git/commit/e3431a02e1d23ad506512b906f244eae1db05035

Non-working case:

llvm-mingw-20240130-ucrt-ubuntu-20.04-x86_64

Last lines of build output:

...
tools/winegcc/winegcc -o dlls/ntdll/x86_64-windows/ntdll.dll --wine-objdir . -b x86_64-w64-mingw32 -Wl,--wine-builtin -shared \
  /home/focht/projects/wine/mainline-src/dlls/ntdll/ntdll.spec -nodefaultlibs \
  -Wl,--image-base,0x170000000 dlls/ntdll/x86_64-windows/actctx.o dlls/ntdll/x86_64-windows/atom.o \
  dlls/ntdll/x86_64-windows/crypt.o dlls/ntdll/x86_64-windows/debugbuffer.o \
  dlls/ntdll/x86_64-windows/env.o dlls/ntdll/x86_64-windows/error.o \
  dlls/ntdll/x86_64-windows/exception.o dlls/ntdll/x86_64-windows/handletable.o \
  dlls/ntdll/x86_64-windows/heap.o dlls/ntdll/x86_64-windows/large_int.o \
  dlls/ntdll/x86_64-windows/loader.o dlls/ntdll/x86_64-windows/locale.o \
  dlls/ntdll/x86_64-windows/math.o dlls/ntdll/x86_64-windows/misc.o dlls/ntdll/x86_64-windows/path.o \
  dlls/ntdll/x86_64-windows/printf.o dlls/ntdll/x86_64-windows/process.o \
  dlls/ntdll/x86_64-windows/reg.o dlls/ntdll/x86_64-windows/relay.o \
  dlls/ntdll/x86_64-windows/resource.o dlls/ntdll/x86_64-windows/rtl.o \
  dlls/ntdll/x86_64-windows/rtlbitmap.o dlls/ntdll/x86_64-windows/rtlstr.o \
  dlls/ntdll/x86_64-windows/sec.o dlls/ntdll/x86_64-windows/signal_arm.o \
  dlls/ntdll/x86_64-windows/signal_arm64.o dlls/ntdll/x86_64-windows/signal_arm64ec.o \
  dlls/ntdll/x86_64-windows/signal_i386.o dlls/ntdll/x86_64-windows/signal_x86_64.o \
  dlls/ntdll/x86_64-windows/string.o dlls/ntdll/x86_64-windows/sync.o \
  dlls/ntdll/x86_64-windows/thread.o dlls/ntdll/x86_64-windows/threadpool.o \
  dlls/ntdll/x86_64-windows/time.o dlls/ntdll/x86_64-windows/version.o \
  dlls/ntdll/x86_64-windows/wcstring.o dlls/ntdll/version.res \
  -Wl,--debug-file,dlls/ntdll/x86_64-windows/ntdll.pdb libs/musl/x86_64-windows/libmusl.a \
  dlls/winecrt0/x86_64-windows/libwinecrt0.a -Wl,--dynamicbase --no-default-config
ld.lld: error: duplicate symbol: __chkstk
>>> defined at dlls/ntdll/x86_64-windows/signal_x86_64.o
>>> defined at libclang_rt.builtins-x86_64.a(chkstk.S.obj)
clang: error: linker command failed with exit code 1 (use -v to see invocation)
winegcc: /home/focht/projects/wine/llvm-mingw-20240130-ucrt-ubuntu-20.04-x86_64/bin/x86_64-w64-mingw32-gcc failed

Dumping symbols from compiler rt builtins:

llvm-nm -gC ./llvm-mingw-20240130-ucrt-ubuntu-20.04-x86_64/lib/clang/18/lib/windows/libclang_rt.builtins-x86_64.a | grep -B1 __chkstk
chkstk.S.obj:
00000000 T ___chkstk_ms
00000000 T __chkstk

Working case (only switching out toolchain):

llvm-mingw-20231128-ucrt-ubuntu-20.04-x86_64

llvm-nm -gC ./llvm-mingw-20231128-ucrt-ubuntu-20.04-x86_64/lib/clang/17/lib/windows/libclang_rt.builtins-x86_64.a | grep -B1 __chkstk
chkstk.S.obj:
00000000 T ___chkstk_ms
--
chkstk2.S.obj:
00000003 T ___chkstk

My guess would be that llvm upstream commit https://github.com/llvm/llvm-project/4580030171cce7168ddd06351333a2845246a4338ae64 caused the regression (part of llvmorg-18.1.0-rc1).

Regards

mati865 commented 5 months ago

GitHub says https://github.com/llvm/llvm-project/4580030171cce7168ddd06351333a2845246a4338ae64 doesn't exist.

mstorsjo commented 5 months ago

Dumping symbols from compiler rt builtins:

llvm-nm -gC ./llvm-mingw-20240130-ucrt-ubuntu-20.04-x86_64/lib/clang/18/lib/windows/libclang_rt.builtins-x86_64.a | grep -B1 __chkstk
chkstk.S.obj:
00000000 T ___chkstk_ms
00000000 T __chkstk

Working case (only switching out toolchain):

_llvm-mingw-20231128-ucrt-ubuntu-20.04-x8664

llvm-nm -gC ./llvm-mingw-20231128-ucrt-ubuntu-20.04-x86_64/lib/clang/17/lib/windows/libclang_rt.builtins-x86_64.a | grep -B1 __chkstk
chkstk.S.obj:
00000000 T ___chkstk_ms
--
chkstk2.S.obj:
00000003 T ___chkstk

My guess would be that llvm upstream commit https://github.com/llvm/llvm-project/4580030171cce7168ddd06351333a2845246a4338ae64 caused the regression (part of llvmorg-18.1.0-rc1).

The commit link appears to be broken, but it is indeed https://github.com/llvm/llvm-project/commit/885d7b759b5c166c07c07f4c58c6e0ba110fb0c2 that breaks this case.

The rationale for the upstream commit, is that both MSVC-style environments and mingw environments use a similar function, with the exact same calling convention etc, but they refer to it differently, ___chkstk_ms and __chkstk. So previously, we had an unused object file, chkstk2.S.obj, which contained a symbol ___chkstk (three underscores), which looks seemingly similar to the name used in the MSVC style environments, __chkstk, but works differently (and nothing ever uses it). So we got rid of it. And in case one uses a MSVC style environment, but wants compiler-rt builtins to provide the symbol, we'd provide it with the right name for MSVC as well - see https://discourse.llvm.org/t/compiler-rt-built-using-msvc-is-missing-chkstk/73059/7 for such a request.

For ARM/AArch64 there's less confusion, both MSVC and mingw style environments use the name __chkstk, which is provided in compiler-rt builtins. When Wine provides __chkstk themselves, nothing ever needs to pull it in from compiler-rt, and all is well. But for x86, the compiler generated code needs to pull in the object file from compiler-rt, which then now provides a symbol that conflicts with Wine's own symbol.

(It's a bit ironic that this is caught only now - I do test building latest wine with latest llvm-mingw every night, but I only do that for ARM/AArch64.)

I'd pull in @julliard and @cjacek from Wine, for opinions on what to do here. Should Wine itself provide the mingw style names as well, to avoid pulling in these files at all from compiler-rt builtins or libgcc, or should we just remove the problematic symbol from compiler-rt (or provide it as two object files, with one symbol each, even if the implementation is identical?

rmi1974 commented 5 months ago

Hello Martin,

yes, I meant indeed https://github.com/llvm/llvm-project/commit/885d7b759b5c166c07c07f4c58c6e0ba110fb0c2 Not sure how I ended up with that other sha1.

Regards,

cjacek commented 5 months ago

MSVC variants of *chkstk implementation is exported by ntdll.dll (like other similar builtin functions), so Wine currently doesn't need to do anything special about them. That won't work for mingw variants, because there is no export for them. We could, however, move it all into a static library, like both mingw and MSVC do. Here is a draft of something like that (it would need a proper implementation instead of no-ops): https://gitlab.winehq.org/jacek/wine/-/commit/d58157e758beeb66c614fed1178d86b6cc51eb63 This obviously won't help released Wine versions.

From LLVM point of view, I'm not sure how unique Wine is. In the article lined in the discourse topic (https://metricpanda.com/rival-fortress-update-45-dealing-with-__chkstk-__chkstk_ms-when-cross-compiling-for-windows/) there is a suggestion to provide a noop __chkstk. Sanity of that suggestion aside, if people indeed do things like that (and don't limit it to MSVC), then such projects will have the same problem as Wine. Having those symbols separated seems safer, I'm not sure.

(And to add to that irony, I build Wine git with LLVM both in msvc and mingw mode daily, but mingw uses a separated, more stable build, so only msvc uses git version of LLVM).

julliard commented 5 months ago

MSVC variants of *chkstk implementation is exported by ntdll.dll (like other similar builtin functions), so Wine currently doesn't need to do anything special about them. That won't work for mingw variants, because there is no export for them. We could, however, move it all into a static library, like both mingw and MSVC do. Here is a draft of something like that (it would need a proper implementation instead of no-ops): https://gitlab.winehq.org/jacek/wine/-/commit/d58157e758beeb66c614fed1178d86b6cc51eb63 This obviously won't help released Wine versions.

It's only an issue when linking ntdll, right? If so, we could simply add a private __chkstk_ms in ntdll. Still, it does feel like working around a mingw bug...

mstorsjo commented 5 months ago

MSVC variants of *chkstk implementation is exported by ntdll.dll (like other similar builtin functions), so Wine currently doesn't need to do anything special about them. That won't work for mingw variants, because there is no export for them. We could, however, move it all into a static library, like both mingw and MSVC do. Here is a draft of something like that (it would need a proper implementation instead of no-ops): https://gitlab.winehq.org/jacek/wine/-/commit/d58157e758beeb66c614fed1178d86b6cc51eb63 This obviously won't help released Wine versions.

Thanks for looking into it!

From LLVM point of view, I'm not sure how unique Wine is. In the article lined in the discourse topic (https://metricpanda.com/rival-fortress-update-45-dealing-with-__chkstk-__chkstk_ms-when-cross-compiling-for-windows/) there is a suggestion to provide a noop __chkstk. Sanity of that suggestion aside, if people indeed do things like that (and don't limit it to MSVC), then such projects will have the same problem as Wine. Having those symbols separated seems safer, I'm not sure.

It's only an issue when linking ntdll, right? If so, we could simply add a private __chkstk_ms in ntdll. Still, it does feel like working around a mingw bug...

Yeah, I think I agree here. For simplicity and safety's sake, we shouldn't provide both these symbols in the same object file in compiler-rt builtins. (This has been a recurring thing in mingw-w64-crt as well; whenever an object file provides more than one symbol, someone manages to make them conflict, somehow, where the solution usually is to split it into one symbol per object file.)

I'm not entirely sure if we should provide the MS name here at all quite yet... While I did add this in response to the linked discourse post, the change won't help directly as is anyway, because those files aren't built when building compiler-rt for a MSVC target anyway (see https://github.com/llvm/llvm-project/blob/main/compiler-rt/lib/builtins/CMakeLists.txt#L303-L341) - it would only help MSVC mode users with a mingw-built compiler-rt builtins library.

So ideally, we'd provide two separate object files, one with each symbol (or only one object file, with only the symbol that is relevant for the tool mode?). But perhaps the safest step for now would be to just skip the newly added extra symbol.

mstorsjo commented 4 months ago

A new prerelease with LLVM 18.1.0 RC 2 is out now, https://github.com/mstorsjo/llvm-mingw/releases/tag/20240207, and this issue should be fixed there unless I'm mistaken.

rmi1974 commented 4 months ago

A new prerelease with LLVM 18.1.0 RC 2 is out now, https://github.com/mstorsjo/llvm-mingw/releases/tag/20240207, and this issue should be fixed there unless I'm mistaken.

I can confirm, it works now. Thank you Martin!

For reference, LLVM upstream commit: https://github.com/llvm/llvm-project/commit/248aeac1ad2cf4f583490dd1312a5b448d2bb8cc

mstorsjo commented 4 months ago

Thanks for confirming! Let's close this issue then.