Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

common: Fix multiple _xgetbv() error on GCC 8 #2419

Merged
merged 1 commit into from May 5, 2018

Conversation

turtleli
Copy link
Member

@turtleli turtleli commented May 4, 2018

GCC 8 now provides _xgetbv, so avoid using our own definition in that case.

Fixes #2417.

GCC 8 now provides _xgetbv, so avoid using our own definition in that
case.
@gregory38
Copy link
Contributor

it can be merged

@turtleli turtleli merged commit 15efe69 into PCSX2:master May 5, 2018
@turtleli turtleli deleted the fix-gcc8 branch May 5, 2018 23:53
@lucag73
Copy link

lucag73 commented May 6, 2018

I am not sure this is the proper fix...
now pcsx2 segfaults and does not start anymore.

@lucag73
Copy link

lucag73 commented May 6, 2018

More in detail: I have done a clean build of 8f6a3d9
(by checking out the tree anew and also clearing the compiler cache) and now pcsx2 fails with a segmentation fault:
Interface is initializing. Entering Pcsx2App::OnInit!
Applying operating system default language...
Command line parsing...
Command line parsed!
Segmentation fault (core dumped)

Using the original definition of _xgetbv (by reverting the file and removing the definition) works.

@lucag73
Copy link

lucag73 commented May 6, 2018

Here there are some more data on the segfault
From the output of gdb, it appears that the error happens in common/src/x86emitter/cpudetect.cpp

Core was generated by `./PCSX2'.
Program terminated with signal SIGSEGV, Segmentation fault.
#0 x86capabilities::Identify (this=0x1) at /home/lucag/Src/EMU/tst/common/src/x86emitter/cpudetect.cpp:189
189 if (isIdentified)
with the following backtrace

Thread 1 (Thread 0xf5328e80 (LWP 4453)):
#0 0x0836ed2a in x86capabilities::Identify() (this=0x1) at /home/lucag/Src/EMU/tst/common/src/x86emitter/cpudetect.cpp:189
#1 0x0836f0bb in x86capabilities::CountCores() (this=0x1) at /home/lucag/Src/EMU/tst/common/src/x86emitter/cpudetect.cpp:156
#2 0x00000000 in ()

actually, the last instructions executed are
│0x836ed1f x86capabilities::Identify()+1 mov %esp,%ebp
│0x836ed21 x86capabilities::Identify()+3 push %edi
│0x836ed22 x86capabilities::Identify()+4 push %esi
│0x836ed23 x86capabilities::Identify()+5 push %ebx
│0x836ed24 x86capabilities::Identify()+6 sub $0x2c,%esp
│0x836ed27 x86capabilities::Identify()+9 mov 0x8(%ebp),%esi
│0x836ed2a x86capabilities::Identify()+12 cmpb $0x0,(%esi)

but here esi=0x1 which does not make sense

@lucag73 lucag73 mentioned this pull request May 6, 2018
@gregory38
Copy link
Contributor

Esi is this. The pointer object. Maybe there is a stack corruption.

Anyway, do we really need xgetbv. Any modern OS should support AVX.

I'm surprised we don't have issue on gsdx side too

@lucag73
Copy link

lucag73 commented May 6, 2018 via email

@gregory38
Copy link
Contributor

Would you be able to dump asm code for cpudetect.cpp.o file in both case. You can use objdump.

@lucag73
Copy link

lucag73 commented May 6, 2018

Sure...
the "_orig" file contains the dump of cpudedect compiled with --debug using the unpatched source tree, while the "_patched" version is the same using the version of __xgetbv() of pcsx2 (but it was compiled with --release, so optimization options might differ).
If needed I can try and recompile also the patched version with --debug

cpudedetct_disasm_orig.txt
cpudedetct_disasm_patched.txt

@lucag73
Copy link

lucag73 commented May 6, 2018

Here there is a dump of a working cpudetect built with debug

cpudedetct_disasm_patched_debug.txt

@gregory38
Copy link
Contributor

Nice. Generated code seem corrupted too. Great. Potentially it is a 32 bits issue only.

@turtleli
Copy link
Member Author

turtleli commented May 6, 2018

Potentially it is a 32 bits issue only.

I think GCC's _xgetbv() is broken on 64-bit too. It immediately overwrites one of the output registers. https://godbolt.org/g/K1NnPk

@gregory38
Copy link
Contributor

I can understand that 32 bits is barely tested but seriously 64 bits...
I have a gcc bugzilla account. I will post your testcase, if I don't find a similar issue

@gregory38
Copy link
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants