Hi, PS: this was intended to be short; well, I've failed at that but I believe it's worth a read and the whole topic might well matter to you even if you don't care about Windows since the main concern is with the ABI of Eina_Thread.
A bit more context to this: the EFLs currently use the win32 API directly to expose a pthread-like API. The code is roughly 1k lines and, surprisingly, involves thread code (and everyone loves debugging threading issues). It is also very specific to windows with some bits which highlight win32's beauty (clock_gettime() you say? with CLOCK_MONOTONIC? surely noone wants a simple way to do that?). Of course, it has some bugs. This has lead to the idea of switching to winpthreads. Winpthreads is a library that has been created by mingw-w64 to provide a back-end to GCC for C++11 and C11 thread support. It has a design difference with pthreads-win32 that I didn't understand at first: its pthread_t type is an integer while pthreads-win32's is a struct (more on this below). Winpthreads is used quite heavily nowadays and has an active upstream. Moreover, since it is used by GCC for C++11 thread support, it is available for every toolchain which supports this configuration (and likely is already a dependency). It would better to have a configure-time check but it isn't strictly needed. The library is slightly young but is already proven. The main concern I've seen is report of lower performance compared to directly using the win32 API but it should be solved eventually and the mingw-w64 upstream is very friendly (did I mention I was fully objective even though I am part of it?). After some quick testing, it was found that using winpthreads improved the testsuite scores and solved errors for which the only possible reaction upon seeing the corresponding backtrace is "errr, yeah, hmmm, sure, why not, maybe, erm, well, let's go shopping". And didn't introduce new errors. That's always something positive. :) On Thu, Dec 18, 2014, Michelle Legrand wrote: > Hello, > > I am facing an issue with Enventor on Windows which leads Enventor to crash > immediately with a segmentation fault at launch. The error tracked by GDB is > in the following assignement in enventor/src/lib/auto_comp.c > (https://git.enlightenment.org/tools/enventor.git/tree/src/lib/auto_comp.c) : > ad->init_thread = ecore_thread_run(init_thread_cb, init_thread_end_cb, > init_thread_cancel_cb, ad); > > We found out that this segmentation fault is due to the type Eina_Thread > being defined by a unsigned long int which on Windows seems to have an > unsuffisant size. The reason is that Windows uses the LLP64 data models as > described on > http://en.wikipedia.org/wiki/64-bit_computing#64-bit_data_models. The exact issue is caused by a cast from Eina_Thread to pthread_t. Eina_Thread is an unsigned long int and pthread_t is ... implementation-defined. The code works only by chance. If my memory is working well enough (and the Traquair Jacobite Ale isn't too strong), the following is a reproducer for the bad code: /* from eina_thread.h */ typedef unsigned long int Eina_Thread; /* somewhere else in the EFLs */ Eina_Thread thread; pthread_something((pthread_t*) &thread); The following with an array is probably more explicit: void foo(uint32_t arr[2]) { arr[0] = 0xdead; arr[1] = 0xbeef; } void f(void) { uint32_t foo[1]; foo((uint32_t*) foo); } Result: stack has been smashed. Note that for extra fun, no stack-smashing detection finds this because the uint32_t is followed by padding (or, if the compiler is smart, by another stack-allocated variable that fits in the gap). (for extra extra fun, the pointer ends up in the rbx register which is callee-saved, which makes it possible for the callee to corrupt its callers' data) There's actually one (or more) intermediate function call(s) between the creation of the variable and the call to pthread_something(). This makes the compiler unable to just see > There would be two solutions to correct this problem. One would be to define > Eina_Thread as unsigned long long int for Win32 exclusively, or we could use > a standard integer type as uint64_t. > I would like to have your opinion about this and what would be the best > solution to adopt. A concern that arose on IRC is that some unspecified french borker has learned about the concept of "defensive code" and has therefore sprinkled his code with traps and especially, cast to longs. If you've followed the WP link above (btw, it can be accessed through the shorter URI: http://en.wikipedia.org/wiki/LLP64 ), you've seen that Windows x86_64 has longs of only 32 bits and it makes it impossible to use longs for storage of several datatypes (especially pointers). I believe it is doable to review the pthread-specific code for casts to longs and that being able to smithe the win32-specific code will very quickly reduce the maintenance burden. Thinking about it now, first step would probably be to remove the cast to pthread_t* and let the compiler complain if it feels like French today. Another issue is that eina_thread.h exposes the bad line: typedef unsigned long int Eina_Thread; This is wrong because pthread_t can be anything and the fact that it is an int is only the "common" case. The issue has been introduced by the following commit: commit 09748cfb154a314b3f98add4e0b98b30f9839df2 Author: Gustavo Sverzut Barbieri <barbi...@gmail.com> Date: Mon Dec 31 17:26:33 2012 +0000 efl: beef thread documentation and error reporting. eina_thread_join() is nasty and didn't report errors :-( I'm using Eina_Error here, but it's global to the application and not thread-local. Maybe we should make eina_error_get() and eina_error_set() thread-local storage? SVN revision: 81936 The net effect for eina_thread.h is: -typedef pthread_t Eina_Thread; +typedef unsigned long int Eina_Thread; IOW, this commit (which was supposed to be documentation) has made the implementation of Eina_Thread public and possibly incorrect. Also, considering the timestamp, I believe this awards k-s a shiny first-class-borker medal. Therefore, one way to go would be to hide again the implementation of Eina_Thread by making it a typedef of pthread_t again. I don't think it is currently known whether this will require additional work. This is probably the more correct change but might be riskier than simply #ifdef'ing a typedef to the right size for windows x86_64. As Michelle said: what do you think should be done? Regards, Adrien Nader ------------------------------------------------------------------------------ Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server from Actuate! Instantly Supercharge Your Business Reports and Dashboards with Interactivity, Sharing, Native Excel Exports, App Integration & more Get technology previously reserved for billion-dollar corporations, FREE http://pubads.g.doubleclick.net/gampad/clk?id=164703151&iu=/4140/ostg.clktrk _______________________________________________ enlightenment-devel mailing list enlightenment-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
