(call-process ...) hangs in emacs

[Ken Brown]

On 8/5/2014 9:58 AM, Corinna Vinschen wrote:
> On Aug  5 08:21, Ken Brown wrote:
>> On 8/4/2014 9:45 AM, Corinna Vinschen wrote:
>>> ...this, and the fact that fork/exec (vfork == fork on Cygwin) still
>>> works nicely in other scenarios points to some problem with the usage of
>>> pthread_mutexes in the application may be the culprit.
>>>
>>> For instance, is it possible that emacs expects the pthread_mutexes
>>> in malloc to be ERRORCHECK mutexes?  What if you explicitely set
>>> them to ERRORCHECK at creation time?
>>
>> That doesn't seem to be the issue, but I think I did find the problem, and
>> it looks like there might be both an emacs bug and a Cygwin bug. Here's the
>> relevant code from emacs's gmalloc.c:
>>
>> pthread_mutex_t _malloc_mutex = PTHREAD_MUTEX_INITIALIZER;
>> pthread_mutex_t _aligned_blocks_mutex = PTHREAD_MUTEX_INITIALIZER;
>>
>> [...]
>>
>>    /* Some pthread implementations call malloc for statically
>>       initialized mutexes when they are used first.  To avoid such a
>>       situation, we initialize mutexes here while their use is
>>       disabled in malloc etc.  */
>>    pthread_mutex_init (&_malloc_mutex, NULL);
>>    pthread_mutex_init (&_aligned_blocks_mutex, NULL);
>>
>>
>> The pthread_mutexes are initialized twice, resulting in undefined behavior
>> according to Posix.  That's the emacs bug.
>
> That's not the problem.  It's not necessary to call pthread_mutex_init
> on statically initialized mutexes, but it doesn't hurt either.  Only
> when calling pthread_mutex_init twice on the same object it goes
> downhill, especially when the first incarnation of the mutex was already
> locked.
>
>> But simply removing the static
>> initialization doesn't fix the problem.  On the other hand, the following
>> patch does seem to fix it, at least in preliminary testing:
>>
>> === modified file 'src/gmalloc.c'
>> --- src/gmalloc.c       2014-03-04 19:02:49 +0000
>> +++ src/gmalloc.c       2014-08-05 01:35:38 +0000
>> @@ -490,8 +490,8 @@
>>   }
>>
>>   #ifdef USE_PTHREAD
>> -pthread_mutex_t _malloc_mutex = PTHREAD_MUTEX_INITIALIZER;
>> -pthread_mutex_t _aligned_blocks_mutex = PTHREAD_MUTEX_INITIALIZER;
>> +pthread_mutex_t _malloc_mutex;
>> +pthread_mutex_t _aligned_blocks_mutex;
>>   int _malloc_thread_enabled_p;
>>
>>   static void
>> @@ -526,8 +526,11 @@
>>        initialized mutexes when they are used first.  To avoid such a
>>        situation, we initialize mutexes here while their use is
>>        disabled in malloc etc.  */
>> -  pthread_mutex_init (&_malloc_mutex, NULL);
>> -  pthread_mutex_init (&_aligned_blocks_mutex, NULL);
>> +  pthread_mutexattr_t attr1, attr2;
>> +  pthread_mutexattr_settype (&attr1, PTHREAD_MUTEX_NORMAL);
>> +  pthread_mutexattr_settype (&attr2, PTHREAD_MUTEX_NORMAL);
>> +  pthread_mutex_init (&_malloc_mutex, &attr1);
>> +  pthread_mutex_init (&_aligned_blocks_mutex, &attr2);
>>     pthread_atfork (malloc_atfork_handler_prepare,
>>                    malloc_atfork_handler_parent,
>>                    malloc_atfork_handler_child);
>>
>>
>> The first hunk avoids the double initialization, but I don't understand why
>> the second hunk does anything.  Since PTHREAD_MUTEX_NORMAL is now the
>> default, shouldn't calling pthread_mutex_init with NULL second argument be
>> equivalent to my calls to pthread_mutexattr_settype?  Does this indicate a
>> Cygwin bug, or am I misunderstanding something?
>
> AFAICS you're missing something.  Your pthread_mutexattr_t attr1, attr2
> are not initialized.  They contain some random values, thus they are not
> good objects.  The calls to pthread_mutexattr_settype as well as the
> calls to pthread_mutex_init will fail with EINVAL, but you won't see it
> due to missing error handling, and you end up without mutexes at all.
> If you call pthread_mutexattr_init before calling
> pthread_mutexattr_settype the situation shoul;d be the same as before.

Thanks for catching my mistake.  Your earlier suggestion about 
explicitly setting the pthread_mutexes to be ERRORCHECK mutexes seems to 
fix the problem (as long as I remember to call pthread_mutexattr_init). 
  The revised patch is attached.  I went back to using both the static 
and dynamic initializations as in the original code, since you said 
that's harmless.

Angelo and Katsumi, could you test it and see if it solves the problems 
you reported?  If so, I'll issue new emacs releases.

Ken
-------------- next part --------------
=== modified file 'src/gmalloc.c'
--- src/gmalloc.c	2014-03-04 19:02:49 +0000
+++ src/gmalloc.c	2014-08-05 17:30:18 +0000
@@ -490,8 +490,8 @@
 }
 
 #ifdef USE_PTHREAD
-pthread_mutex_t _malloc_mutex = PTHREAD_MUTEX_INITIALIZER;
-pthread_mutex_t _aligned_blocks_mutex = PTHREAD_MUTEX_INITIALIZER;
+pthread_mutex_t _malloc_mutex = PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP;
+pthread_mutex_t _aligned_blocks_mutex = PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP;
 int _malloc_thread_enabled_p;
 
 static void
@@ -526,8 +526,13 @@
      initialized mutexes when they are used first.  To avoid such a
      situation, we initialize mutexes here while their use is
      disabled in malloc etc.  */
-  pthread_mutex_init (&_malloc_mutex, NULL);
-  pthread_mutex_init (&_aligned_blocks_mutex, NULL);
+  pthread_mutexattr_t attr1, attr2;
+  pthread_mutexattr_init (&attr1);
+  pthread_mutexattr_init (&attr2);
+  pthread_mutexattr_settype (&attr1, PTHREAD_MUTEX_ERRORCHECK);
+  pthread_mutexattr_settype (&attr2, PTHREAD_MUTEX_ERRORCHECK);
+  pthread_mutex_init (&_malloc_mutex, &attr1);
+  pthread_mutex_init (&_aligned_blocks_mutex, &attr2);
   pthread_atfork (malloc_atfork_handler_prepare,
 		  malloc_atfork_handler_parent,
 		  malloc_atfork_handler_child);

-------------- next part --------------
--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple