Re: threadlib and emacs

[Bruno Haible]

Hi Paul,

> It's fine that Gnulib supports
> threadlib, but Gnulib shouldn't insist on threadlib as opposed to
> POSIX threads.
> ...
> POSIX threads are not perfect, but they're part of a standard
> interface that is reasonably well understood and they are good enough
> for many applications.  Gnulib by-and-large supports a POSIX interface
> in other areas, without having to add in a higher-level gnulib-only
> API and support library.

I agree with that. Nowadays, POSIX threads are more universally supported
than at the time when 'threadlib' was written, and the fact that pthreads-win32
is shipped with mingw-w64 on Cygwin gives it another push.

About a year ago, I already affirmed that it would be good to transform
the {lock,tls,cond,yield,thread} modules to something that provides the
newest POSIX API, supporting other thread APIs under the hood (older pthreads
and Win32 threads being the most important ones). But that refactoring will
take a couple of time and result in a larger pthread.in.h (sure!). (The
pthread_sigmask module alone took me one week to make work portably.)

> > If someone wants to use pselect() in a library, that library may be
> > linked into multithreaded programs. With your patch, the package that
> > provides the library and blindly requests 'pselect' will get a
> > pselect() emulation that is not multithread-safe.
> 
> That's a good point.  So let's omit that patch's change to the
> dependencies (in modules/pthread_sigmask).  That is, Emacs etc. can
> use --avoid=threadlib; the default can continue to drag in threadlib.

Good. Then that's fine with me.

When the proposed rewrite of the thread facilities will have happened,
we may be able to simplify much of the non-threadlib code that you are
adding now. But it's not a show-stopper that has to hold up your work
for Emacs.

> diff --git a/ChangeLog b/ChangeLog
> index a521e9b..8c9851f 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,13 @@
> +2011-07-19  Paul Eggert  <address@hidden>
> +
> +     pthread_sigmask: assume POSIX threads if not using threadlib
> +     * m4/pthread_sigmask.m4 (gl_FUNC_PTHREAD_SIGMASK):
> +     Assume POSIX threads only if gl_THREADLIB is not defined.
> +     Simplify the calculation of HAVE_PTHREAD_SIGMASK, by initially
> +     setting it to 0 if the initial check fails, and setting it to 1
> +     if a later, fancier check works; this makes it easier to understand
> +     the non-threadlib code.
> +
>  2011-07-16  Paul Eggert  <address@hidden>
>  
>       pthread_sigmask: ensure usleep is declared
> diff --git a/m4/pthread_sigmask.m4 b/m4/pthread_sigmask.m4
> index 3803988..4a4901f 100644
> --- a/m4/pthread_sigmask.m4
> +++ b/m4/pthread_sigmask.m4
> @@ -1,4 +1,4 @@
> -# pthread_sigmask.m4 serial 10
> +# pthread_sigmask.m4 serial 12
>  dnl Copyright (C) 2011 Free Software Foundation, Inc.
>  dnl This file is free software; the Free Software Foundation
>  dnl gives unlimited permission to copy and/or distribute it,
> @@ -6,16 +6,21 @@ dnl with or without modifications, as long as this notice 
> is preserved.
>  
>  AC_DEFUN([gl_FUNC_PTHREAD_SIGMASK],
>  [
> -  AC_REQUIRE([gl_THREADLIB])
> -
>    AC_CHECK_FUNCS_ONCE([pthread_sigmask])
> +  if test $ac_cv_func_pthread_sigmask != yes; then
> +    HAVE_PTHREAD_SIGMASK=0
> +  fi
>    LIB_PTHREAD_SIGMASK=
> +
> +  m4_ifdef([gl_[]THREADLIB], [
> +    dnl Assume threadlib is in use if its main symbol is defined.
> +    dnl Spell the symbol in a funny way, so that aclocal doesn't see it
> +    dnl and define it for us even if we don't want it.
> +    AC_REQUIRE([gl_[]THREADLIB])
>    if test "$gl_threads_api" = posix; then
> -    if test $ac_cv_func_pthread_sigmask = yes; then
> -      dnl pthread_sigmask is available without -lpthread.
> -      :
> -    else
> -      if test -n "$LIBMULTITHREAD"; then
> +      if test $ac_cv_func_pthread_sigmask != yes &&
> +         test -n "$LIBMULTITHREAD"
> +      then
>          AC_CACHE_CHECK([for pthread_sigmask in $LIBMULTITHREAD],
>            [gl_cv_func_pthread_sigmask_in_LIBMULTITHREAD],
>            [gl_save_LIBS="$LIBS"
> @@ -34,13 +39,8 @@ AC_DEFUN([gl_FUNC_PTHREAD_SIGMASK],
>          if test $gl_cv_func_pthread_sigmask_in_LIBMULTITHREAD = yes; then
>            dnl pthread_sigmask is available with -lpthread.
>            LIB_PTHREAD_SIGMASK="$LIBMULTITHREAD"
> -        else
> -          dnl pthread_sigmask is not available at all.
> -          HAVE_PTHREAD_SIGMASK=0
> +            HAVE_PTHREAD_SIGMASK=1
>          fi
> -      else
> -        dnl pthread_sigmask is not available at all.
> -        HAVE_PTHREAD_SIGMASK=0
>        fi
>      fi
>    else

Essentially fine with me, except that I prefer a patch that leaves the
if-else chain and the associated comments in place. Your change causes
HAVE_PTHREAD_SIGMASK to be initially 1, then sometimes set to 0, and then
in some cases set back to 1. Which is certainly shorter but not so easy
to understand and (in my opinion) more likely to introduce bugs in the
future.

> @@ -52,10 +52,31 @@ AC_DEFUN([gl_FUNC_PTHREAD_SIGMASK],
>      dnl link dependencies.
>      if test $ac_cv_func_pthread_sigmask = yes; then
>        REPLACE_PTHREAD_SIGMASK=1
> -    else
> -      HAVE_PTHREAD_SIGMASK=0
>      fi
>    fi
> +  ],[
> +    dnl Assume threadlib is not in use.
> +    dnl Assume POSIX.1-2008 (or later) semantics.  Do not fiddle with
> +    dnl compiler or linker options, since any application not
> +    dnl already properly configured for threads is most likely single
> +    dnl threaded and can use gnulib's sigprocmask-based substitute.
> +    if test $ac_cv_func_pthread_sigmask = yes; then
> +      AC_CACHE_CHECK([for pthread_sigmask with POSIX signature],
> +        [gl_cv_func_pthread_sigmask_posix_signature],
> +        [AC_LINK_IFELSE(
> +           [AC_LANG_PROGRAM(
> +              [[#include <signal.h>
> +              ]],
> +              [[return pthread_sigmask (0, (sigset_t *) 0, (sigset_t *) 
> 0);]])
> +           ],
> +           [gl_cv_func_pthread_sigmask_posix_signature=yes],
> +           [gl_cv_func_pthread_sigmask_posix_signature=no])])

What is this AC_CACHE_CHECK good for? I've listed all the portability bugs
that were encountered during porting in
doc/posix-functions/pthread_sigmask.texi, and a wrong signature was not
among the problems found. The only thing this AC_CACHE_CHECK does is to
drop support for MacOS X 10.3, FreeBSD 6.4, OpenBSD 3.8, OSF/1 4.0, Solaris 2.6,
but that is not needed, because lib/signal.in.h already contains the
fix for these systems (a #include <pthread.h>), and it does not require
configure-time checks.

> +      if test "$gl_cv_func_pthread_sigmask_posix_signature" != yes; then
> +        REPLACE_PTHREAD_SIGMASK=1
> +      fi
> +    fi
> +  ])
> +

I'm applying this simplified patch (again with "diff -w") in your and my
name. I have tested the patch on glibc, MacOS X 10.5, FreeBSD 6.4, OpenBSD 4.9,
AIX 7.1, HP-UX 11.31, IRIX 6.5, OSF/1 5.1, Solaris 7,8,9,10. The only
problematic finding was that on IRIX 6.5, I got

  ksh[10]: 11401472 Memory fault(coredump)
  FAIL: test-pthread_sigmask2

with a stack trace that looks like this:

#0  0x0fa5852c in _sigprocmask ()
    at 
/xlv51/patches/7100/work/irix/lib/libc/libc_n32_M4/signal/sigprocmask.c:46
#1  0x100018d4 in pthread_sigmask ()
    at 
/home/haible/multibuild-1165/irix65-cc/testdir1/gllib/pthread_sigmask.c:66
#2  0x0c06034c in _SGIPT_libc_sigprocmask ()
    at /xlv52/patches/7084/work/eoe/lib/libpthread/libpthread_n32_M3/sig.c:285
#3  0x0fa58550 in _sigprocmask ()
    at 
/xlv51/patches/7100/work/irix/lib/libc/libc_n32_M4/signal/sigprocmask.c:47
#4  0x100018d4 in pthread_sigmask ()
    at 
/home/haible/multibuild-1165/irix65-cc/testdir1/gllib/pthread_sigmask.c:66
#5  0x0c06034c in _SGIPT_libc_sigprocmask ()
    at /xlv52/patches/7084/work/eoe/lib/libpthread/libpthread_n32_M3/sig.c:285
#6  0x0fa58550 in _sigprocmask ()
    at 
/xlv51/patches/7100/work/irix/lib/libc/libc_n32_M4/signal/sigprocmask.c:47
...

The reason was
S["gl_LTLIBOBJS"]=" pthread_sigmask.lo"
S["gl_LIBOBJS"]=" pthread_sigmask.o"
S["REPLACE_PTHREAD_SIGMASK"]="0"
S["HAVE_PTHREAD_SIGMASK"]="0"
S["LIB_PTHREAD_SIGMASK"]=""
D["HAVE_RAW_DECL_PTHREAD_SIGMASK"]=" 1"
and the 'pthread_sigmask' symbol in the test-pthread_sigmask2 executable
overrides the 'pthread_sigmask' symbol from libpthread in a way that causes
an endless recursion. Defining 'rpl_pthread_sigmask' instead of
'pthread_sigmask' fixes it.

Anything still missing with that?


2011-07-19  Paul Eggert  <address@hidden>
            Bruno Haible  <address@hidden>

        pthread_sigmask: assume POSIX threads if --avoid=threadlib
        * m4/pthread_sigmask.m4 (gl_FUNC_PTHREAD_SIGMASK): If gl_THREADLIB is
        not defined, assume POSIX threads and look for pthread_sigmask in
        $LIBS, without changing $CPPFLAGS.

--- m4/pthread_sigmask.m4.bak   2011-07-16 02:11:38.000000000 +0200
+++ m4/pthread_sigmask.m4       2011-07-20 02:46:37.000000000 +0200
@@ -1,4 +1,4 @@
-# pthread_sigmask.m4 serial 10
+# pthread_sigmask.m4 serial 11
 dnl Copyright (C) 2011 Free Software Foundation, Inc.
 dnl This file is free software; the Free Software Foundation
 dnl gives unlimited permission to copy and/or distribute it,
@@ -6,10 +6,16 @@
 
 AC_DEFUN([gl_FUNC_PTHREAD_SIGMASK],
 [
-  AC_REQUIRE([gl_THREADLIB])
-
   AC_CHECK_FUNCS_ONCE([pthread_sigmask])
   LIB_PTHREAD_SIGMASK=
+
+  dnl Test whether the gnulib module 'threadlib' is in use.
+  dnl Some packages like Emacs use --avoid=threadlib.
+  dnl Write the symbol in such a way that it does not cause 'aclocal' to pick
+  dnl the threadlib.m4 file that is installed in $PREFIX/share/aclocal/.
+  m4_ifdef([gl_[]THREADLIB], [
+    AC_REQUIRE([gl_[]THREADLIB])
+
   if test "$gl_threads_api" = posix; then
     if test $ac_cv_func_pthread_sigmask = yes; then
       dnl pthread_sigmask is available without -lpthread.
@@ -56,6 +62,25 @@
       HAVE_PTHREAD_SIGMASK=0
     fi
   fi
+  ], [
+    dnl The module 'threadlib' is not in use, due to --avoid=threadlib being
+    dnl specified.
+    dnl The package either has prepared CPPFLAGS and LIBS for use of POSIX:2008
+    dnl threads, or wants to build single-threaded programs.
+    if test $ac_cv_func_pthread_sigmask = yes; then
+      dnl pthread_sigmask exists and does not require extra libraries.
+      dnl Assume that it is declared.
+      :
+    else
+      dnl pthread_sigmask either does not exist or needs extra libraries.
+      HAVE_PTHREAD_SIGMASK=0
+      dnl Define the symbol rpl_pthread_sigmask, not pthread_sigmask,
+      dnl so as to not accidentally override the system's pthread_sigmask
+      dnl symbol from libpthread. This is necessary on IRIX 6.5.
+      REPLACE_PTHREAD_SIGMASK=1
+    fi
+  ])
+
   AC_SUBST([LIB_PTHREAD_SIGMASK])
   dnl We don't need a variable LTLIB_PTHREAD_SIGMASK, because when
   dnl "$gl_threads_api" = posix, $LTLIBMULTITHREAD and $LIBMULTITHREAD are the

-- 
In memoriam Paul Schneider 
<http://en.wikipedia.org/wiki/Paul_Schneider_(pastor)>