[PATCH] some misuse of GCPROs

[PATCH] some misuse of GCPROs

[Dmitry Antipov]

[-- Attachment #1: Type: text/plain, Size: 70 bytes --]

...found while hacking around GC_MARK_STACK with DEBUG_GCPRO.

Dmitry

[-- Attachment #2: gcpro_fixes.patch --]
[-- Type: text/plain, Size: 1992 bytes --]

=== modified file 'src/ChangeLog'
--- src/ChangeLog	2011-11-18 12:41:36 +0000
+++ src/ChangeLog	2011-11-18 14:02:15 +0000
@@ -1,3 +1,9 @@
+2011-11-18  Dmitry Antipov  <dmantipov@yandex.ru>
+
+	* keymap.c (Fwhere_is_internal): Add missing RETURN_UNGCPROs.
+
+	* fileio.c (Finsert_file_contents): Use inner_gcpro.
+
 2011-11-18  Eli Zaretskii  <eliz@gnu.org>
 
 	* dispnew.c (swap_glyph_pointers): Swap the used[] arrays and the

=== modified file 'src/fileio.c'
--- src/fileio.c	2011-09-30 20:22:01 +0000
+++ src/fileio.c	2011-11-18 13:58:04 +0000
@@ -3686,6 +3686,7 @@
       int this_count = SPECPDL_INDEX ();
       int multibyte = ! NILP (BVAR (current_buffer, enable_multibyte_characters));
       Lisp_Object conversion_buffer;
+      struct gcpro inner_gcpro1;
 
       conversion_buffer = code_conversion_save (1, multibyte);
 
@@ -3701,7 +3702,7 @@
       inserted = 0;		/* Bytes put into CONVERSION_BUFFER so far.  */
       unprocessed = 0;		/* Bytes not processed in previous loop.  */
 
-      GCPRO1 (conversion_buffer);
+      GCPRO1_VAR (conversion_buffer, inner_gcpro);
       while (how_much < total)
 	{
 	  /* We read one bunch by one (READ_BUF_SIZE bytes) to allow
@@ -3729,7 +3730,7 @@
 	  if (coding.carryover_bytes > 0)
 	    memcpy (read_buf, coding.carryover, unprocessed);
 	}
-      UNGCPRO;
+      UNGCPRO_VAR (inner_gcpro);
       emacs_close (fd);
 
       /* We should remove the unwind_protect calling

=== modified file 'src/keymap.c'
--- src/keymap.c	2011-11-17 17:40:48 +0000
+++ src/keymap.c	2011-11-18 13:50:01 +0000
@@ -2624,11 +2624,11 @@
       /* We have a list of advertised bindings.  */
       while (CONSP (tem))
 	if (EQ (shadow_lookup (keymaps, XCAR (tem), Qnil, 0), definition))
-	  return XCAR (tem);
+	  RETURN_UNGCPRO (XCAR (tem));
 	else
 	  tem = XCDR (tem);
       if (EQ (shadow_lookup (keymaps, tem, Qnil, 0), definition))
-	return tem;
+	RETURN_UNGCPRO (tem);
     }
 
   sequences = Freverse (where_is_internal (definition, keymaps,

Re: [PATCH] some misuse of GCPROs

[Stefan Monnier]

> +	* keymap.c (Fwhere_is_internal): Add missing RETURN_UNGCPROs.

Thanks, installed.

> +	* fileio.c (Finsert_file_contents): Use inner_gcpro.

I really dislike this inner_gcpro business, so I haven't installed
this hunk.  What is it needed for?


        Stefan

Re: [PATCH] some misuse of GCPROs

[Dmitry Antipov]

On 11/18/2011 08:51 PM, Stefan Monnier wrote:

> I really dislike this inner_gcpro business, so I haven't installed
> this hunk.  What is it needed for?

I too, but this is a fix for 'reuse GCPRO' error. The following code

...
Lisp_Object foo;
struct gcpro gcpro1;
...
GCPRO1 (foo);
...
if (...)
   {
      Lisp_Object bar;
      ...
      GCPRO1 (bar);
      ...
      UNGCPRO;
   }
....

is wrong because it clears the protection of 'foo' by re-using
'gcpro1' to protect 'bar'.

It should be:

...
Lisp_Object foo;
struct gcpro gcpro1;
...
GCPRO1 (foo);
...
if (...)
   {
      Lisp_Object bar;
      struct gcpro inner_gcpro1;
      ...
      GCPRO1_VAR (bar, inner_gcpro);
      ...
      UNGCPRO_VAR (inner_gcpro);
   }
....

Dmitry

Re: [PATCH] some misuse of GCPROs

[Paul Eggert]

On 11/18/11 08:51, Stefan Monnier wrote:
> I really dislike this inner_gcpro business, so I haven't installed
> this hunk.  What is it needed for?

It's needed to prevent gcprolist from becoming corrupted when
GC_MARK_STACK is not GC_MAKE_GCPROS_NOOPS.  Without the patch,
Finsert_file_contents sometimes invokes GCPRO5 and then GCPRO1
on the same struct gcpro, without an intervening UNGCPRO.
This corrupts the list.

Here's a proposed alternative patch that avoids inner_gcpro.
It's simpler code, though it has the minor run-time disadvantage
of GCPROing conversion_buffer more than is strictly needed
(until insert-file-contents returns).

=== modified file 'src/ChangeLog'
--- src/ChangeLog	2011-11-18 18:29:29 +0000
+++ src/ChangeLog	2011-11-18 18:59:32 +0000
@@ -1,5 +1,9 @@
 2011-11-18  Paul Eggert  <eggert@cs.ucla.edu>
 
+	* fileio.c (Finsert_file_contents): Don't corrupt gcprolist
+	if GC_MARK_STACK is not GC_MAKE_GCPROS_NOOPS.  Reported by Dmitry Antipov
+	in <http://lists.gnu.org/archive/html/emacs-devel/2011-11/msg00263.html>.
+
 	Fix minor problems found by static checking.
 	* dispextern.h, xdisp.c (row_hash): Declare extern only if XASSERTS.
 	* dispnew.c (verify_row_hash): Now static.

=== modified file 'src/fileio.c'
--- src/fileio.c	2011-09-30 20:22:01 +0000
+++ src/fileio.c	2011-11-18 19:00:20 +0000
@@ -3182,8 +3182,9 @@
   off_t beg_offset, end_offset;
   register EMACS_INT unprocessed;
   int count = SPECPDL_INDEX ();
-  struct gcpro gcpro1, gcpro2, gcpro3, gcpro4, gcpro5;
+  struct gcpro gcpro1, gcpro2, gcpro3, gcpro4, gcpro5, gcpro6;
   Lisp_Object handler, val, insval, orig_filename, old_undo;
+  Lisp_Object conversion_buffer;
   Lisp_Object p;
   EMACS_INT total = 0;
   int not_regular = 0;
@@ -3209,8 +3210,9 @@
   p = Qnil;
   orig_filename = Qnil;
   old_undo = Qnil;
+  conversion_buffer = Qnil;
 
-  GCPRO5 (filename, val, p, orig_filename, old_undo);
+  GCPRO6 (filename, val, p, orig_filename, old_undo, conversion_buffer);
 
   CHECK_STRING (filename);
   filename = Fexpand_file_name (filename, Qnil);
@@ -3685,7 +3687,6 @@
       EMACS_INT this = 0;
       int this_count = SPECPDL_INDEX ();
       int multibyte = ! NILP (BVAR (current_buffer, enable_multibyte_characters));
-      Lisp_Object conversion_buffer;
 
       conversion_buffer = code_conversion_save (1, multibyte);
 
@@ -3701,7 +3702,6 @@
       inserted = 0;		/* Bytes put into CONVERSION_BUFFER so far.  */
       unprocessed = 0;		/* Bytes not processed in previous loop.  */
 
-      GCPRO1 (conversion_buffer);
       while (how_much < total)
 	{
 	  /* We read one bunch by one (READ_BUF_SIZE bytes) to allow
@@ -3729,7 +3729,6 @@
 	  if (coding.carryover_bytes > 0)
 	    memcpy (read_buf, coding.carryover, unprocessed);
 	}
-      UNGCPRO;
       emacs_close (fd);
 
       /* We should remove the unwind_protect calling

Re: [PATCH] some misuse of GCPROs

[Stefan Monnier]

>> I really dislike this inner_gcpro business, so I haven't installed
>> this hunk.  What is it needed for?
> I too, but this is a fix for 'reuse GCPRO' error. The following code

Oh, I see, thanks.  I added the patch below, then.


        Stefan


=== modified file 'src/fileio.c'
--- src/fileio.c	2011-11-20 02:29:42 +0000
+++ src/fileio.c	2011-11-20 04:31:52 +0000
@@ -3686,6 +3686,7 @@
       int this_count = SPECPDL_INDEX ();
       int multibyte = ! NILP (BVAR (current_buffer, enable_multibyte_characters));
       Lisp_Object conversion_buffer;
+      struct gcpro gcpro1;
 
       conversion_buffer = code_conversion_save (1, multibyte);