[Bug 50429] New: Serious Sam Engine / Serious Sam Classic broken MP with math functions from musl
https://bugs.winehq.org/show_bug.cgi?id=50429 Bug ID: 50429 Summary: Serious Sam Engine / Serious Sam Classic broken MP with math functions from musl Product: Wine Version: 5.15 Hardware: x86-64 OS: Linux Status: UNCONFIRMED Severity: normal Priority: P2 Component: msvcrt Assignee: wine-bugs(a)winehq.org Reporter: marc.pluhar+winehq(a)gmail.com Distribution: --- The problem described in #10229 reappears with version 5.15 of wine: Cross-Platform Network Multiplayer with Serious Sam Classic (First or Second Encounter) is not possible. After some time all clients, which run on a different platform, desync. I suspect the commit 8034d4179c88ae9176ecaae210c30e40084ae29c is responsible. To test this, it is best to start a dedicated server on a windows machine with the "DemoCoop" profile of The First Encounter and then try to connect from Serious Sam started under wine. The connection fails instantly with "CRC error in DIFF!". Tested with 784cb2060ab63076adc349dcb1d15a6cb5eb2bc4 (master) on Arch Linux x86-64. If using version 5.14 everything works as expected. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=50429 marc.pluhar+winehq(a)gmail.com changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |regression -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=50429 Olivier F. R. Dierick <o.dierick(a)piezo-forte.be> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |o.dierick(a)piezo-forte.be --- Comment #1 from Olivier F. R. Dierick <o.dierick(a)piezo-forte.be> --- Hello, Can you try to revert commit 8034d41 to be sure? Instructions to revert a commit can be found there: https://wiki.winehq.org/Regression_Testing#Reverting_the_patch Regards. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=50429 --- Comment #2 from Marc Pluhar <marc.pluhar+winehq(a)gmail.com> --- Hello, unfortunately things moved around a bit, so a simple revert of the commit on master did not work (conflicts). Seems all the MSVCRT_ functions were renamed. I did a bisect instead: 2cb6a1780c37ad8f3a261600c3b187a192486b22 is the first bad commit When I tried reverting this commit, it also errored out with conflicts. So I went to the 5.15-Tag and reverted the commit 2cb6a1780. This version works and I can once again connect to the windows dedicated server. I hope this helps. Kind regards -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=50429 Zebediah Figura <z.figura12(a)gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |z.figura12(a)gmail.com --- Comment #3 from Zebediah Figura <z.figura12(a)gmail.com> --- Thanks for the report. Possibly the most helpful thing to do is to add a line like FIXME("%.16e\n", x); to the top of our asin() function, and record a log (with default channels) to show what it's trying to take the arcsine of. We can then compare Wine against native and look for discrepancies. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=50429 --- Comment #4 from Marc Pluhar <marc.pluhar+winehq(a)gmail.com> --- Created attachment 69047 --> https://bugs.winehq.org/attachment.cgi?id=69047 arcsin_fixme_serious_sam_tfe This is a logfile of a failed connection attempt, there are 1000 calls to the function with 193 unique values for x. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=50429 Marc Pluhar <marc.pluhar+winehq(a)gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Regression SHA1| |2cb6a1780c37ad8f3a261600c3b | |187a192486b22 -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=50429 Marc Pluhar <marc.pluhar+winehq(a)gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |download URL| |https://archive.org/details | |/SeriousSamDemo -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=50429 --- Comment #5 from Zebediah Figura <z.figura12(a)gmail.com> --- Created attachment 69056 --> https://bugs.winehq.org/attachment.cgi?id=69056 asin assembly version I notice that the demo ships with its own msvcrt.dll. Is the same true of the actual game? I'm curious about the effects of using that one (I think you'd need to set the DLL override to n,b). It may be that it's a custom patch. The attached patch changes our asin implementation to one used as a fallback in glibc. It actually matches native 32-bit msvcrt from Windows 7 perfectly for all of the values from the log above. Please also try it. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=50429 --- Comment #6 from Marc P <marc.pluhar+winehq(a)gmail.com> --- Yes, the full game also ships the msvcrt.dll in the Bin folder. When I add a DLL override with (native, builtin) the game works (with master). This version of msvcrt.dll (sha1: 63a4fcd64ecea975c1b91de04702c68a9f2a3c7d) is the same in the demo and in the full version and does seem to be a vanilla version: https://fix4dll.com/msvcrt_dll The game also works correctly with your patch applied (I removed the dll override beforehand). Thank you for your help. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=50429 Piotr Caban <piotr.caban(a)gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |piotr.caban(a)gmail.com --- Comment #7 from Piotr Caban <piotr.caban(a)gmail.com> --- I've started looking on this bug today, here's what I have found so far: - native implementation sets sse2 status word, not x87 - current implementation returns correct values for all calls recorded in arcsin_fixme_serious_sam_tfe file - current implementation returns different values than native in some cases, e.g.: asin(-1.08632087707519531250000e-01) = -1.08846890318393410557185e-01, expected -1.08846890318393396679397e-01 - we used to implement asin(x) as atan2(x, sqrt((1 - x) * (1 + x))), it's probably not going to fix the problem because both atan2 and sqrt needs precision fixes as well I guess that the proper solution is to implement __libm_sse2_asin and call it inside asin. I didn't test yet if the function is affected by _set_SSE2_enable function. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=50429 --- Comment #8 from Zebediah Figura <z.figura12(a)gmail.com> --- (In reply to Piotr Caban from comment #7)
I've started looking on this bug today, here's what I have found so far: - native implementation sets sse2 status word, not x87 - current implementation returns correct values for all calls recorded in arcsin_fixme_serious_sam_tfe file - current implementation returns different values than native in some cases, e.g.: asin(-1.08632087707519531250000e-01) = -1.08846890318393410557185e-01, expected -1.08846890318393396679397e-01 - we used to implement asin(x) as atan2(x, sqrt((1 - x) * (1 + x))), it's probably not going to fix the problem because both atan2 and sqrt needs precision fixes as well
I guess that the proper solution is to implement __libm_sse2_asin and call it inside asin. I didn't test yet if the function is affected by _set_SSE2_enable function.
Is that also true for 32-bit? My testing showed that behaviour differed between msvcrt versions and architectures, but that 32-bit msvcrt returns the same values as the attached x87 implementation on both windows 7 and windows 10. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=50429 --- Comment #9 from Piotr Caban <piotr.caban(a)gmail.com> --- (In reply to Zebediah Figura from comment #8)
Is that also true for 32-bit? I was testing the sse control word with 32-bit ucrtbase. It's not set in msvcrt.dll from the game. I wonder how it works with msvcrt.dll from Windows 7 or 10 (I'll take a look tomorrow)?
I guess we might reimplement and call _CIasin in this case. Probably using sse implementation will not be visible for most of the applications anyway (if we decide to use the same code for msvcrt and ucrtbase). -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=50429 --- Comment #10 from Zebediah Figura <z.figura12(a)gmail.com> --- Here are the tests I did: 32-bit msvcrt: Results match x87 version using fpatan exactly. Results are affected by setting x87 control word precision flags beforehand. 64-bit msvcrt: A different set of results. Also affected by setting x87 precision flags. 32-bit ucrtbase: A different set of results. Not affected by x87 precision flags. 64-bit ucrtbase: A different set of results. Not affected by x87 precision flags. 32-bit wine results do seem to perfectly match 32-bit ucrtbase results. I didn't test with any other value than in the logs attached, though. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=50429 --- Comment #11 from Piotr Caban <piotr.caban(a)gmail.com> --- Created attachment 69076 --> https://bugs.winehq.org/attachment.cgi?id=69076 asin test app Let's focus on 32-bit for now - this is what this bug is about. Your test result are not matching with mine, I'm attaching test application I've used. I have also tested the function on all values listed in attached log. The attached test compares values returned by ucrtbase and msvcrt. It can be run on Windows or on Wine (e.g. with native ucrtbase). Sample usage: i686-w64-mingw32-gcc test.c a.exe asin 1 10000 |grep -v "status word error" or to change precision you can run: a.exe asin 1 10000 0xa001f Here's what I can see: - ucrtbase returns exactly the same values as msvcrt for all tested values (tested on Windows XP, 7, 10) (it includes both random arguments and all the arguments from attached log) Note that x87 returned values are not guaranteed to match on different CPUs. It might affect the test results. This is one of the advantages of using sse. Here are some more notes: - ucrtbase uses sse instructions if x87 control word is not changed - ucrtbase uses x87 implementation after changing x87 (and sse) rounding mode - msvcrt uses x87 implementation - x87 precision control is not affecting the results on Windows (both in msvcrt and ucrtbase) What was not tested? I didn't check if the game changes x87 control word. I've tested only changing precision and rounding control words. What would be interesting to test? If the game has similar problems when game is run on Windows boxes with CPU's that are returning different values for asin (if there are any). Long story short: the main differences in test results are x87 precision flags effect and values comparison between msvcrt and ucrtbase. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=50429 --- Comment #12 from Piotr Caban <piotr.caban(a)gmail.com> --- I've done some more tests comparing msvcrt.dll shipped with game and in Windows 10. It turns out that the dll shipped with the game is older and produces different results for some arguments. While we need to update the math functions to match with native whenever possible I think that in this case the correct fix is to use native dll. We will be trying to match with newer versions of msvcrt.dll anyway. Here's an example of test that shows the difference: Windows 10: asin(4.68926392495632171630859e-02) = 4.69098418012151852085623e-02 Windows 10 using game supplied dll: asin(4.68926392495632171630859e-02) = 4.69098418012151782696684e-02 -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=50429 --- Comment #13 from Zebediah Figura <z.figura12(a)gmail.com> --- Created attachment 69080 --> https://bugs.winehq.org/attachment.cgi?id=69080 asin tests Attached are my (much more crude) tests. I get different results between msvcrt and ucrtbase even on an up to date windows 10 (real metal) box. For example, msvcrt says asin(4.9649044871330261e-01) == 5.1955101354071775e-01, but ucrtbase has that ending in 1787. The msvcrt value also matches a stock windows 7 install running in a VM. DLL versions are: win10 msvcrt: 7.0.19041.546 win10 ucrtbase: 10.0.19041.546 win7 msvcrt: 7.0.7600.16385 win10 processor is Intel Core i5-2400. Machine running the win7 VM is AMD FX 8350. When running your test program on windows 10 I see a bunch of status word mismatches but no value mismatches. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=50429 --- Comment #14 from Piotr Caban <piotr.caban(a)gmail.com> --- (In reply to Zebediah Figura from comment #13)
When running your test program on windows 10 I see a bunch of status word mismatches but no value mismatches. It's because wine's tests are using different x87 cw. You can see similar value mismatches after setting cw to 0x9001f.
-- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=50429 --- Comment #15 from Zebediah Figura <z.figura12(a)gmail.com> --- (In reply to Piotr Caban from comment #14)
(In reply to Zebediah Figura from comment #13)
When running your test program on windows 10 I see a bunch of status word mismatches but no value mismatches. It's because wine's tests are using different x87 cw. You can see similar value mismatches after setting cw to 0x9001f.
Indeed. I get identical results between msvcrt and ucrtbase if I use 24-bit or 64-bit precision; only 53-bit precision shows a difference. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=50429 --- Comment #16 from Piotr Caban <piotr.caban(a)gmail.com> --- It should be fixed by a6e3987e5b58cac29f003b20325373deadbd90af. Please retest. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=50429 Marc P <marc.pluhar+winehq(a)gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|UNCONFIRMED |RESOLVED Resolution|--- |FIXED --- Comment #17 from Marc P <marc.pluhar+winehq(a)gmail.com> --- (In reply to Piotr Caban from comment #16)
It should be fixed by a6e3987e5b58cac29f003b20325373deadbd90af. Please retest.
I tested with current master (7a9745022b1bfcc235b922be98a8fdc91976c587) and removed the DLL overrides. Both The First Encounter and The Second Encounter are now cross-platform playable again. Thanks for your work. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=50429 Marc P <marc.pluhar+winehq(a)gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Fixed by SHA1| |a6e3987e5b58cac29f003b20325 | |373deadbd90af -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=50429 Alexandre Julliard <julliard(a)winehq.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|RESOLVED |CLOSED --- Comment #18 from Alexandre Julliard <julliard(a)winehq.org> --- Closing bugs fixed in 6.2. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
participants (1)
-
WineHQ Bugzilla