* [PATCH] some misuse of GCPROs
@ 2011-11-18 14:06 Dmitry Antipov
2011-11-18 16:51 ` Stefan Monnier
0 siblings, 1 reply; 5+ messages in thread
From: Dmitry Antipov @ 2011-11-18 14:06 UTC (permalink / raw
To: emacs-devel
[-- 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,
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] some misuse of GCPROs
2011-11-18 14:06 [PATCH] some misuse of GCPROs Dmitry Antipov
@ 2011-11-18 16:51 ` Stefan Monnier
2011-11-18 17:24 ` Dmitry Antipov
2011-11-18 19:04 ` Paul Eggert
0 siblings, 2 replies; 5+ messages in thread
From: Stefan Monnier @ 2011-11-18 16:51 UTC (permalink / raw
To: Dmitry Antipov; +Cc: emacs-devel
> + * 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] some misuse of GCPROs
2011-11-18 16:51 ` Stefan Monnier
@ 2011-11-18 17:24 ` Dmitry Antipov
2011-11-20 4:35 ` Stefan Monnier
2011-11-18 19:04 ` Paul Eggert
1 sibling, 1 reply; 5+ messages in thread
From: Dmitry Antipov @ 2011-11-18 17:24 UTC (permalink / raw
To: Stefan Monnier; +Cc: emacs-devel
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] some misuse of GCPROs
2011-11-18 16:51 ` Stefan Monnier
2011-11-18 17:24 ` Dmitry Antipov
@ 2011-11-18 19:04 ` Paul Eggert
1 sibling, 0 replies; 5+ messages in thread
From: Paul Eggert @ 2011-11-18 19:04 UTC (permalink / raw
To: Stefan Monnier; +Cc: Dmitry Antipov, emacs-devel
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] some misuse of GCPROs
2011-11-18 17:24 ` Dmitry Antipov
@ 2011-11-20 4:35 ` Stefan Monnier
0 siblings, 0 replies; 5+ messages in thread
From: Stefan Monnier @ 2011-11-20 4:35 UTC (permalink / raw
To: Dmitry Antipov; +Cc: emacs-devel
>> 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);
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2011-11-20 4:35 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-11-18 14:06 [PATCH] some misuse of GCPROs Dmitry Antipov
2011-11-18 16:51 ` Stefan Monnier
2011-11-18 17:24 ` Dmitry Antipov
2011-11-20 4:35 ` Stefan Monnier
2011-11-18 19:04 ` Paul Eggert
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.