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

gsdx: Avoid illegal instruction crash on older CPUs #1477

Merged
merged 6 commits into from Aug 2, 2016

Conversation

turtleli
Copy link
Member

Changes:

  • Defers initialisation of vectors to GSinit
  • Defers initialisation of GSdxApp theApp on Linux
  • Changes the library name string to a char array

Tested so far (using some QEMU VMs (Core 2(SSSE3), Nehalem(SSE42))):

  • VS2015 SSE4, AVX, AVX2
  • GCC 6.1.1 SSE4, AVX, AVX2

Haven't tested the PSX stuff.

I've added a temporary AppVeyor commit to build all GSdx configurations.

Fixes #999
Fixes #1423

@bositman
Copy link
Member

Tested on a non AVX2 machine, worked nicely. Maybe add the 'Your CPU does not support SSE blahblah" message in the error message pop-up as well as in the console as it currently is. Good job though :)

@ssakash ssakash added this to the Release 1.6 milestone Jul 25, 2016
@ramapcsx2
Copy link
Member

Very nice!
Hope to test this later today. Also on a non-AVX2 processor here.

@gregory38
Copy link
Contributor

Good jobs. For a lib name, a local string (not static) could maybe work? I'm fine with the buffer but is there a safe version of sprintf (snprintf ?) to limit the format to 255 byte.

@turtleli
Copy link
Member Author

Maybe add the 'Your CPU does not support SSE blahblah" message in the error message pop-up as well as in the console as it currently is

I think that would require extending the plugin API since the message box comes from PCSX2 and there currently isn't a way to pass error messages from the plugins. I kind of need to change the console message slightly so it doesn't tell me SSE5 isn't supported for the AVX/AVX2 plugins.

For a lib name, a local string (not static) could maybe work? I'm fine with the buffer but is there a safe version of sprintf (snprintf ?) to limit the format to 255 byte.

I think a local string would go out of scope. I suppose snprintf could be used, though it probably doesn't offer much safety in this case (user input does not affect string, string is unlikely to ever reach 255 characters long).

@gregory38
Copy link
Contributor

Yes it is unlikely to be unsafe. IMHO, safe by default is better and more future-proof. It is up to you.

Also add a newline to the error message and report AVX/AVX2 instead of
SSE5.00 and SSE5.01.
It can cause PCSX2 to crash if the instructions aren't supported.
Visual Studio 2015 initialises unordered_map using vector instructions,
which can cause PCSX2 to crash if the instructions aren't supported.
vector push_back causes a SIGILL signal on a Nehalem (SSE4.2) QEMU VM
when compiled with GCC 6.1.1.

However, an empty constructor causes illegal instruction exceptions to be
generated on a Windows VM.

So here's an inbetween that looks stupid but works on what I've tested.
"static string str;" causes a SIGILL signal on a "Nehalem" (SSE4.2)
QEMU VM when compiled with GCC 6.1.1.
@turtleli
Copy link
Member Author

Updated the console error message and used snprintf.

If I can be bothered then I'll check gcc 5.x, though if it needs changes it might be a lot of work (it would mean 3 VMs and a hell of a lot of compiling). If not, I'll just recheck my code and remove the AppVeyor commit before merging.

@ramapcsx2
Copy link
Member

Okay, this works here on Win10 x64, compiling with vs2015 and running on an AVX only cpu.
When selecting the AVX2 dll, I get a not supported popup from PCSX2.
I also tried the AVX2 dll in a PSX emulator > crashes when configuring / opening. This is probably the same as before.

@gregory38
Copy link
Contributor

Don't bother too much with gcc/linux. It will likely be as good as windows. Most people uses distribution packages which are limited to sse2 (maybe one day for avx).

@turtleli
Copy link
Member Author

Updated - should prevent crashing on PSX emulators now (but I didn't really test it on my VM).

Check the instruction set first in GPUinit, GPUconfigure and GPUtext
to prevent unsupported vector instructions from being executed.

Move the vector initialisation in GPUinit to a separate function - it
avoids a vzeroupper instruction.
@turtleli
Copy link
Member Author

turtleli commented Aug 1, 2016

Updated - it really shouldn't crash on PSX emulators now, but if it does, someone else can fix the issue (and it doesn't benefit from AVX2 anyway).

I'll probably merge tomorrow after I do a final check.

@turtleli turtleli merged commit f978f9a into PCSX2:master Aug 2, 2016
@turtleli turtleli deleted the gsdx-defer-init branch August 2, 2016 22:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants