Re: Pretest?

[Giorgos Keramidas]

On 2007-03-19 18:31, YAMAMOTO Mitsuharu <address@hidden> wrote:
> >>>>> On Mon, 19 Mar 2007 09:00:10 +0100, Jan Dj?rv <address@hidden> said:
> 
> > A thing to do after the release might be to synchronize with the
> > thread safe malloc in glibc.
> 
> What do we do before the release?  Possible options would be:
> 
>   1) Use the system malloc library (and abandon emacs_blocked_*) if
>      HAVE_GTK_AND_PTHREAD.
>   2) Modify src/gmalloc.c to make it thread safe.
> 
> I just tried the latter, but I can't test it myself.

Thanks for the patch :-)

Since I was the one reporting instability with Emacs compiled
with GTK+ support, I can roll an Emacs build with GTK+ enabled,
and see if this makes things more stable here.

I just updated my local copy of the CVS repository with all
changes up to:

    changeset:   80041:df347cacb5a9
    tag:         tip
    user:        gm
    date:        Mon Mar 19 20:59:53 2007 +0000
    summary:     Change form of license text to match rest of Emacs.

and I'm building a new snapshot now...

> Index: src/gmalloc.c
> ===================================================================
> RCS file: /cvsroot/emacs/emacs/src/gmalloc.c,v
> retrieving revision 1.20
> diff -c -p -r1.20 gmalloc.c
> *** src/gmalloc.c     21 Jan 2007 04:18:16 -0000      1.20
> --- src/gmalloc.c     19 Mar 2007 09:18:53 -0000
> ***************
> *** 1,6 ****
> --- 1,9 ----
>   /* This file is no longer automatically generated from libc.  */
>   
>   #define _MALLOC_INTERNAL
> + #ifdef HAVE_GTK_AND_PTHREAD
> + #define USE_PTHREAD
> + #endif
>   
>   /* The malloc headers and source files from the C library follow here.  */
>   
> *************** Fifth Floor, Boston, MA 02110-1301, USA.
> *** 73,78 ****
> --- 76,85 ----
>   #include <unistd.h>
>   #endif
>   
> + #ifdef USE_PTHREAD
> + #include <pthread.h>
> + #endif
> + 
>   #endif      /* _MALLOC_INTERNAL.  */
>   
>   
> *************** extern __ptr_t _malloc_internal PP ((__m
> *** 229,234 ****
> --- 236,244 ----
>   extern __ptr_t _realloc_internal PP ((__ptr_t __ptr, __malloc_size_t 
> __size));
>   extern void _free_internal PP ((__ptr_t __ptr));
>   
> + #ifdef USE_PTHREAD
> + extern pthread_mutex_t _malloc_mutex;
> + #endif
>   #endif /* _MALLOC_INTERNAL.  */
>   
>   /* Given an address in the middle of a malloc'd object,
> *************** register_heapinfo ()
> *** 536,548 ****
>       _heapinfo[block + blocks].busy.info.size = -blocks;
>   }
>   
> ! /* Set everything up and remember that we have.  */
> ! int
> ! __malloc_initialize ()
> ! {
> !   if (__malloc_initialized)
> !     return 0;
>   
>   #ifdef GC_MCHECK
>     mcheck (NULL);
>   #endif
> --- 546,559 ----
>       _heapinfo[block + blocks].busy.info.size = -blocks;
>   }
>   
> ! #ifdef USE_PTHREAD
> ! static pthread_once_t malloc_init_once_control = PTHREAD_ONCE_INIT;
> ! pthread_mutex_t _malloc_mutex;
> ! #endif
>   
> + static void
> + malloc_initialize_1 ()
> + {
>   #ifdef GC_MCHECK
>     mcheck (NULL);
>   #endif
> *************** __malloc_initialize ()
> *** 550,559 ****
>     if (__malloc_initialize_hook)
>       (*__malloc_initialize_hook) ();
>   
>     heapsize = HEAP / BLOCKSIZE;
>     _heapinfo = (malloc_info *) align (heapsize * sizeof (malloc_info));
>     if (_heapinfo == NULL)
> !     return 0;
>     memset (_heapinfo, 0, heapsize * sizeof (malloc_info));
>     _heapinfo[0].free.size = 0;
>     _heapinfo[0].free.next = _heapinfo[0].free.prev = 0;
> --- 561,581 ----
>     if (__malloc_initialize_hook)
>       (*__malloc_initialize_hook) ();
>   
> + #ifdef USE_PTHREAD
> +   {
> +     pthread_mutexattr_t attr;
> + 
> +     pthread_mutexattr_init (&attr);
> +     pthread_mutexattr_settype (&attr, PTHREAD_MUTEX_RECURSIVE);
> +     pthread_mutex_init (&_malloc_mutex, &attr);
> +     pthread_mutexattr_destroy (&attr);
> +   }
> + #endif
> + 
>     heapsize = HEAP / BLOCKSIZE;
>     _heapinfo = (malloc_info *) align (heapsize * sizeof (malloc_info));
>     if (_heapinfo == NULL)
> !     return;
>     memset (_heapinfo, 0, heapsize * sizeof (malloc_info));
>     _heapinfo[0].free.size = 0;
>     _heapinfo[0].free.next = _heapinfo[0].free.prev = 0;
> *************** __malloc_initialize ()
> *** 565,571 ****
>   
>     __malloc_initialized = 1;
>     PROTECT_MALLOC_STATE (1);
> !   return 1;
>   }
>   
>   static int morecore_recursing;
> --- 587,609 ----
>   
>     __malloc_initialized = 1;
>     PROTECT_MALLOC_STATE (1);
> !   return;
> ! }
> ! 
> ! /* Set everything up and remember that we have.  */
> ! int
> ! __malloc_initialize ()
> ! {
> ! #ifdef USE_PTHREAD
> !   pthread_once (&malloc_init_once_control, malloc_initialize_1);
> ! #else
> !   if (__malloc_initialized)
> !     return 0;
> ! 
> !   malloc_initialize_1 ();
> ! #endif
> ! 
> !   return __malloc_initialized;
>   }
>   
>   static int morecore_recursing;
> *************** _malloc_internal (size)
> *** 708,713 ****
> --- 746,754 ----
>       return NULL;
>   #endif
>   
> + #ifdef USE_PTHREAD
> +   pthread_mutex_lock (&_malloc_mutex);
> + #endif
>     PROTECT_MALLOC_STATE (0);
>   
>     if (size < sizeof (struct list))
> *************** _malloc_internal (size)
> *** 765,771 ****
>         if (result == NULL)
>           {
>             PROTECT_MALLOC_STATE (1);
> !           return NULL;
>           }
>   
>         /* Link all fragments but the first into the free list.  */
> --- 806,812 ----
>         if (result == NULL)
>           {
>             PROTECT_MALLOC_STATE (1);
> !           goto out;
>           }
>   
>         /* Link all fragments but the first into the free list.  */
> *************** _malloc_internal (size)
> *** 831,837 ****
>               }
>             result = morecore (wantblocks * BLOCKSIZE);
>             if (result == NULL)
> !             return NULL;
>             block = BLOCK (result);
>             /* Put the new block at the end of the free list.  */
>             _heapinfo[block].free.size = wantblocks;
> --- 872,878 ----
>               }
>             result = morecore (wantblocks * BLOCKSIZE);
>             if (result == NULL)
> !             goto out;
>             block = BLOCK (result);
>             /* Put the new block at the end of the free list.  */
>             _heapinfo[block].free.size = wantblocks;
> *************** _malloc_internal (size)
> *** 886,891 ****
> --- 927,936 ----
>       }
>   
>     PROTECT_MALLOC_STATE (1);
> +  out:
> + #ifdef USE_PTHREAD
> +   pthread_mutex_unlock (&_malloc_mutex);
> + #endif
>     return result;
>   }
>   
> *************** _free_internal (ptr)
> *** 996,1001 ****
> --- 1041,1049 ----
>     if (ptr == NULL)
>       return;
>   
> + #ifdef USE_PTHREAD
> +   pthread_mutex_lock (&_malloc_mutex);
> + #endif
>     PROTECT_MALLOC_STATE (0);
>   
>     for (l = _aligned_blocks; l != NULL; l = l->next)
> *************** _free_internal (ptr)
> *** 1221,1226 ****
> --- 1269,1277 ----
>       }
>   
>     PROTECT_MALLOC_STATE (1);
> + #ifdef USE_PTHREAD
> +   pthread_mutex_unlock (&_malloc_mutex);
> + #endif
>   }
>   
>   /* Return memory to the heap.  */
> *************** _realloc_internal (ptr, size)
> *** 1384,1389 ****
> --- 1435,1443 ----
>   
>     block = BLOCK (ptr);
>   
> + #ifdef USE_PTHREAD
> +   pthread_mutex_lock (&_malloc_mutex);
> + #endif
>     PROTECT_MALLOC_STATE (0);
>   
>     type = _heapinfo[block].busy.type;
> *************** _realloc_internal (ptr, size)
> *** 1398,1404 ****
>           {
>             memcpy (result, ptr, size);
>             _free_internal (ptr);
> !           return result;
>           }
>       }
>   
> --- 1452,1458 ----
>           {
>             memcpy (result, ptr, size);
>             _free_internal (ptr);
> !           goto out;
>           }
>       }
>   
> *************** _realloc_internal (ptr, size)
> *** 1451,1457 ****
>                 (void) _malloc_internal (blocks * BLOCKSIZE);
>                 _free_internal (previous);
>               }
> !           return NULL;
>           }
>         if (ptr != result)
>           memmove (result, ptr, blocks * BLOCKSIZE);
> --- 1505,1511 ----
>                 (void) _malloc_internal (blocks * BLOCKSIZE);
>                 _free_internal (previous);
>               }
> !           goto out;
>           }
>         if (ptr != result)
>           memmove (result, ptr, blocks * BLOCKSIZE);
> *************** _realloc_internal (ptr, size)
> *** 1471,1477 ****
>            and copy the lesser of the new size and the old. */
>         result = _malloc_internal (size);
>         if (result == NULL)
> !         return NULL;
>         memcpy (result, ptr, min (size, (__malloc_size_t) 1 << type));
>         _free_internal (ptr);
>       }
> --- 1525,1531 ----
>            and copy the lesser of the new size and the old. */
>         result = _malloc_internal (size);
>         if (result == NULL)
> !         goto out;
>         memcpy (result, ptr, min (size, (__malloc_size_t) 1 << type));
>         _free_internal (ptr);
>       }
> *************** _realloc_internal (ptr, size)
> *** 1479,1484 ****
> --- 1533,1542 ----
>       }
>   
>     PROTECT_MALLOC_STATE (1);
> +  out:
> + #ifdef USE_PTHREAD
> +   pthread_mutex_unlock (&_malloc_mutex);
> + #endif
>     return result;
>   }
>   
> 
> 
> _______________________________________________
> Emacs-devel mailing list
> address@hidden
> http://lists.gnu.org/mailman/listinfo/emacs-devel