all messages for Emacs-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
* [mituharu@math.s.chiba-u.ac.jp: Re: emacs-22.1 with GTK dumps core when Gnome wigets clicked]
@ 2007-06-17 21:49 Richard Stallman
  2007-06-18  1:11 ` YAMAMOTO Mitsuharu
  0 siblings, 1 reply; 23+ messages in thread
From: Richard Stallman @ 2007-06-17 21:49 UTC (permalink / raw)
  To: emacs-devel

Would someone else (other that Mitsuharu) please study this
patch to check whether it is really correct?  Things like this
can be tricky.

------- Start of forwarded message -------
X-Spam-Status: No, score=0.5 required=5.0 tests=DNS_FROM_RFC_ABUSE,
	UNPARSEABLE_RELAY autolearn=no version=3.1.0
Date: Thu, 14 Jun 2007 11:16:30 +0900
From: YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp>
To: Gary Mills <mills@cc.umanitoba.ca>
In-Reply-To: <20070610141003.GA12085@cc.umanitoba.ca>
Organization: Faculty of Science, Chiba University
MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka")
Content-Type: text/plain; charset=US-ASCII
Cc: bug-gnu-emacs@gnu.org
Subject: Re: emacs-22.1 with GTK dumps core when Gnome wigets clicked

I found a possible problem, which may or may not be relevant to this
issue, in the initialization of the recursive mutex in src/gmalloc.c.
Could you try this patch?

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

Index: src/gmalloc.c
===================================================================
RCS file: /cvsroot/emacs/emacs/src/gmalloc.c,v
retrieving revision 1.21
diff -c -p -r1.21 gmalloc.c
*** src/gmalloc.c	28 Mar 2007 08:16:05 -0000	1.21
- --- src/gmalloc.c	14 Jun 2007 02:03:33 -0000
*************** extern __malloc_size_t _bytes_free;
*** 235,240 ****
- --- 235,243 ----
  extern __ptr_t _malloc_internal PP ((__malloc_size_t __size));
  extern __ptr_t _realloc_internal PP ((__ptr_t __ptr, __malloc_size_t __size));
  extern void _free_internal PP ((__ptr_t __ptr));
+ extern __ptr_t _malloc_internal_nolock PP ((__malloc_size_t __size));
+ extern __ptr_t _realloc_internal_nolock PP ((__ptr_t __ptr, __malloc_size_t __size));
+ extern void _free_internal_nolock PP ((__ptr_t __ptr));
  
  #ifdef USE_PTHREAD
  extern pthread_mutex_t _malloc_mutex;
*************** register_heapinfo ()
*** 554,560 ****
  
  #ifdef USE_PTHREAD
  static pthread_once_t malloc_init_once_control = PTHREAD_ONCE_INIT;
! pthread_mutex_t _malloc_mutex;
  #endif
  
  static void
- --- 557,563 ----
  
  #ifdef USE_PTHREAD
  static pthread_once_t malloc_init_once_control = PTHREAD_ONCE_INIT;
! pthread_mutex_t _malloc_mutex = PTHREAD_MUTEX_INITIALIZER;
  #endif
  
  static void
*************** malloc_initialize_1 ()
*** 567,573 ****
    if (__malloc_initialize_hook)
      (*__malloc_initialize_hook) ();
  
! #ifdef USE_PTHREAD
    {
      pthread_mutexattr_t attr;
  
- --- 570,578 ----
    if (__malloc_initialize_hook)
      (*__malloc_initialize_hook) ();
  
!   /* We don't use recursive mutex because pthread_mutexattr_init may
!      call malloc internally.  */
! #if 0 /* defined (USE_PTHREAD) */
    {
      pthread_mutexattr_t attr;
  
*************** morecore (size)
*** 661,667 ****
  	     `morecore_recursing' flag and return null.  */
  	  int save = errno;	/* Don't want to clobber errno with ENOMEM.  */
  	  morecore_recursing = 1;
! 	  newinfo = (malloc_info *) _realloc_internal
  	    (_heapinfo, newsize * sizeof (malloc_info));
  	  morecore_recursing = 0;
  	  if (newinfo == NULL)
- --- 666,672 ----
  	     `morecore_recursing' flag and return null.  */
  	  int save = errno;	/* Don't want to clobber errno with ENOMEM.  */
  	  morecore_recursing = 1;
! 	  newinfo = (malloc_info *) _realloc_internal_nolock
  	    (_heapinfo, newsize * sizeof (malloc_info));
  	  morecore_recursing = 0;
  	  if (newinfo == NULL)
*************** morecore (size)
*** 717,723 ****
        /* Reset _heaplimit so _free_internal never decides
  	 it can relocate or resize the info table.  */
        _heaplimit = 0;
!       _free_internal (oldinfo);
        PROTECT_MALLOC_STATE (0);
  
        /* The new heap limit includes the new table just allocated.  */
- --- 722,728 ----
        /* Reset _heaplimit so _free_internal never decides
  	 it can relocate or resize the info table.  */
        _heaplimit = 0;
!       _free_internal_nolock (oldinfo);
        PROTECT_MALLOC_STATE (0);
  
        /* The new heap limit includes the new table just allocated.  */
*************** morecore (size)
*** 732,738 ****
  
  /* Allocate memory from the heap.  */
  __ptr_t
! _malloc_internal (size)
       __malloc_size_t size;
  {
    __ptr_t result;
- --- 737,743 ----
  
  /* Allocate memory from the heap.  */
  __ptr_t
! _malloc_internal_nolock (size)
       __malloc_size_t size;
  {
    __ptr_t result;
*************** _malloc_internal (size)
*** 752,758 ****
      return NULL;
  #endif
  
- -   LOCK ();
    PROTECT_MALLOC_STATE (0);
  
    if (size < sizeof (struct list))
- --- 757,762 ----
*************** _malloc_internal (size)
*** 802,811 ****
  	  /* No free fragments of the desired size, so get a new block
  	     and break it into fragments, returning the first.  */
  #ifdef GC_MALLOC_CHECK
! 	  result = _malloc_internal (BLOCKSIZE);
  	  PROTECT_MALLOC_STATE (0);
  #else
! 	  result = malloc (BLOCKSIZE);
  #endif
  	  if (result == NULL)
  	    {
- --- 806,815 ----
  	  /* No free fragments of the desired size, so get a new block
  	     and break it into fragments, returning the first.  */
  #ifdef GC_MALLOC_CHECK
! 	  result = _malloc_internal_nolock (BLOCKSIZE);
  	  PROTECT_MALLOC_STATE (0);
  #else
! 	  result = _malloc_internal_nolock (BLOCKSIZE);
  #endif
  	  if (result == NULL)
  	    {
*************** _malloc_internal (size)
*** 932,938 ****
- --- 936,954 ----
  
    PROTECT_MALLOC_STATE (1);
   out:
+   return result;
+ }
+ 
+ __ptr_t
+ _malloc_internal (size)
+      __malloc_size_t size;
+ {
+   __ptr_t result;
+ 
+   LOCK ();
+   result = _malloc_internal_nolock (size);
    UNLOCK ();
+ 
    return result;
  }
  
*************** void (*__free_hook) PP ((__ptr_t __ptr))
*** 1024,1032 ****
  struct alignlist *_aligned_blocks = NULL;
  
  /* Return memory to the heap.
!    Like `free' but don't call a __free_hook if there is one.  */
  void
! _free_internal (ptr)
       __ptr_t ptr;
  {
    int type;
- --- 1040,1048 ----
  struct alignlist *_aligned_blocks = NULL;
  
  /* Return memory to the heap.
!    Like `_free_internal' but don't lock mutex.  */
  void
! _free_internal_nolock (ptr)
       __ptr_t ptr;
  {
    int type;
*************** _free_internal (ptr)
*** 1043,1049 ****
    if (ptr == NULL)
      return;
  
- -   LOCK ();
    PROTECT_MALLOC_STATE (0);
  
    for (l = _aligned_blocks; l != NULL; l = l->next)
- --- 1059,1064 ----
*************** _free_internal (ptr)
*** 1158,1164 ****
  		 table's blocks to the system before we have copied them to
  		 the new location.  */
  	      _heaplimit = 0;
! 	      _free_internal (_heapinfo);
  	      _heaplimit = oldlimit;
  
  	      /* Tell malloc to search from the beginning of the heap for
- --- 1173,1179 ----
  		 table's blocks to the system before we have copied them to
  		 the new location.  */
  	      _heaplimit = 0;
! 	      _free_internal_nolock (_heapinfo);
  	      _heaplimit = oldlimit;
  
  	      /* Tell malloc to search from the beginning of the heap for
*************** _free_internal (ptr)
*** 1166,1173 ****
  	      _heapindex = 0;
  
  	      /* Allocate new space for the info table and move its data.  */
! 	      newinfo = (malloc_info *) _malloc_internal (info_blocks
! 							  * BLOCKSIZE);
  	      PROTECT_MALLOC_STATE (0);
  	      memmove (newinfo, _heapinfo, info_blocks * BLOCKSIZE);
  	      _heapinfo = newinfo;
- --- 1181,1188 ----
  	      _heapindex = 0;
  
  	      /* Allocate new space for the info table and move its data.  */
! 	      newinfo = (malloc_info *) _malloc_internal_nolock (info_blocks
! 								 * BLOCKSIZE);
  	      PROTECT_MALLOC_STATE (0);
  	      memmove (newinfo, _heapinfo, info_blocks * BLOCKSIZE);
  	      _heapinfo = newinfo;
*************** _free_internal (ptr)
*** 1231,1239 ****
  	  _bytes_free -= BLOCKSIZE;
  
  #ifdef GC_MALLOC_CHECK
! 	  _free_internal (ADDRESS (block));
  #else
! 	  free (ADDRESS (block));
  #endif
  	}
        else if (_heapinfo[block].busy.info.frag.nfree != 0)
- --- 1246,1254 ----
  	  _bytes_free -= BLOCKSIZE;
  
  #ifdef GC_MALLOC_CHECK
! 	  _free_internal_nolock (ADDRESS (block));
  #else
! 	  _free_internal_nolock (ADDRESS (block));
  #endif
  	}
        else if (_heapinfo[block].busy.info.frag.nfree != 0)
*************** _free_internal (ptr)
*** 1269,1274 ****
- --- 1284,1299 ----
      }
  
    PROTECT_MALLOC_STATE (1);
+ }
+ 
+ /* Return memory to the heap.
+    Like `free' but don't call a __free_hook if there is one.  */
+ void
+ _free_internal (ptr)
+      __ptr_t ptr;
+ {
+   LOCK ();
+   _free_internal_nolock (ptr);
    UNLOCK ();
  }
  
*************** __ptr_t (*__realloc_hook) PP ((__ptr_t _
*** 1415,1421 ****
     new region.  This module has incestuous knowledge of the
     internals of both free and malloc. */
  __ptr_t
! _realloc_internal (ptr, size)
       __ptr_t ptr;
       __malloc_size_t size;
  {
- --- 1440,1446 ----
     new region.  This module has incestuous knowledge of the
     internals of both free and malloc. */
  __ptr_t
! _realloc_internal_nolock (ptr, size)
       __ptr_t ptr;
       __malloc_size_t size;
  {
*************** _realloc_internal (ptr, size)
*** 1425,1439 ****
  
    if (size == 0)
      {
!       _free_internal (ptr);
!       return _malloc_internal (0);
      }
    else if (ptr == NULL)
!     return _malloc_internal (size);
  
    block = BLOCK (ptr);
  
- -   LOCK ();
    PROTECT_MALLOC_STATE (0);
  
    type = _heapinfo[block].busy.type;
- --- 1450,1463 ----
  
    if (size == 0)
      {
!       _free_internal_nolock (ptr);
!       return _malloc_internal_nolock (0);
      }
    else if (ptr == NULL)
!     return _malloc_internal_nolock (size);
  
    block = BLOCK (ptr);
  
    PROTECT_MALLOC_STATE (0);
  
    type = _heapinfo[block].busy.type;
*************** _realloc_internal (ptr, size)
*** 1443,1453 ****
        /* Maybe reallocate a large block to a small fragment.  */
        if (size <= BLOCKSIZE / 2)
  	{
! 	  result = _malloc_internal (size);
  	  if (result != NULL)
  	    {
  	      memcpy (result, ptr, size);
! 	      _free_internal (ptr);
  	      goto out;
  	    }
  	}
- --- 1467,1477 ----
        /* Maybe reallocate a large block to a small fragment.  */
        if (size <= BLOCKSIZE / 2)
  	{
! 	  result = _malloc_internal_nolock (size);
  	  if (result != NULL)
  	    {
  	      memcpy (result, ptr, size);
! 	      _free_internal_nolock (ptr);
  	      goto out;
  	    }
  	}
*************** _realloc_internal (ptr, size)
*** 1467,1473 ****
  	     Now we will free this chunk; increment the statistics counter
  	     so it doesn't become wrong when _free_internal decrements it.  */
  	  ++_chunks_used;
! 	  _free_internal (ADDRESS (block + blocks));
  	  result = ptr;
  	}
        else if (blocks == _heapinfo[block].busy.info.size)
- --- 1491,1497 ----
  	     Now we will free this chunk; increment the statistics counter
  	     so it doesn't become wrong when _free_internal decrements it.  */
  	  ++_chunks_used;
! 	  _free_internal_nolock (ADDRESS (block + blocks));
  	  result = ptr;
  	}
        else if (blocks == _heapinfo[block].busy.info.size)
*************** _realloc_internal (ptr, size)
*** 1482,1489 ****
  	  /* Prevent free from actually returning memory to the system.  */
  	  oldlimit = _heaplimit;
  	  _heaplimit = 0;
! 	  _free_internal (ptr);
! 	  result = _malloc_internal (size);
  	  PROTECT_MALLOC_STATE (0);
  	  if (_heaplimit == 0)
  	    _heaplimit = oldlimit;
- --- 1506,1513 ----
  	  /* Prevent free from actually returning memory to the system.  */
  	  oldlimit = _heaplimit;
  	  _heaplimit = 0;
! 	  _free_internal_nolock (ptr);
! 	  result = _malloc_internal_nolock (size);
  	  PROTECT_MALLOC_STATE (0);
  	  if (_heaplimit == 0)
  	    _heaplimit = oldlimit;
*************** _realloc_internal (ptr, size)
*** 1493,1505 ****
  		 the thing we just freed.  Unfortunately it might
  		 have been coalesced with its neighbors.  */
  	      if (_heapindex == block)
! 	        (void) _malloc_internal (blocks * BLOCKSIZE);
  	      else
  		{
  		  __ptr_t previous
! 		    = _malloc_internal ((block - _heapindex) * BLOCKSIZE);
! 		  (void) _malloc_internal (blocks * BLOCKSIZE);
! 		  _free_internal (previous);
  		}
  	      goto out;
  	    }
- --- 1517,1529 ----
  		 the thing we just freed.  Unfortunately it might
  		 have been coalesced with its neighbors.  */
  	      if (_heapindex == block)
! 	        (void) _malloc_internal_nolock (blocks * BLOCKSIZE);
  	      else
  		{
  		  __ptr_t previous
! 		    = _malloc_internal_nolock ((block - _heapindex) * BLOCKSIZE);
! 		  (void) _malloc_internal_nolock (blocks * BLOCKSIZE);
! 		  _free_internal_nolock (previous);
  		}
  	      goto out;
  	    }
*************** _realloc_internal (ptr, size)
*** 1519,1536 ****
  	{
  	  /* The new size is different; allocate a new space,
  	     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);
  	}
        break;
      }
  
    PROTECT_MALLOC_STATE (1);
   out:
    UNLOCK ();
    return result;
  }
  
- --- 1543,1573 ----
  	{
  	  /* The new size is different; allocate a new space,
  	     and copy the lesser of the new size and the old. */
! 	  result = _malloc_internal_nolock (size);
  	  if (result == NULL)
  	    goto out;
  	  memcpy (result, ptr, min (size, (__malloc_size_t) 1 << type));
! 	  _free_internal_nolock (ptr);
  	}
        break;
      }
  
    PROTECT_MALLOC_STATE (1);
   out:
+   return result;
+ }
+ 
+ __ptr_t
+ _realloc_internal (ptr, size)
+      __ptr_t ptr;
+      __malloc_size_t size;
+ {
+   __ptr_t result;
+ 
+   LOCK();
+   result = _realloc_internal_nolock (ptr, size);
    UNLOCK ();
+ 
    return result;
  }
  


_______________________________________________
bug-gnu-emacs mailing list
bug-gnu-emacs@gnu.org
http://lists.gnu.org/mailman/listinfo/bug-gnu-emacs
------- End of forwarded message -------

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [mituharu@math.s.chiba-u.ac.jp: Re: emacs-22.1 with GTK dumps core when Gnome wigets clicked]
  2007-06-17 21:49 [mituharu@math.s.chiba-u.ac.jp: Re: emacs-22.1 with GTK dumps core when Gnome wigets clicked] Richard Stallman
@ 2007-06-18  1:11 ` YAMAMOTO Mitsuharu
  2007-06-21 10:13   ` Jan Djärv
  2007-06-24 23:47   ` Richard Stallman
  0 siblings, 2 replies; 23+ messages in thread
From: YAMAMOTO Mitsuharu @ 2007-06-18  1:11 UTC (permalink / raw)
  To: rms; +Cc: emacs-devel

>>>>> On Sun, 17 Jun 2007 17:49:28 -0400, Richard Stallman <rms@gnu.org> said:

> Would someone else (other that Mitsuharu) please study this patch to
> check whether it is really correct?  Things like this can be tricky.

Please take a look at the following one instead.  It tries to fix the
following problems of gmalloc.c with HAVE_GTK_AND_PTHREAD in Emacs 22.1.

* HAVE_GTK_AND_PTHREAD was checked before including config.h.
* malloc_initialize_1 called pthread_mutexattr_init that may call malloc.
* _aligned_blocks was not protected.
* __malloc_hook etc. may be modified between its NULL-check and the use.

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

Index: src/gmalloc.c
===================================================================
RCS file: /cvsroot/emacs/emacs/src/gmalloc.c,v
retrieving revision 1.21
diff -c -p -r1.21 gmalloc.c
*** src/gmalloc.c	28 Mar 2007 08:16:05 -0000	1.21
--- src/gmalloc.c	18 Jun 2007 01:00:00 -0000
***************
*** 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.  */
  
--- 1,6 ----
*************** Fifth Floor, Boston, MA 02110-1301, USA.
*** 40,45 ****
--- 37,46 ----
  #include <config.h>
  #endif
  
+ #ifdef HAVE_GTK_AND_PTHREAD
+ #define USE_PTHREAD
+ #endif
+ 
  #if ((defined __cplusplus || (defined (__STDC__) && __STDC__) \
        || defined STDC_HEADERS || defined PROTOTYPES) \
       && ! defined (BROKEN_PROTOTYPES))
*************** extern __malloc_size_t _bytes_free;
*** 235,248 ****
  extern __ptr_t _malloc_internal PP ((__malloc_size_t __size));
  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;
  #define LOCK()     pthread_mutex_lock (&_malloc_mutex)
  #define UNLOCK()   pthread_mutex_unlock (&_malloc_mutex)
  #else
  #define LOCK()
  #define UNLOCK()
  #endif
  
  #endif /* _MALLOC_INTERNAL.  */
--- 236,256 ----
  extern __ptr_t _malloc_internal PP ((__malloc_size_t __size));
  extern __ptr_t _realloc_internal PP ((__ptr_t __ptr, __malloc_size_t __size));
  extern void _free_internal PP ((__ptr_t __ptr));
+ extern __ptr_t _malloc_internal_nolock PP ((__malloc_size_t __size));
+ extern __ptr_t _realloc_internal_nolock PP ((__ptr_t __ptr, __malloc_size_t __size));
+ extern void _free_internal_nolock PP ((__ptr_t __ptr));
  
  #ifdef USE_PTHREAD
! extern pthread_mutex_t _malloc_mutex, _aligned_blocks_mutex;
  #define LOCK()     pthread_mutex_lock (&_malloc_mutex)
  #define UNLOCK()   pthread_mutex_unlock (&_malloc_mutex)
+ #define LOCK_ALIGNED_BLOCKS()     pthread_mutex_lock (&_aligned_blocks_mutex)
+ #define UNLOCK_ALIGNED_BLOCKS()   pthread_mutex_unlock (&_aligned_blocks_mutex)
  #else
  #define LOCK()
  #define UNLOCK()
+ #define LOCK_ALIGNED_BLOCKS()
+ #define UNLOCK_ALIGNED_BLOCKS()
  #endif
  
  #endif /* _MALLOC_INTERNAL.  */
*************** register_heapinfo ()
*** 554,560 ****
  
  #ifdef USE_PTHREAD
  static pthread_once_t malloc_init_once_control = PTHREAD_ONCE_INIT;
! pthread_mutex_t _malloc_mutex;
  #endif
  
  static void
--- 562,569 ----
  
  #ifdef USE_PTHREAD
  static pthread_once_t malloc_init_once_control = PTHREAD_ONCE_INIT;
! pthread_mutex_t _malloc_mutex = PTHREAD_MUTEX_INITIALIZER;
! pthread_mutex_t _aligned_blocks_mutex = PTHREAD_MUTEX_INITIALIZER;
  #endif
  
  static void
*************** malloc_initialize_1 ()
*** 567,573 ****
    if (__malloc_initialize_hook)
      (*__malloc_initialize_hook) ();
  
! #ifdef USE_PTHREAD
    {
      pthread_mutexattr_t attr;
  
--- 576,584 ----
    if (__malloc_initialize_hook)
      (*__malloc_initialize_hook) ();
  
!   /* We don't use recursive mutex because pthread_mutexattr_init may
!      call malloc internally.  */
! #if 0 /* defined (USE_PTHREAD) */
    {
      pthread_mutexattr_t attr;
  
*************** static int morecore_recursing;
*** 616,624 ****
  
  /* Get neatly aligned memory, initializing or
     growing the heap info table as necessary. */
! static __ptr_t morecore PP ((__malloc_size_t));
  static __ptr_t
! morecore (size)
       __malloc_size_t size;
  {
    __ptr_t result;
--- 627,635 ----
  
  /* Get neatly aligned memory, initializing or
     growing the heap info table as necessary. */
! static __ptr_t morecore_nolock PP ((__malloc_size_t));
  static __ptr_t
! morecore_nolock (size)
       __malloc_size_t size;
  {
    __ptr_t result;
*************** morecore (size)
*** 661,667 ****
  	     `morecore_recursing' flag and return null.  */
  	  int save = errno;	/* Don't want to clobber errno with ENOMEM.  */
  	  morecore_recursing = 1;
! 	  newinfo = (malloc_info *) _realloc_internal
  	    (_heapinfo, newsize * sizeof (malloc_info));
  	  morecore_recursing = 0;
  	  if (newinfo == NULL)
--- 672,678 ----
  	     `morecore_recursing' flag and return null.  */
  	  int save = errno;	/* Don't want to clobber errno with ENOMEM.  */
  	  morecore_recursing = 1;
! 	  newinfo = (malloc_info *) _realloc_internal_nolock
  	    (_heapinfo, newsize * sizeof (malloc_info));
  	  morecore_recursing = 0;
  	  if (newinfo == NULL)
*************** morecore (size)
*** 717,723 ****
        /* Reset _heaplimit so _free_internal never decides
  	 it can relocate or resize the info table.  */
        _heaplimit = 0;
!       _free_internal (oldinfo);
        PROTECT_MALLOC_STATE (0);
  
        /* The new heap limit includes the new table just allocated.  */
--- 728,734 ----
        /* Reset _heaplimit so _free_internal never decides
  	 it can relocate or resize the info table.  */
        _heaplimit = 0;
!       _free_internal_nolock (oldinfo);
        PROTECT_MALLOC_STATE (0);
  
        /* The new heap limit includes the new table just allocated.  */
*************** morecore (size)
*** 732,738 ****
  
  /* Allocate memory from the heap.  */
  __ptr_t
! _malloc_internal (size)
       __malloc_size_t size;
  {
    __ptr_t result;
--- 743,749 ----
  
  /* Allocate memory from the heap.  */
  __ptr_t
! _malloc_internal_nolock (size)
       __malloc_size_t size;
  {
    __ptr_t result;
*************** _malloc_internal (size)
*** 752,758 ****
      return NULL;
  #endif
  
-   LOCK ();
    PROTECT_MALLOC_STATE (0);
  
    if (size < sizeof (struct list))
--- 763,768 ----
*************** _malloc_internal (size)
*** 802,809 ****
  	  /* No free fragments of the desired size, so get a new block
  	     and break it into fragments, returning the first.  */
  #ifdef GC_MALLOC_CHECK
! 	  result = _malloc_internal (BLOCKSIZE);
  	  PROTECT_MALLOC_STATE (0);
  #else
  	  result = malloc (BLOCKSIZE);
  #endif
--- 812,821 ----
  	  /* No free fragments of the desired size, so get a new block
  	     and break it into fragments, returning the first.  */
  #ifdef GC_MALLOC_CHECK
! 	  result = _malloc_internal_nolock (BLOCKSIZE);
  	  PROTECT_MALLOC_STATE (0);
+ #elif defined (USE_PTHREAD)
+ 	  result = _malloc_internal_nolock (BLOCKSIZE);
  #else
  	  result = malloc (BLOCKSIZE);
  #endif
*************** _malloc_internal (size)
*** 874,880 ****
   		  _heaplimit += wantblocks - lastblocks;
  		  continue;
  		}
! 	      result = morecore (wantblocks * BLOCKSIZE);
  	      if (result == NULL)
  		goto out;
  	      block = BLOCK (result);
--- 886,892 ----
   		  _heaplimit += wantblocks - lastblocks;
  		  continue;
  		}
! 	      result = morecore_nolock (wantblocks * BLOCKSIZE);
  	      if (result == NULL)
  		goto out;
  	      block = BLOCK (result);
*************** _malloc_internal (size)
*** 932,938 ****
--- 944,962 ----
  
    PROTECT_MALLOC_STATE (1);
   out:
+   return result;
+ }
+ 
+ __ptr_t
+ _malloc_internal (size)
+      __malloc_size_t size;
+ {
+   __ptr_t result;
+ 
+   LOCK ();
+   result = _malloc_internal_nolock (size);
    UNLOCK ();
+ 
    return result;
  }
  
*************** __ptr_t
*** 940,949 ****
  malloc (size)
       __malloc_size_t size;
  {
    if (!__malloc_initialized && !__malloc_initialize ())
      return NULL;
  
!   return (__malloc_hook != NULL ? *__malloc_hook : _malloc_internal) (size);
  }
  \f
  #ifndef _LIBC
--- 964,979 ----
  malloc (size)
       __malloc_size_t size;
  {
+   __ptr_t (*hook) (__malloc_size_t);
+ 
    if (!__malloc_initialized && !__malloc_initialize ())
      return NULL;
  
!   /* Copy the value of __malloc_hook to an automatic variable in case
!      __malloc_hook is modified in another thread between its
!      NULL-check and the use.  */
!   hook = __malloc_hook;
!   return (hook != NULL ? *hook : _malloc_internal) (size);
  }
  \f
  #ifndef _LIBC
*************** void (*__free_hook) PP ((__ptr_t __ptr))
*** 1024,1032 ****
  struct alignlist *_aligned_blocks = NULL;
  
  /* Return memory to the heap.
!    Like `free' but don't call a __free_hook if there is one.  */
  void
! _free_internal (ptr)
       __ptr_t ptr;
  {
    int type;
--- 1054,1062 ----
  struct alignlist *_aligned_blocks = NULL;
  
  /* Return memory to the heap.
!    Like `_free_internal' but don't lock mutex.  */
  void
! _free_internal_nolock (ptr)
       __ptr_t ptr;
  {
    int type;
*************** _free_internal (ptr)
*** 1043,1051 ****
    if (ptr == NULL)
      return;
  
-   LOCK ();
    PROTECT_MALLOC_STATE (0);
  
    for (l = _aligned_blocks; l != NULL; l = l->next)
      if (l->aligned == ptr)
        {
--- 1073,1081 ----
    if (ptr == NULL)
      return;
  
    PROTECT_MALLOC_STATE (0);
  
+   LOCK_ALIGNED_BLOCKS ();
    for (l = _aligned_blocks; l != NULL; l = l->next)
      if (l->aligned == ptr)
        {
*************** _free_internal (ptr)
*** 1053,1058 ****
--- 1083,1089 ----
  	ptr = l->exact;
  	break;
        }
+   UNLOCK_ALIGNED_BLOCKS ();
  
    block = BLOCK (ptr);
  
*************** _free_internal (ptr)
*** 1158,1164 ****
  		 table's blocks to the system before we have copied them to
  		 the new location.  */
  	      _heaplimit = 0;
! 	      _free_internal (_heapinfo);
  	      _heaplimit = oldlimit;
  
  	      /* Tell malloc to search from the beginning of the heap for
--- 1189,1195 ----
  		 table's blocks to the system before we have copied them to
  		 the new location.  */
  	      _heaplimit = 0;
! 	      _free_internal_nolock (_heapinfo);
  	      _heaplimit = oldlimit;
  
  	      /* Tell malloc to search from the beginning of the heap for
*************** _free_internal (ptr)
*** 1166,1173 ****
  	      _heapindex = 0;
  
  	      /* Allocate new space for the info table and move its data.  */
! 	      newinfo = (malloc_info *) _malloc_internal (info_blocks
! 							  * BLOCKSIZE);
  	      PROTECT_MALLOC_STATE (0);
  	      memmove (newinfo, _heapinfo, info_blocks * BLOCKSIZE);
  	      _heapinfo = newinfo;
--- 1197,1204 ----
  	      _heapindex = 0;
  
  	      /* Allocate new space for the info table and move its data.  */
! 	      newinfo = (malloc_info *) _malloc_internal_nolock (info_blocks
! 								 * BLOCKSIZE);
  	      PROTECT_MALLOC_STATE (0);
  	      memmove (newinfo, _heapinfo, info_blocks * BLOCKSIZE);
  	      _heapinfo = newinfo;
*************** _free_internal (ptr)
*** 1230,1237 ****
  	  _chunks_free -= BLOCKSIZE >> type;
  	  _bytes_free -= BLOCKSIZE;
  
! #ifdef GC_MALLOC_CHECK
! 	  _free_internal (ADDRESS (block));
  #else
  	  free (ADDRESS (block));
  #endif
--- 1261,1268 ----
  	  _chunks_free -= BLOCKSIZE >> type;
  	  _bytes_free -= BLOCKSIZE;
  
! #if defined (GC_MALLOC_CHECK) || defined (USE_PTHREAD)
! 	  _free_internal_nolock (ADDRESS (block));
  #else
  	  free (ADDRESS (block));
  #endif
*************** _free_internal (ptr)
*** 1269,1274 ****
--- 1300,1315 ----
      }
  
    PROTECT_MALLOC_STATE (1);
+ }
+ 
+ /* Return memory to the heap.
+    Like `free' but don't call a __free_hook if there is one.  */
+ void
+ _free_internal (ptr)
+      __ptr_t ptr;
+ {
+   LOCK ();
+   _free_internal_nolock (ptr);
    UNLOCK ();
  }
  
*************** FREE_RETURN_TYPE
*** 1278,1285 ****
  free (ptr)
       __ptr_t ptr;
  {
!   if (__free_hook != NULL)
!     (*__free_hook) (ptr);
    else
      _free_internal (ptr);
  }
--- 1319,1328 ----
  free (ptr)
       __ptr_t ptr;
  {
!   void (*hook) (__ptr_t) = __free_hook;
! 
!   if (hook != NULL)
!     (*hook) (ptr);
    else
      _free_internal (ptr);
  }
*************** __ptr_t (*__realloc_hook) PP ((__ptr_t _
*** 1415,1421 ****
     new region.  This module has incestuous knowledge of the
     internals of both free and malloc. */
  __ptr_t
! _realloc_internal (ptr, size)
       __ptr_t ptr;
       __malloc_size_t size;
  {
--- 1458,1464 ----
     new region.  This module has incestuous knowledge of the
     internals of both free and malloc. */
  __ptr_t
! _realloc_internal_nolock (ptr, size)
       __ptr_t ptr;
       __malloc_size_t size;
  {
*************** _realloc_internal (ptr, size)
*** 1425,1439 ****
  
    if (size == 0)
      {
!       _free_internal (ptr);
!       return _malloc_internal (0);
      }
    else if (ptr == NULL)
!     return _malloc_internal (size);
  
    block = BLOCK (ptr);
  
-   LOCK ();
    PROTECT_MALLOC_STATE (0);
  
    type = _heapinfo[block].busy.type;
--- 1468,1481 ----
  
    if (size == 0)
      {
!       _free_internal_nolock (ptr);
!       return _malloc_internal_nolock (0);
      }
    else if (ptr == NULL)
!     return _malloc_internal_nolock (size);
  
    block = BLOCK (ptr);
  
    PROTECT_MALLOC_STATE (0);
  
    type = _heapinfo[block].busy.type;
*************** _realloc_internal (ptr, size)
*** 1443,1453 ****
        /* Maybe reallocate a large block to a small fragment.  */
        if (size <= BLOCKSIZE / 2)
  	{
! 	  result = _malloc_internal (size);
  	  if (result != NULL)
  	    {
  	      memcpy (result, ptr, size);
! 	      _free_internal (ptr);
  	      goto out;
  	    }
  	}
--- 1485,1495 ----
        /* Maybe reallocate a large block to a small fragment.  */
        if (size <= BLOCKSIZE / 2)
  	{
! 	  result = _malloc_internal_nolock (size);
  	  if (result != NULL)
  	    {
  	      memcpy (result, ptr, size);
! 	      _free_internal_nolock (ptr);
  	      goto out;
  	    }
  	}
*************** _realloc_internal (ptr, size)
*** 1467,1473 ****
  	     Now we will free this chunk; increment the statistics counter
  	     so it doesn't become wrong when _free_internal decrements it.  */
  	  ++_chunks_used;
! 	  _free_internal (ADDRESS (block + blocks));
  	  result = ptr;
  	}
        else if (blocks == _heapinfo[block].busy.info.size)
--- 1509,1515 ----
  	     Now we will free this chunk; increment the statistics counter
  	     so it doesn't become wrong when _free_internal decrements it.  */
  	  ++_chunks_used;
! 	  _free_internal_nolock (ADDRESS (block + blocks));
  	  result = ptr;
  	}
        else if (blocks == _heapinfo[block].busy.info.size)
*************** _realloc_internal (ptr, size)
*** 1482,1489 ****
  	  /* Prevent free from actually returning memory to the system.  */
  	  oldlimit = _heaplimit;
  	  _heaplimit = 0;
! 	  _free_internal (ptr);
! 	  result = _malloc_internal (size);
  	  PROTECT_MALLOC_STATE (0);
  	  if (_heaplimit == 0)
  	    _heaplimit = oldlimit;
--- 1524,1531 ----
  	  /* Prevent free from actually returning memory to the system.  */
  	  oldlimit = _heaplimit;
  	  _heaplimit = 0;
! 	  _free_internal_nolock (ptr);
! 	  result = _malloc_internal_nolock (size);
  	  PROTECT_MALLOC_STATE (0);
  	  if (_heaplimit == 0)
  	    _heaplimit = oldlimit;
*************** _realloc_internal (ptr, size)
*** 1493,1505 ****
  		 the thing we just freed.  Unfortunately it might
  		 have been coalesced with its neighbors.  */
  	      if (_heapindex == block)
! 	        (void) _malloc_internal (blocks * BLOCKSIZE);
  	      else
  		{
  		  __ptr_t previous
! 		    = _malloc_internal ((block - _heapindex) * BLOCKSIZE);
! 		  (void) _malloc_internal (blocks * BLOCKSIZE);
! 		  _free_internal (previous);
  		}
  	      goto out;
  	    }
--- 1535,1547 ----
  		 the thing we just freed.  Unfortunately it might
  		 have been coalesced with its neighbors.  */
  	      if (_heapindex == block)
! 	        (void) _malloc_internal_nolock (blocks * BLOCKSIZE);
  	      else
  		{
  		  __ptr_t previous
! 		    = _malloc_internal_nolock ((block - _heapindex) * BLOCKSIZE);
! 		  (void) _malloc_internal_nolock (blocks * BLOCKSIZE);
! 		  _free_internal_nolock (previous);
  		}
  	      goto out;
  	    }
*************** _realloc_internal (ptr, size)
*** 1519,1536 ****
  	{
  	  /* The new size is different; allocate a new space,
  	     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);
  	}
        break;
      }
  
    PROTECT_MALLOC_STATE (1);
   out:
    UNLOCK ();
    return result;
  }
  
--- 1561,1591 ----
  	{
  	  /* The new size is different; allocate a new space,
  	     and copy the lesser of the new size and the old. */
! 	  result = _malloc_internal_nolock (size);
  	  if (result == NULL)
  	    goto out;
  	  memcpy (result, ptr, min (size, (__malloc_size_t) 1 << type));
! 	  _free_internal_nolock (ptr);
  	}
        break;
      }
  
    PROTECT_MALLOC_STATE (1);
   out:
+   return result;
+ }
+ 
+ __ptr_t
+ _realloc_internal (ptr, size)
+      __ptr_t ptr;
+      __malloc_size_t size;
+ {
+   __ptr_t result;
+ 
+   LOCK();
+   result = _realloc_internal_nolock (ptr, size);
    UNLOCK ();
+ 
    return result;
  }
  
*************** realloc (ptr, size)
*** 1539,1549 ****
       __ptr_t ptr;
       __malloc_size_t size;
  {
    if (!__malloc_initialized && !__malloc_initialize ())
      return NULL;
  
!   return (__realloc_hook != NULL ? *__realloc_hook : _realloc_internal)
!     (ptr, size);
  }
  /* Copyright (C) 1991, 1992, 1994 Free Software Foundation, Inc.
  
--- 1594,1606 ----
       __ptr_t ptr;
       __malloc_size_t size;
  {
+   __ptr_t (*hook) (__ptr_t, __malloc_size_t);
+ 
    if (!__malloc_initialized && !__malloc_initialize ())
      return NULL;
  
!   hook = __realloc_hook;
!   return (hook != NULL ? *hook : _realloc_internal) (ptr, size);
  }
  /* Copyright (C) 1991, 1992, 1994 Free Software Foundation, Inc.
  
*************** memalign (alignment, size)
*** 1681,1689 ****
  {
    __ptr_t result;
    unsigned long int adj, lastadj;
  
!   if (__memalign_hook)
!     return (*__memalign_hook) (alignment, size);
  
    /* Allocate a block with enough extra space to pad the block with up to
       (ALIGNMENT - 1) bytes if necessary.  */
--- 1738,1747 ----
  {
    __ptr_t result;
    unsigned long int adj, lastadj;
+   __ptr_t (*hook) (__malloc_size_t, __malloc_size_t) = __memalign_hook;
  
!   if (hook)
!     return (*hook) (alignment, size);
  
    /* Allocate a block with enough extra space to pad the block with up to
       (ALIGNMENT - 1) bytes if necessary.  */
*************** memalign (alignment, size)
*** 1718,1723 ****
--- 1776,1782 ----
  	 of an allocated block.  */
  
        struct alignlist *l;
+       LOCK_ALIGNED_BLOCKS ();
        for (l = _aligned_blocks; l != NULL; l = l->next)
  	if (l->aligned == NULL)
  	  /* This slot is free.  Use it.  */
*************** memalign (alignment, size)
*** 1725,1740 ****
        if (l == NULL)
  	{
  	  l = (struct alignlist *) malloc (sizeof (struct alignlist));
! 	  if (l == NULL)
  	    {
! 	      free (result);
! 	      return NULL;
  	    }
- 	  l->next = _aligned_blocks;
- 	  _aligned_blocks = l;
  	}
!       l->exact = result;
!       result = l->aligned = (char *) result + alignment - adj;
      }
  
    return result;
--- 1784,1806 ----
        if (l == NULL)
  	{
  	  l = (struct alignlist *) malloc (sizeof (struct alignlist));
! 	  if (l != NULL)
  	    {
! 	      l->next = _aligned_blocks;
! 	      _aligned_blocks = l;
  	    }
  	}
!       if (l != NULL)
! 	{
! 	  l->exact = result;
! 	  result = l->aligned = (char *) result + alignment - adj;
! 	}
!       UNLOCK_ALIGNED_BLOCKS ();
!       if (l == NULL)
! 	{
! 	  free (result);
! 	  result = NULL;
! 	}
      }
  
    return result;

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [mituharu@math.s.chiba-u.ac.jp: Re: emacs-22.1 with GTK dumps core when Gnome wigets clicked]
  2007-06-18  1:11 ` YAMAMOTO Mitsuharu
@ 2007-06-21 10:13   ` Jan Djärv
  2007-06-21 11:36     ` YAMAMOTO Mitsuharu
  2007-06-25  5:33     ` Ken Raeburn
  2007-06-24 23:47   ` Richard Stallman
  1 sibling, 2 replies; 23+ messages in thread
From: Jan Djärv @ 2007-06-21 10:13 UTC (permalink / raw)
  To: YAMAMOTO Mitsuharu; +Cc: rms, emacs-devel



YAMAMOTO Mitsuharu skrev:
>>>>>> On Sun, 17 Jun 2007 17:49:28 -0400, Richard Stallman <rms@gnu.org> said:
> 
>> Would someone else (other that Mitsuharu) please study this patch to
>> check whether it is really correct?  Things like this can be tricky.
> 
> Please take a look at the following one instead.  It tries to fix the
> following problems of gmalloc.c with HAVE_GTK_AND_PTHREAD in Emacs 22.1.
> 
> * HAVE_GTK_AND_PTHREAD was checked before including config.h.
> * malloc_initialize_1 called pthread_mutexattr_init that may call malloc.
> * _aligned_blocks was not protected.
> * __malloc_hook etc. may be modified between its NULL-check and the use.

The patch was corrupted by some mailer so I could not apply it, but:

!   /* Copy the value of __malloc_hook to an automatic variable in case
!      __malloc_hook is modified in another thread between its
!      NULL-check and the use.  */
!   hook = __malloc_hook;
!   return (hook != NULL ? *hook : _malloc_internal) (size);
   }

Assignment is not guaranteed to be atomic.  It probably is on 32-bit systems, 
but you should not assume it.

	Jan D.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [mituharu@math.s.chiba-u.ac.jp: Re: emacs-22.1 with GTK dumps core when Gnome wigets clicked]
  2007-06-21 10:13   ` Jan Djärv
@ 2007-06-21 11:36     ` YAMAMOTO Mitsuharu
  2007-06-25  5:33     ` Ken Raeburn
  1 sibling, 0 replies; 23+ messages in thread
From: YAMAMOTO Mitsuharu @ 2007-06-21 11:36 UTC (permalink / raw)
  To: Jan Djärv; +Cc: rms, emacs-devel

>>>>> On Thu, 21 Jun 2007 12:13:29 +0200, Jan Djärv <jan.h.d@swipnet.se> said:

> The patch was corrupted by some mailer so I could not apply it, but:

> !   /* Copy the value of __malloc_hook to an automatic variable in case
> !      __malloc_hook is modified in another thread between its
> !      NULL-check and the use.  */
> !   hook = __malloc_hook;
> !   return (hook != NULL ? *hook : _malloc_internal) (size);
>    }

> Assignment is not guaranteed to be atomic.  It probably is on 32-bit systems, 
> but you should not assume it.

Maybe.  But malloc in glibc 2.5 also does the same thing and I suspect
that we cannot do better as long as we try to keep the same interface
with respect to __malloc_hook etc.

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [mituharu@math.s.chiba-u.ac.jp: Re: emacs-22.1 with GTK dumps core when Gnome wigets clicked]
  2007-06-18  1:11 ` YAMAMOTO Mitsuharu
  2007-06-21 10:13   ` Jan Djärv
@ 2007-06-24 23:47   ` Richard Stallman
  1 sibling, 0 replies; 23+ messages in thread
From: Richard Stallman @ 2007-06-24 23:47 UTC (permalink / raw)
  To: YAMAMOTO Mitsuharu; +Cc: emacs-devel

Since nobody has found a real problem with your patch,
would you please install it in Emacs 22 and in the trunk?

Please ack when it is installed.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [mituharu@math.s.chiba-u.ac.jp: Re: emacs-22.1 with GTK dumps core when Gnome wigets clicked]
  2007-06-21 10:13   ` Jan Djärv
  2007-06-21 11:36     ` YAMAMOTO Mitsuharu
@ 2007-06-25  5:33     ` Ken Raeburn
  2007-06-25  8:30       ` YAMAMOTO Mitsuharu
  1 sibling, 1 reply; 23+ messages in thread
From: Ken Raeburn @ 2007-06-25  5:33 UTC (permalink / raw)
  To: emacs-devel

On Jun 21, 2007, at 06:13, Jan Djärv wrote:
> YAMAMOTO Mitsuharu skrev:
>>>>>>> On Sun, 17 Jun 2007 17:49:28 -0400, Richard Stallman  
>>>>>>> <rms@gnu.org> said:
>>> Would someone else (other that Mitsuharu) please study this patch to
>>> check whether it is really correct?  Things like this can be tricky.
>> Please take a look at the following one instead.  It tries to fix the
>> following problems of gmalloc.c with HAVE_GTK_AND_PTHREAD in Emacs  
>> 22.1.
>> * HAVE_GTK_AND_PTHREAD was checked before including config.h.
>> * malloc_initialize_1 called pthread_mutexattr_init that may call  
>> malloc.
>> * _aligned_blocks was not protected.
>> * __malloc_hook etc. may be modified between its NULL-check and  
>> the use.
>
> The patch was corrupted by some mailer so I could not apply it, but:
>
> !   /* Copy the value of __malloc_hook to an automatic variable in  
> case
> !      __malloc_hook is modified in another thread between its
> !      NULL-check and the use.  */
> !   hook = __malloc_hook;
> !   return (hook != NULL ? *hook : _malloc_internal) (size);
>   }
>
> Assignment is not guaranteed to be atomic.  It probably is on 32- 
> bit systems, but you should not assume it.

Nor is this change guaranteed to cause there to be only one access to  
"__malloc_hook", unless you change it to be declared volatile.  (And,  
in fact, if you're optimizing, I would've expected it to be only one  
access in the original code on nearly all platforms.)  Mutex  
protection around accesses to the hook variable would be the safest  
(and wouldn't require temporary variables or volatile qualifiers),  
though performance would not be as good.

Ken

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [mituharu@math.s.chiba-u.ac.jp: Re: emacs-22.1 with GTK dumps core when Gnome wigets clicked]
  2007-06-25  5:33     ` Ken Raeburn
@ 2007-06-25  8:30       ` YAMAMOTO Mitsuharu
  2007-06-25  9:30         ` Ken Raeburn
  0 siblings, 1 reply; 23+ messages in thread
From: YAMAMOTO Mitsuharu @ 2007-06-25  8:30 UTC (permalink / raw)
  To: Ken Raeburn; +Cc: emacs-devel

>>>>> On Mon, 25 Jun 2007 01:33:59 -0400, Ken Raeburn <raeburn@raeburn.org> said:

>> !   /* Copy the value of __malloc_hook to an automatic variable in  
>> case
>> !      __malloc_hook is modified in another thread between its
>> !      NULL-check and the use.  */
>> !   hook = __malloc_hook;
>> !   return (hook != NULL ? *hook : _malloc_internal) (size);
>> }
>> 
>> Assignment is not guaranteed to be atomic.  It probably is on 32- 
>> bit systems, but you should not assume it.

> Nor is this change guaranteed to cause there to be only one access to  
> "__malloc_hook", unless you change it to be declared volatile.  (And,  
> in fact, if you're optimizing, I would've expected it to be only one  
> access in the original code on nearly all platforms.)

As a matter of fact, the original code caused a real problem:

  http://lists.gnu.org/archive/html/bug-gnu-emacs/2007-06/msg00170.html

> Mutex protection around accesses to the hook variable would be the
> safest (and wouldn't require temporary variables or volatile
> qualifiers), though performance would not be as good.

Temporary variables will be necessary so that we can unlock the mutex
before the actual call to the hook.  We also need to add some special
functions to change the hook variables and use them instead of
assignments to the hook variables in alloc.c.  That's why I said that
"malloc in glibc 2.5 also does the same thing and I suspect that we
cannot do better as long as we try to keep the same interface with
respect to __malloc_hook etc." in
http://lists.gnu.org/archive/html/emacs-devel/2007-06/msg01503.html

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [mituharu@math.s.chiba-u.ac.jp: Re: emacs-22.1 with GTK dumps core when Gnome wigets clicked]
  2007-06-25  8:30       ` YAMAMOTO Mitsuharu
@ 2007-06-25  9:30         ` Ken Raeburn
  2007-06-25 10:05           ` YAMAMOTO Mitsuharu
  0 siblings, 1 reply; 23+ messages in thread
From: Ken Raeburn @ 2007-06-25  9:30 UTC (permalink / raw)
  To: YAMAMOTO Mitsuharu; +Cc: emacs-devel

On Jun 25, 2007, at 04:30, YAMAMOTO Mitsuharu wrote:
>> Nor is this change guaranteed to cause there to be only one access to
>> "__malloc_hook", unless you change it to be declared volatile.  (And,
>> in fact, if you're optimizing, I would've expected it to be only one
>> access in the original code on nearly all platforms.)
>
> As a matter of fact, the original code caused a real problem:
>
>   http://lists.gnu.org/archive/html/bug-gnu-emacs/2007-06/ 
> msg00170.html

I'd believe it could be, sure.  But if that code were produced with  
gcc -O2, I'd be disappointed.  Other compilers ... eh, some of them  
aren't that good. :-)

>> Mutex protection around accesses to the hook variable would be the
>> safest (and wouldn't require temporary variables or volatile
>> qualifiers), though performance would not be as good.
>
> Temporary variables will be necessary so that we can unlock the mutex
> before the actual call to the hook.  We also need to add some special
> functions to change the hook variables and use them instead of
> assignments to the hook variables in alloc.c.  That's why I said that
> "malloc in glibc 2.5 also does the same thing and I suspect that we
> cannot do better as long as we try to keep the same interface with
> respect to __malloc_hook etc." in
> http://lists.gnu.org/archive/html/emacs-devel/2007-06/msg01503.html

Yes, I think keeping the current glibc interface -- at least, as the  
one we actually use -- seems like a poor idea.  Though the change  
should be coordinated with glibc maintainers, of course.

Ken

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [mituharu@math.s.chiba-u.ac.jp: Re: emacs-22.1 with GTK dumps core when Gnome wigets clicked]
  2007-06-25  9:30         ` Ken Raeburn
@ 2007-06-25 10:05           ` YAMAMOTO Mitsuharu
  2007-06-25 15:46             ` Richard Stallman
  2007-06-25 15:46             ` Richard Stallman
  0 siblings, 2 replies; 23+ messages in thread
From: YAMAMOTO Mitsuharu @ 2007-06-25 10:05 UTC (permalink / raw)
  To: Ken Raeburn; +Cc: emacs-devel

>>>>> On Mon, 25 Jun 2007 05:30:14 -0400, Ken Raeburn <raeburn@raeburn.org> said:

>> We also need to add some special functions to change the hook
>> variables and use them instead of assignments to the hook variables
>> in alloc.c.  That's why I said that "malloc in glibc 2.5 also does
>> the same thing and I suspect that we cannot do better as long as we
>> try to keep the same interface with respect to __malloc_hook etc."
>> in
>> http://lists.gnu.org/archive/html/emacs-devel/2007-06/msg01503.html

> Yes, I think keeping the current glibc interface -- at least, as the
> one we actually use -- seems like a poor idea.  Though the change
> should be coordinated with glibc maintainers, of course.

It looks like a medium-term solution.  Then we should also consider
options other than maintaining the current gmalloc.c.  Namely,

  1) Include a newer version of GNU malloc (hopefully with a better
     interface for hooks).
  2) Use the system malloc if pthreads are needed.
     (emacs_blocked_malloc etc. will be unnecessary when SYNC_INPUT
      becomes default.)

For a short-term solution, do you agree to install my patch for Emacs
22.2?  Unfortunately we couldn't notice that gmalloc.c was
thread-unsafe until very recently, and we needed an immediate solution
then (although it failed to solve the problem actually).

http://lists.gnu.org/archive/html/emacs-devel/2007-03/msg00856.html

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [mituharu@math.s.chiba-u.ac.jp: Re: emacs-22.1 with GTK dumps core when Gnome wigets clicked]
  2007-06-25 10:05           ` YAMAMOTO Mitsuharu
@ 2007-06-25 15:46             ` Richard Stallman
  2007-06-26  3:45               ` YAMAMOTO Mitsuharu
  2007-06-25 15:46             ` Richard Stallman
  1 sibling, 1 reply; 23+ messages in thread
From: Richard Stallman @ 2007-06-25 15:46 UTC (permalink / raw)
  To: YAMAMOTO Mitsuharu; +Cc: raeburn, emacs-devel

    For a short-term solution, do you agree to install my patch for Emacs
    22.2?  Unfortunately we couldn't notice that gmalloc.c was
    thread-unsafe until very recently, and we needed an immediate solution
    then (although it failed to solve the problem actually).

I agree we need a solution for this now.  I am not sure (from
following the discussion with one eye) whether your solution actually
works, or whether it needs further change, or whether (if so) you've
done the further change.

What is the status of your fix?

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [mituharu@math.s.chiba-u.ac.jp: Re: emacs-22.1 with GTK dumps core when Gnome wigets clicked]
  2007-06-25 10:05           ` YAMAMOTO Mitsuharu
  2007-06-25 15:46             ` Richard Stallman
@ 2007-06-25 15:46             ` Richard Stallman
  2007-06-25 16:33               ` Stefan Monnier
  1 sibling, 1 reply; 23+ messages in thread
From: Richard Stallman @ 2007-06-25 15:46 UTC (permalink / raw)
  To: YAMAMOTO Mitsuharu; +Cc: raeburn, emacs-devel

      1) Include a newer version of GNU malloc (hopefully with a better
	 interface for hooks).

I have nothing against that.

      2) Use the system malloc if pthreads are needed.
	 (emacs_blocked_malloc etc. will be unnecessary when SYNC_INPUT
	  becomes default.)

There is a serious problem with SYNC_INPUT, which is the reason it has
not been made the default.  The problem is that it makes Emacs unable
to update the display except when it can read input.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [mituharu@math.s.chiba-u.ac.jp: Re: emacs-22.1 with GTK dumps core when Gnome wigets clicked]
  2007-06-25 15:46             ` Richard Stallman
@ 2007-06-25 16:33               ` Stefan Monnier
  2007-06-25 17:04                 ` Chong Yidong
  2007-06-26 16:58                 ` Richard Stallman
  0 siblings, 2 replies; 23+ messages in thread
From: Stefan Monnier @ 2007-06-25 16:33 UTC (permalink / raw)
  To: rms; +Cc: raeburn, YAMAMOTO Mitsuharu, emacs-devel

> There is a serious problem with SYNC_INPUT, which is the reason it has
> not been made the default.  The problem is that it makes Emacs unable
> to update the display except when it can read input.

You are still somewhat unclear about how SYNC_INPUT works.  It can update
the display just fine even when Emacs can't read input.  The only times when
it can't update the display is when it's stuck in an endless loop with no
QUIT or UNBLOCK_INPUT in sight.


        Stefan

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [mituharu@math.s.chiba-u.ac.jp: Re: emacs-22.1 with GTK dumps core when Gnome wigets clicked]
  2007-06-25 16:33               ` Stefan Monnier
@ 2007-06-25 17:04                 ` Chong Yidong
  2007-06-26 16:58                   ` Richard Stallman
  2007-06-26 16:58                 ` Richard Stallman
  1 sibling, 1 reply; 23+ messages in thread
From: Chong Yidong @ 2007-06-25 17:04 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: raeburn, rms, YAMAMOTO Mitsuharu, emacs-devel

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> There is a serious problem with SYNC_INPUT, which is the reason it has
>> not been made the default.  The problem is that it makes Emacs unable
>> to update the display except when it can read input.
>
> You are still somewhat unclear about how SYNC_INPUT works.  It can update
> the display just fine even when Emacs can't read input.  The only times when
> it can't update the display is when it's stuck in an endless loop with no
> QUIT or UNBLOCK_INPUT in sight.

I was under the impression, from the last time this was discussed on
emacs-devel, that the plan was to move to SYNC_INPUT after the Emacs
22.1 release.  If this is still the case, then I can go through the
Emacs sources to find and fix loops lacking QUIT.  Then we can make
the switch.

(On the other hand, it would be easier to do this after the unicode
merge.  I have several other cleanups that are also sitting around
waiting for the merge.)

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [mituharu@math.s.chiba-u.ac.jp: Re: emacs-22.1 with GTK dumps core when Gnome wigets clicked]
  2007-06-25 15:46             ` Richard Stallman
@ 2007-06-26  3:45               ` YAMAMOTO Mitsuharu
  2007-06-26 22:48                 ` Richard Stallman
  0 siblings, 1 reply; 23+ messages in thread
From: YAMAMOTO Mitsuharu @ 2007-06-26  3:45 UTC (permalink / raw)
  To: rms; +Cc: raeburn, emacs-devel

>>>>> On Mon, 25 Jun 2007 11:46:05 -0400, Richard Stallman <rms@gnu.org> said:

>     For a short-term solution, do you agree to install my patch for
>     Emacs 22.2?  Unfortunately we couldn't notice that gmalloc.c was
>     thread-unsafe until very recently, and we needed an immediate
>     solution then (although it failed to solve the problem
>     actually).

> I agree we need a solution for this now.  I am not sure (from
> following the discussion with one eye) whether your solution
> actually works, or whether it needs further change, or whether (if
> so) you've done the further change.

> What is the status of your fix?

As for the issue on the hook variables such as __malloc_hook, it will
work at least as well as glibc malloc.  But neither is correct in a
strict sense.  I've just installed my patch to the trunk and the
EMACS_22_BASE branch with adding a comment below:

  /* Copy the value of __malloc_hook to an automatic variable in case
     __malloc_hook is modified in another thread between its
     NULL-check and the use.

     Note: Strictly speaking, this is not a right solution.  We should
     use mutexes to access non-read-only variables that are shared
     among multiple threads.  We just leave it for compatibility with
     glibc malloc (i.e., assignments to __malloc_hook) for now.  */

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [mituharu@math.s.chiba-u.ac.jp: Re: emacs-22.1 with GTK dumps core when Gnome wigets clicked]
  2007-06-25 16:33               ` Stefan Monnier
  2007-06-25 17:04                 ` Chong Yidong
@ 2007-06-26 16:58                 ` Richard Stallman
  1 sibling, 0 replies; 23+ messages in thread
From: Richard Stallman @ 2007-06-26 16:58 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: raeburn, mituharu, emacs-devel

    You are still somewhat unclear about how SYNC_INPUT works.  It can update
    the display just fine even when Emacs can't read input.  The only times when
    it can't update the display is when it's stuck in an endless loop with no
    QUIT or UNBLOCK_INPUT in sight.

I stand corrected.  In that case, let's make it the default.
As Yidong mentioned, we need to find such loops and fix them.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [mituharu@math.s.chiba-u.ac.jp: Re: emacs-22.1 with GTK dumps core when Gnome wigets clicked]
  2007-06-25 17:04                 ` Chong Yidong
@ 2007-06-26 16:58                   ` Richard Stallman
  2007-06-26 21:20                     ` David Kastrup
  2007-06-26 23:35                     ` Chong Yidong
  0 siblings, 2 replies; 23+ messages in thread
From: Richard Stallman @ 2007-06-26 16:58 UTC (permalink / raw)
  To: Chong Yidong; +Cc: raeburn, monnier, mituharu, emacs-devel

    I was under the impression, from the last time this was discussed on
    emacs-devel, that the plan was to move to SYNC_INPUT after the Emacs
    22.1 release.  If this is still the case, then I can go through the
    Emacs sources to find and fix loops lacking QUIT.  Then we can make
    the switch.

Please do it.

    (On the other hand, it would be easier to do this after the unicode
    merge.  I have several other cleanups that are also sitting around
    waiting for the merge.)

How about doing it now in the Unicode-2 branch?

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [mituharu@math.s.chiba-u.ac.jp: Re: emacs-22.1 with GTK dumps core when Gnome wigets clicked]
  2007-06-26 16:58                   ` Richard Stallman
@ 2007-06-26 21:20                     ` David Kastrup
  2007-06-26 23:01                       ` Jason Rumney
  2007-06-27 16:47                       ` Richard Stallman
  2007-06-26 23:35                     ` Chong Yidong
  1 sibling, 2 replies; 23+ messages in thread
From: David Kastrup @ 2007-06-26 21:20 UTC (permalink / raw)
  To: rms; +Cc: Chong Yidong, raeburn, monnier, mituharu, emacs-devel

Richard Stallman <rms@gnu.org> writes:

>     I was under the impression, from the last time this was discussed on
>     emacs-devel, that the plan was to move to SYNC_INPUT after the Emacs
>     22.1 release.  If this is still the case, then I can go through the
>     Emacs sources to find and fix loops lacking QUIT.  Then we can make
>     the switch.
>
> Please do it.
>
>     (On the other hand, it would be easier to do this after the unicode
>     merge.  I have several other cleanups that are also sitting around
>     waiting for the merge.)
>
> How about doing it now in the Unicode-2 branch?

Unless I am mistaken, we have some semimanual merge going from the
trunk to unicode-2 but not the other way round.  So putting stuff in
the trunk will tend to end up being less manual work.

Miles, Handa-san?  Am I mistaken here?

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [mituharu@math.s.chiba-u.ac.jp: Re: emacs-22.1 with GTK dumps core when Gnome wigets clicked]
  2007-06-26  3:45               ` YAMAMOTO Mitsuharu
@ 2007-06-26 22:48                 ` Richard Stallman
  0 siblings, 0 replies; 23+ messages in thread
From: Richard Stallman @ 2007-06-26 22:48 UTC (permalink / raw)
  To: YAMAMOTO Mitsuharu; +Cc: raeburn, emacs-devel

Thank you.

    As for the issue on the hook variables such as __malloc_hook, it will
    work at least as well as glibc malloc.  But neither is correct in a
    strict sense.

In practice, it is not crucial that this code be formally correct.
It is enough that it operate correctly on the platforms it is used on.

There are not very many modern platforms any more, and some of them
don't use gmalloc.  Would people like to check the modern platforms
and verify that there is not really any problem?

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [mituharu@math.s.chiba-u.ac.jp: Re: emacs-22.1 with GTK dumps core when Gnome wigets clicked]
  2007-06-26 21:20                     ` David Kastrup
@ 2007-06-26 23:01                       ` Jason Rumney
  2007-06-27 16:47                       ` Richard Stallman
  1 sibling, 0 replies; 23+ messages in thread
From: Jason Rumney @ 2007-06-26 23:01 UTC (permalink / raw)
  To: David Kastrup; +Cc: rms, Chong Yidong, emacs-devel, raeburn, monnier, mituharu

David Kastrup wrote:
> Unless I am mistaken, we have some semimanual merge going from the
> trunk to unicode-2 but not the other way round.  So putting stuff in
> the trunk will tend to end up being less manual work.
>   

Only if the intention is to merge the change into the trunk separately
from the unicode branch merge.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [mituharu@math.s.chiba-u.ac.jp: Re: emacs-22.1 with GTK dumps core when Gnome wigets clicked]
  2007-06-26 16:58                   ` Richard Stallman
  2007-06-26 21:20                     ` David Kastrup
@ 2007-06-26 23:35                     ` Chong Yidong
  2007-06-27  1:31                       ` Stefan Monnier
  2007-06-27 19:50                       ` Richard Stallman
  1 sibling, 2 replies; 23+ messages in thread
From: Chong Yidong @ 2007-06-26 23:35 UTC (permalink / raw)
  To: rms; +Cc: raeburn, monnier, mituharu, emacs-devel

Richard Stallman <rms@gnu.org> writes:

>     I was under the impression, from the last time this was discussed on
>     emacs-devel, that the plan was to move to SYNC_INPUT after the Emacs
>     22.1 release.  If this is still the case, then I can go through the
>     Emacs sources to find and fix loops lacking QUIT.  Then we can make
>     the switch.
>
> Please do it.
>
>     (On the other hand, it would be easier to do this after the unicode
>     merge.  I have several other cleanups that are also sitting around
>     waiting for the merge.)
>
> How about doing it now in the Unicode-2 branch?

Then it would have to be merged back into the trunk.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [mituharu@math.s.chiba-u.ac.jp: Re: emacs-22.1 with GTK dumps core when Gnome wigets clicked]
  2007-06-26 23:35                     ` Chong Yidong
@ 2007-06-27  1:31                       ` Stefan Monnier
  2007-06-27 19:50                       ` Richard Stallman
  1 sibling, 0 replies; 23+ messages in thread
From: Stefan Monnier @ 2007-06-27  1:31 UTC (permalink / raw)
  To: Chong Yidong; +Cc: raeburn, rms, mituharu, emacs-devel

> Then it would have to be merged back into the trunk.

Given that the turnk is merged into the trunk, the "merge" of the unicode
branch into the trunk will be nothing more than a funny form or renaming
where the branch we used to call "unicode" now becomes "trunk".  So "merging
it back" will be a no-op, really.


        Stefan

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [mituharu@math.s.chiba-u.ac.jp: Re: emacs-22.1 with GTK dumps core when Gnome wigets clicked]
  2007-06-26 21:20                     ` David Kastrup
  2007-06-26 23:01                       ` Jason Rumney
@ 2007-06-27 16:47                       ` Richard Stallman
  1 sibling, 0 replies; 23+ messages in thread
From: Richard Stallman @ 2007-06-27 16:47 UTC (permalink / raw)
  To: David Kastrup; +Cc: cyd, raeburn, monnier, mituharu, emacs-devel

    >     (On the other hand, it would be easier to do this after the unicode
    >     merge.  I have several other cleanups that are also sitting around
    >     waiting for the merge.)
    >
    > How about doing it now in the Unicode-2 branch?

    Unless I am mistaken, we have some semimanual merge going from the
    trunk to unicode-2 but not the other way round.  So putting stuff in
    the trunk will tend to end up being less manual work.

I presume the reason he wants to do these after the unicode-2 merge is
that they interact with the unicode-2 changes, and would have to be
done differently in the trunk and in unicode-2.  If so, doing them
now, only in unicode-2, would avoid the need for extra work.

If they don't interact, then maybe then can be done in the trunk now.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [mituharu@math.s.chiba-u.ac.jp: Re: emacs-22.1 with GTK dumps core when Gnome wigets clicked]
  2007-06-26 23:35                     ` Chong Yidong
  2007-06-27  1:31                       ` Stefan Monnier
@ 2007-06-27 19:50                       ` Richard Stallman
  1 sibling, 0 replies; 23+ messages in thread
From: Richard Stallman @ 2007-06-27 19:50 UTC (permalink / raw)
  To: Chong Yidong; +Cc: raeburn, monnier, mituharu, emacs-devel

    > How about doing it now in the Unicode-2 branch?

    Then it would have to be merged back into the trunk.

Yes, along with the rest of unicode-2.  But that will be easy, right?
The changes in the trunk are getting merged into unicode-2, so when
we do that merge (in a few weeks, I think), it should be straightforward.

^ permalink raw reply	[flat|nested] 23+ messages in thread

end of thread, other threads:[~2007-06-27 19:50 UTC | newest]

Thread overview: 23+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2007-06-17 21:49 [mituharu@math.s.chiba-u.ac.jp: Re: emacs-22.1 with GTK dumps core when Gnome wigets clicked] Richard Stallman
2007-06-18  1:11 ` YAMAMOTO Mitsuharu
2007-06-21 10:13   ` Jan Djärv
2007-06-21 11:36     ` YAMAMOTO Mitsuharu
2007-06-25  5:33     ` Ken Raeburn
2007-06-25  8:30       ` YAMAMOTO Mitsuharu
2007-06-25  9:30         ` Ken Raeburn
2007-06-25 10:05           ` YAMAMOTO Mitsuharu
2007-06-25 15:46             ` Richard Stallman
2007-06-26  3:45               ` YAMAMOTO Mitsuharu
2007-06-26 22:48                 ` Richard Stallman
2007-06-25 15:46             ` Richard Stallman
2007-06-25 16:33               ` Stefan Monnier
2007-06-25 17:04                 ` Chong Yidong
2007-06-26 16:58                   ` Richard Stallman
2007-06-26 21:20                     ` David Kastrup
2007-06-26 23:01                       ` Jason Rumney
2007-06-27 16:47                       ` Richard Stallman
2007-06-26 23:35                     ` Chong Yidong
2007-06-27  1:31                       ` Stefan Monnier
2007-06-27 19:50                       ` Richard Stallman
2007-06-26 16:58                 ` Richard Stallman
2007-06-24 23:47   ` Richard Stallman

Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/emacs.git
	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.