search

PCSX2 / pcsx2

PCSX2 - The Playstation 2 Emulator
https://pcsx2.net
GNU General Public License v3.0
11.57k stars 1.6k forks source link

[BUG]: Compilation failure since 1.7.3669 #7543

Closed alucryd closed 1 year ago

alucryd commented 1 year ago

Describe the Bug

Pcsx2 has stopped building since 1.7.3669 with multiple undefined references, see log: https://paste.xinu.at/k26mTd0wZCwFKjyz/ Disabling LTO didn't help.

Reproduction Steps

Expected Behavior

No response

PCSX2 Revision

v1.7.3669

Operating System

Linux (64bit) - Specify Distro Below

If Linux - Specify Distro

Arch Linux

stenzek commented 1 year ago

Sorry, we have nothing to do with arch packaging, speak to whomever maintains it.

CMake builds themselves are fine, our CI uses it to produce the appimage.

weirdbeardgame commented 1 year ago

Though as the arch AUR packager. There's been no issue on my end either.

alucryd commented 1 year ago

Sorry, we have nothing to do with arch packaging, speak to whomever maintains it.

CMake builds themselves are fine, our CI uses it to produce the appimage.

I am whomever maintains it... There was no change on our end, so the recent breakage is definitely not on us, I'm quite taken aback by your reaction :/

Does your CI use -DDISABLE_ADVANCE_SIMD=ON? My guess is it doesn't, and that's what stopped working since I can spot multiple references to AVX2 in the error log.

@kenshen112 The AUR package also doesn't use -DDISABLE_ADVANCE_SIMD=ON, which probably explains why it still works fine.

weirdbeardgame commented 1 year ago

Yeah. That's most likely the case, https://github.com/PCSX2/pcsx2/commit/d47c9b0773a5c502e545f68dd0e7f59c3061bb2f https://github.com/PCSX2/pcsx2/commit/a8382ceb5073e41e9d1958dcd3b8721d5510be29

alucryd commented 1 year ago

Unfortunately, despite the commit name it would seem that fix is not enough.

stenzek commented 1 year ago

We only support the appimage, since that's all we have control over. You can check the scripts used to build the appimage, they're in this repo.

alucryd commented 1 year ago

I'm not sure how breaking a cmake flag and having control over a package are related, especially since our build recipes are open source and there for everyone to see and improve so control hardly means anything, but if the official stance is that breaking DISABLE_ADVANCE_SIMD is fine, a feature over which you have full control, might as well get rid of it altogether.

refractionpcsx2 commented 1 year ago

I'm unclear what the flag is even there for anymore

The reason advanced SIMD is set to be disabled is to support really old CPUs with only SSE2 support.

We don't support SSE2 anymore, haven't for ages, and all versions are now compiled in to one exe, so there's no need to set any flags to get different extensions out.

I'm not a linux person so I don't know if there's another reason, but if that's the only one, maybe it shouldn't exist anymore. But this was one of the reasons on of our users (and friend of the team) decided to not recommend your package https://www.reddit.com/r/linux_gaming/comments/ikyovw/pcsx2_official_arch_linux_package_not_recommended/

alucryd commented 1 year ago

@refractionpcsx2 Thanks for the clarification. Traditionally most linux distros target vanilla x86_64 to support old CPUs indeed, there are a few outliers, and Arch is actually working on an additional repository supporting x86_64 v3 (ie AVX2), but the majority of packages are SSE2 max at best.

That flag seemed to imply SSE2 was supported, and it worked fine albeit with less performance. If it's officially no longer the case (which is perfectly fine and understandable), I'll just drop the package until we add support for x86_64-v3.

TellowKrinkle commented 1 year ago

Currently, using -DDISABLE_ADVANCE_SIMD=ON will compile a build that supports SSE4 with accelerated code for AVX and AVX2. The CI does use it. Not using it will default to -march=native and compile for whatever CPU the build computer has

Can you test changing this to if (0)?

TellowKrinkle commented 1 year ago

Never mind, got someone else to test, it looks like the issue only occurs with ld, so while you wait for a full fix, you can use lld to avoid the issue.

helkaluin commented 1 year ago

For what it's worth, the same issue popped up after the same set of commits 3 days ago over at the Ubuntu daily PPA. Strangely enough, it only occurs for the 20.04 and 22.04 build targets, and the 22.10 and later build targets were fine.

The issue was resolved after I changed the CMAKE flags to remove -DLTO_PCSX2_CORE=ON and add -DCMAKE_INTERPROCEDURAL_OPTIMIZATION=ON.

If you want the full build logs of the failed compiles:

F0bes commented 1 year ago

I can reproduce the issue with both LD and LLD when CMAKE_INTERPRODECURAL_OPTIMIZATION=ON I was wrong when I thought LLD fixed the issue last night. Looks like LTO on the core is failing (which makes sense looking at the linker errors), what I think you've effectively done helkaluin is only LTO the frontend.

j8r commented 1 year ago

There is a similar issue for the Flatpak package.

stenzek commented 1 year ago

Imo the lto core option can be dropped, we don't use it in the CI. GCC LTO is way too slow anyway, clang is fine with lto'ing everything (the cmake interprocedural option).

Again, if you follow the build scripts from the CI, it'll work fine.

weirdbeardgame commented 1 year ago

Decided to experiment with the SIMD option. I was able to get it to compile if I enabled Clang. Not GCC even with LDD enabled. Though Clang + LTO is incredibly deadly with the AUR, https://aur.archlinux.org/packages/pcsx2-git

[1/527] Linking C static library 3rdparty/rcheevos/librcheevos.a
FAILED: 3rdparty/rcheevos/librcheevos.a 
: && /usr/bin/cmake -E rm -f 3rdparty/rcheevos/librcheevos.a && "CMAKE_C_COMPILER_AR-NOTFOUND" cr 3rdparty/rcheevos/librcheevos.a [...]
/bin/sh: line 1: CMAKE_C_COMPILER_AR-NOTFOUND: command not found
[2/527] Linking CXX static library 3rdparty/discord-rpc/libdiscord-rpc.a
FAILED: 3rdparty/discord-rpc/libdiscord-rpc.a 
: && /usr/bin/cmake -E rm -f 3rdparty/discord-rpc/libdiscord-rpc.a && "CMAKE_CXX_COMPILER_AR-NOTFOUND" cr 3rdparty/discord-rpc/libdiscord-rpc.a [...]
/bin/sh: line 1: CMAKE_CXX_COMPILER_AR-NOTFOUND: command not found
ninja: build stopped: subcommand failed.
stenzek commented 1 year ago

Sounds like aur's problem then. The toolchain file the CI generates works fine everywhere else.

F0bes commented 1 year ago

The toolchain file does not work for me with clang or gcc. LTO is the offending flag.

stenzek commented 1 year ago

With LTO core or -DCMAKE_INTERPROCEDURAL_OPTIMIZATION=ON?

TellowKrinkle commented 1 year ago

Anyone want to test that PR and see if it helps for them?

F0bes commented 1 year ago

With LTO core or -DCMAKE_INTERPROCEDURAL_OPTIMIZATION=ON?

Either.

Anyone want to test that PR and see if it helps for them?

Doing that right now.

stenzek commented 1 year ago

FWIW, I'd recommend following what the CI has to the letter anyway. LTO particularly (and to some extent the compiler/clang) has a significant impact on PCSX2, and having performance differences between different packages just creates even more of a headache for us. Especially when they go to us first for help, instead of the packager.

helkaluin commented 1 year ago

FWIW, I'd recommend following what the CI has to the letter anyway. LTO particularly (and to some extent the compiler/clang) has a significant impact on PCSX2, and having performance differences between different packages just creates even more of a headache for us. Especially when they go to us first for help, instead of the packager.

That was the reason I changed the cmake flags for the daily PPA to fit https://github.com/PCSX2/pcsx2/commit/a8382ceb5073e41e9d1958dcd3b8721d5510be29. I try to follow you guys' compilation decisions.

And this is a bit off-topic, but since you brought up the whole packaging headache thing --- when you guys deem the Qt port mature enough, it'll have a self-updater built-in, yes? By then I think it'll be prime time to close down the daily PPA (and the AUR and the Flatpak and other distro's own daily builds) since the official AppImage could satisfy the main draw of using those (i.e. easy updates).

weirdbeardgame commented 1 year ago

It already has auto updates. But that's no reason to close a package. More options the better. Plus AppImage will always have issues with things like libpcap where a native package wouldn't

stenzek commented 1 year ago

Yes, the appimage auto updates itself since some time ago. Doesn't need to use external daemons, it's self contained.

This also has the benefit of multiple channels, which we will be moving to eventually (monthly/quarterly/whatever and nightly).