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  <address@hidden>
 
+       * 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