GNOME Bugzilla – Bug 306907
checking for weak attribute support
Last modified: 2018-07-01 08:47:21 UTC
Please describe the problem: libgamin/gam_data.c attempts to avoid using pthreads through the use of weak symbols if the application did not link to libpthread itself. Currently, this is only done on GNU/Linux platforms. This can easily be extended to any platform which accepts __attribute__ ((weak)). The right way to check for this is in autoconf. This, however, is not enough: POSIX does not mandate that e.g. pthread_mutex_lock is a function call only that there must be a function call. On the Hurd, pthread_mutex_lock is also a macro. The right way to use weak functions, then, is to call them by address. For instance, assign pthread_mutex_lock to a variable and then use the variable. The attached patch also fixes a few buglets: 1) pthread_mutex_init was not declared weak (pthread_mutexattr_init was declared as weak twice) 2) pthread_mutex_destroy was not declared weak 3) when calling pthread_mutex_unlock and pthread_mutex_destroy in gamin_data_free, they are not protected by an if (is_threaded > 0) 4) PTHREAD_MUTEX_RECURSIVE_NP is used where POSIX requires PTHREAD_MUTEX_RECURSIVE. The _NP means non-portable. Please let me know if you prefer that I file buglet reports separately. Steps to reproduce: 1. 2. 3. Actual results: Expected results: Does this happen every time? Other information:
Created attachment 47451 [details] [review] patch
Hum, let's say I did not trust too much the compilers, i.e. between the fact they accept to compile it from a .c and the fact it will work from a shared library potentially loaded dynamically and not though ld.so there is a huge gap. Your version side on the very optimistic and mine on the very pessimistic. I did check manually that the way it was done worked on Linux even from a dynamically loaded shared library in both context (program linked to pthread or not). To me the autoconf test provided tests 5% of the real requirements for this to work, there is then 2 approaches: - optimistic, apply the patch and wait for complaints/platforms fixes - pessimistic, only take (platforms/compiler) where this actually work considering that gamin is a 0.1.0 it might be okay to go with the optimistic approach, but I would prefer something more thorough at the configure level I need to think a bit and test on Linux for potential ABI regression. Daniel
Hi, what's the status on that? Gamin fails to build on GNU/Hurd currently due to this error: cc -DHAVE_CONFIG_H -I. -I. -I.. -I.. -I../lib -DBINDIR=\"/usr/lib/gamin\" -DBUILDDIR=\"..\" -DGAM_DEBUG_ENABLED -g -Wall -O2 -MT gam_data.lo -MD -MP -MF .deps/gam_data.Tpo -c gam_data.c -fPIC -DPIC -o .libs/gam_data.o gam_data.c: In function 'gamin_data_new': gam_data.c:474: error: 'PTHREAD_MUTEX_RECURSIVE_NP' undeclared (first use in this function) gam_data.c:474: error: (Each undeclared identifier is reported only once gam_data.c:474: error: for each function it appears in.) make[3]: *** [gam_data.lo] Error 1 Neal said: > 4) PTHREAD_MUTEX_RECURSIVE_NP is used where POSIX requires > PTHREAD_MUTEX_RECURSIVE. The _NP means non-portable. Could we have this fixed, while the rest of the patch is being considered? Should we send a seperate patch for this issue? thanks, Michael
This is CVS head: paphio:~/gamin/libgamin -> grep -C 3 PTHREAD_MUTEX_RECURSIVE *.c gam_data.c- } gam_data.c- if (is_threaded > 0) { gam_data.c- pthread_mutexattr_init(&attr); gam_data.c:#ifndef PTHREAD_MUTEX_RECURSIVE gam_data.c: pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE_NP); gam_data.c-#else gam_data.c: pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE); gam_data.c-#endif gam_data.c- pthread_mutex_init(&ret->lock, &attr); gam_data.c- pthread_mutexattr_destroy(&attr); paphio:~/gamin/libgamin -> i.e. I think it's fixed, I expect PTHREAD_MUTEX_RECURSIVE to be a macro if defined. Daniel
The last commit to that file broke this again: #ifdef __GLIBC__ pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE_NP); as __GLIBC__ is #defined on GNU/Hurd as well. What is the reason to use _NP here? Michael
because otherwise it doesn't compile on Linux as far as I remember. Daniel
Indeed: gcc -DHAVE_CONFIG_H -I. -I. -I.. -I.. -I../lib -DBINDIR=\"/usr/local/libexec\" -DBUILDDIR=\"..\" -DGAM_DEBUG_ENABLED -Wall -Wchar-subscripts -Wmissing-declarations -Wmissing-prototypes -Wnested-externs -Wsign-compare -Wno-sign-compare -g -c gam_data.c -Wp,-MD,.deps/gam_data.TPlo -fPIC -DPIC -o .libs/gam_data.o gam_data.c: In function `gamin_data_new': gam_data.c:460: `pthread_mutexattr_settype' undeclared (first use in this function) gam_data.c:460: (Each undeclared identifier is reported only once gam_data.c:460: for each function it appears in.) gam_data.c:474: `pthread_mutexattr_settype' used prior to declaration gam_data.c:474: warning: implicit declaration of function `pthread_mutexattr_settype' gam_data.c:474: `PTHREAD_MUTEX_RECURSIVE' undeclared (first use in this function) make[2]: *** [gam_data.lo] Error 1 OK, I'll try to ask Neal about this, a hacky solution would be to s/__GLIBC__/__linux__/ (not sure whether this would break GNU/k*BSD perhaps) Michael
I got somebody to try building on GNU/k*BSD and __GLIBC__ is needed, it fails in the same way as Linux above otherwise (after updating config.{guess,sub} to actually recognize it).
Adding "#define _GNU_SOURCE" at the top of libgamin/gam_data.c makes it build fine on GNU/LINUX with `PTHREAD_MUTEX_RECURSIVE' instead of `PTHREAD_MUTEX_RECURSIVE_NP', does that sound OK? Michael
gamin is not under active development anymore and has not seen code changes for many years. Its codebase has been archived: https://gitlab.gnome.org/Archive/gamin/commits/master Closing this report as WONTFIX as part of Bugzilla Housekeeping to reflect reality. Please feel free to reopen this ticket (or rather transfer the project to GNOME Gitlab, as GNOME Bugzilla is deprecated) if anyone takes the responsibility for active development again.