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

Win32 version of ThreadImpl::entryPoint and WindowImplWin32::globalOnEvent leave the stack unaligned when compiled with GCC #412

Closed
Fanael opened this Issue Jun 21, 2013 · 9 comments

Comments

Projects
None yet
4 participants
@Fanael

Fanael commented Jun 21, 2013

GCC on x86 assumes the stack is 16-byte aligned. In absence of callbacks, this is not an issue, as GCC takes care to pad the stack so that it will be left aligned. However, ThreadImpl::entryPoint and WindowImplWin32::globalOnEvent are callbacks from the OS, which keeps the stack aligned only on 4-byte boundary.

Usually it's not a very big problem, because nothing requires 16-byte alignment… unless SSE or AVX is turned on. When it is, the compiler is free to generate a movaps or similar instruction that assumes 16-byte alignment to read from or write to the stack. Since the stack is not 16-byte alignment, this results in a CPU exception, leading to a crash.

This is especially problematic with regards to ThreadImpl::entryPoint, as it calls user code that can do essentially anything.

The solution is to mark these functions, and any other callbacks, should I've missed any, with __attribute__((__force_align_arg_pointer__)) to make sure the stack is realigned when entering these functions.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Jun 21, 2013

Member

leading to a crash

Did it happen to you? Or is it one of these things that never happen in the real world?

Member

LaurentGomila commented Jun 21, 2013

leading to a crash

Did it happen to you? Or is it one of these things that never happen in the real world?

@Fanael

This comment has been minimized.

Show comment
Hide comment
@Fanael

Fanael Jun 21, 2013

It did. It also happened to other people, though without SFML as the middleman.

Fanael commented Jun 21, 2013

It did. It also happened to other people, though without SFML as the middleman.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Jun 21, 2013

Member

Since it happened to you, can you provide a small code that reproduces this error so that I can test it when I fix it?

By the way, do you know what happens with other compilers in this case?

Member

LaurentGomila commented Jun 21, 2013

Since it happened to you, can you provide a small code that reproduces this error so that I can test it when I fix it?

By the way, do you know what happens with other compilers in this case?

@ghost ghost assigned LaurentGomila Jun 21, 2013

@Fanael

This comment has been minimized.

Show comment
Hide comment
@Fanael

Fanael Jun 21, 2013

Since it happened to you, can you provide a small code that reproduces this error so that I can test it when I fix it?

Sure. Crashes under Windows 2000 and XP, seems to work under Windows 7 x64, probably because Microsoft made some internal change.

// to be compiled with -O2 -msse
#include <xmmintrin.h>
#include <iostream>
#include <SFML/System/Thread.hpp>

void foo(__m128 x)
{
  float value;
  _mm_store_ss(&value, x);
  std::cout << "value = " << value << '\n';
}

void threadFn()
{
  __m128 x = _mm_set_ps1(5.0f);
  __m128 y = _mm_set_ps1(4.0f);
  foo(x);
  foo(y);
  foo(x);
  foo(y);
}

int main()
{
  sf::Thread thread(&threadFn);
  thread.launch();
  thread.wait();
}

By the way, do you know what happens with other compilers in this case?

Clang behaves the same way GCC does.

Other compilers assume 4-byte alignment and always emit code that align the stack when they need stronger alignment guarantees, so the problem doesn't occur.

Fanael commented Jun 21, 2013

Since it happened to you, can you provide a small code that reproduces this error so that I can test it when I fix it?

Sure. Crashes under Windows 2000 and XP, seems to work under Windows 7 x64, probably because Microsoft made some internal change.

// to be compiled with -O2 -msse
#include <xmmintrin.h>
#include <iostream>
#include <SFML/System/Thread.hpp>

void foo(__m128 x)
{
  float value;
  _mm_store_ss(&value, x);
  std::cout << "value = " << value << '\n';
}

void threadFn()
{
  __m128 x = _mm_set_ps1(5.0f);
  __m128 y = _mm_set_ps1(4.0f);
  foo(x);
  foo(y);
  foo(x);
  foo(y);
}

int main()
{
  sf::Thread thread(&threadFn);
  thread.launch();
  thread.wait();
}

By the way, do you know what happens with other compilers in this case?

Clang behaves the same way GCC does.

Other compilers assume 4-byte alignment and always emit code that align the stack when they need stronger alignment guarantees, so the problem doesn't occur.

@LaurentGomila LaurentGomila added the new label May 19, 2014

@LaurentGomila LaurentGomila removed their assignment May 19, 2014

@binary1248 binary1248 added the Windows label May 19, 2014

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Jul 5, 2014

Member

Is there still a need to address this issue?

The fix based on @Fanael's description would be trivial. Simply add a conditional (GCC + clang only) __attribute__((__force_align_arg_pointer__)) in front of ThreadImpl::entryPoint. If there is still a need for a fix, I can submit a simple patch in a matter of minutes if nobody else wants to do so...

Member

binary1248 commented Jul 5, 2014

Is there still a need to address this issue?

The fix based on @Fanael's description would be trivial. Simply add a conditional (GCC + clang only) __attribute__((__force_align_arg_pointer__)) in front of ThreadImpl::entryPoint. If there is still a need for a fix, I can submit a simple patch in a matter of minutes if nobody else wants to do so...

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Jul 5, 2014

Member

I can't test it in my XP VM at the moment, but I guess if you know how to fix it, then go ahead.

Member

eXpl0it3r commented Jul 5, 2014

I can't test it in my XP VM at the moment, but I guess if you know how to fix it, then go ahead.

binary1248 added a commit that referenced this issue Jul 13, 2014

Fixed a crash caused by the stack being unaligned when entering Threa…
…dImpl::entryPoint if compiled by GCC or clang and run on Windows XP 32-bit. (#412)
@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Jul 13, 2014

Member

This should be fixed in 520025d. Without it, the example code crashes in my Windows XP 32-bit VM and with it, it doesn't.

Member

binary1248 commented Jul 13, 2014

This should be fixed in 520025d. Without it, the example code crashes in my Windows XP 32-bit VM and with it, it doesn't.

@binary1248 binary1248 added s:accepted and removed s:unassigned labels Jul 13, 2014

@binary1248 binary1248 self-assigned this Jul 13, 2014

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Aug 11, 2014

Member

This branch has been added to my merge list, meaning it will be merged very soon, unless someone raises any concerns.

Member

eXpl0it3r commented Aug 11, 2014

This branch has been added to my merge list, meaning it will be merged very soon, unless someone raises any concerns.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Aug 12, 2014

Member

Has been tested again and confirmed to work, as such it's merged in 7ace909

Member

eXpl0it3r commented Aug 12, 2014

Has been tested again and confirmed to work, as such it's merged in 7ace909

@eXpl0it3r eXpl0it3r closed this Aug 12, 2014

@eXpl0it3r eXpl0it3r modified the milestones: 2.x, 2.2 Aug 12, 2014

jcowgill added a commit to jcowgill/SFML that referenced this issue Sep 22, 2014

Fixed a crash caused by the stack being unaligned when entering Threa…
…dImpl::entryPoint if compiled by GCC or clang and run on Windows XP 32-bit. (SFML#412)

(cherry picked from commit 520025d)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment