recursive load case in openp

recursive load case in openp

[Colin Walters]

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

Hi, 

I found a bug (or rather my init files did) in the current CVS.  It
appears to have been introduced by the following change:

2002-03-29  Eli Zaretskii  <eliz@is.elta.co.il>

	* lread.c (openp, Fload): Encode the file name before passing it
	to `stat', `access', and `emacs_open'.
	(openp): GCPRO the encoded file name.  Don't recompute Lisp
	strings unnecessarily.

The problem is reproducible on my system by starting emacs like:

emacs -q --no-site-file --eval '(set-language-environment "utf-8")'

What seems to be happening is that openp eventually calls
encode_coding_string, which eventually calls temp_output_buffer_setup,
which then runs the hook variable `temp-buffer-setup-hook', whose value
defaults to the single symbol `help-mode-setup'.  Therefore, since
help-mode is autoloaded, emacs will attempt to load it, and enter Fload,
and therefore reenter openp, try to load help-mode again...

The following patch fixes the problem on my system; Does anyone have any
objections to fixing the problem in this way?



[-- Attachment #2: emacs.patch --]
[-- Type: text/x-patch, Size: 3202 bytes --]

Index: bytecode.c
===================================================================
RCS file: /cvsroot/emacs/emacs/src/bytecode.c,v
retrieving revision 1.68
diff -u -r1.68 bytecode.c
--- bytecode.c	20 Mar 2002 07:44:54 -0000	1.68
+++ bytecode.c	8 Apr 2002 02:54:42 -0000
@@ -896,7 +896,7 @@
 	case Btemp_output_buffer_setup:
 	  BEFORE_POTENTIAL_GC ();
 	  CHECK_STRING (TOP);
-	  temp_output_buffer_setup (XSTRING (TOP)->data);
+	  temp_output_buffer_setup (XSTRING (TOP)->data, 1);
 	  AFTER_POTENTIAL_GC ();
 	  TOP = Vstandard_output;
 	  break;
Index: coding.c
===================================================================
RCS file: /cvsroot/emacs/emacs/src/coding.c,v
retrieving revision 1.240
diff -u -r1.240 coding.c
--- coding.c	11 Mar 2002 19:21:09 -0000	1.240
+++ coding.c	8 Apr 2002 02:54:43 -0000
@@ -5801,7 +5801,7 @@
   record_unwind_protect (Fset_buffer, Fcurrent_buffer ());
   record_unwind_protect (code_convert_region_unwind, Qnil);
   GCPRO1 (str);
-  temp_output_buffer_setup (" *code-converting-work*");
+  temp_output_buffer_setup (" *code-converting-work*", 0);
   set_buffer_internal (XBUFFER (Vstandard_output));
   /* We must insert the contents of STR as is without
      unibyte<->multibyte conversion.  For that, we adjust the
Index: lisp.h
===================================================================
RCS file: /cvsroot/emacs/emacs/src/lisp.h,v
retrieving revision 1.415
diff -u -r1.415 lisp.h
--- lisp.h	21 Mar 2002 12:17:51 -0000	1.415
+++ lisp.h	8 Apr 2002 02:54:43 -0000
@@ -2424,7 +2424,7 @@
 EXFUN (Ferror_message_string, 1);
 extern Lisp_Object Vstandard_output, Qstandard_output;
 extern Lisp_Object Qexternal_debugging_output;
-extern void temp_output_buffer_setup P_ ((char *));
+extern void temp_output_buffer_setup P_ ((char *, int));
 extern int print_level, print_escape_newlines;
 extern Lisp_Object Qprint_escape_newlines;
 extern void write_string P_ ((char *, int));
Index: print.c
===================================================================
RCS file: /cvsroot/emacs/emacs/src/print.c,v
retrieving revision 1.174
diff -u -r1.174 print.c
--- print.c	16 Mar 2002 06:53:35 -0000	1.174
+++ print.c	8 Apr 2002 02:54:44 -0000
@@ -577,8 +577,9 @@
 
 
 void
-temp_output_buffer_setup (bufname)
+temp_output_buffer_setup (bufname, hooks)
     char *bufname;
+    int hooks;
 {
   int count = specpdl_ptr - specpdl;
   register struct buffer *old = current_buffer;
@@ -599,7 +600,8 @@
   Ferase_buffer ();
   XSETBUFFER (buf, current_buffer);
 
-  Frun_hooks (1, &Qtemp_buffer_setup_hook);
+  if (hooks)
+    Frun_hooks (1, &Qtemp_buffer_setup_hook);
 
   unbind_to (count, Qnil);
 
@@ -618,7 +620,7 @@
 
   GCPRO1 (args);
   record_unwind_protect (Fset_buffer, Fcurrent_buffer ());
-  temp_output_buffer_setup (bufname);
+  temp_output_buffer_setup (bufname, 1);
   buf = Vstandard_output;
   UNGCPRO;
 
@@ -663,7 +665,7 @@
   GCPRO1(args);
   name = Feval (Fcar (args));
   CHECK_STRING (name);
-  temp_output_buffer_setup (XSTRING (name)->data);
+  temp_output_buffer_setup (XSTRING (name)->data, 1);
   buf = Vstandard_output;
   UNGCPRO;
 

Re: recursive load case in openp

[Eli Zaretskii]

On 7 Apr 2002, Colin Walters wrote:

> What seems to be happening is that openp eventually calls
> encode_coding_string, which eventually calls temp_output_buffer_setup,
> which then runs the hook variable `temp-buffer-setup-hook', whose value
> defaults to the single symbol `help-mode-setup'.  Therefore, since
> help-mode is autoloaded, emacs will attempt to load it, and enter Fload,
> and therefore reenter openp, try to load help-mode again...
> 
> The following patch fixes the problem on my system; Does anyone have any
> objections to fixing the problem in this way?

I'd like to find a solution that doesn't prevent legitimate hooks from 
running.  The problem my change introduced is very specific, so I think 
we should try to find a solution that prevents this specific scenario 
from screwing up Emacs.

I will try to find such a solution.  Thanks for reporting the problem and 
its analysis.

Re: recursive load case in openp

[Richard Stallman]

    > What seems to be happening is that openp eventually calls
    > encode_coding_string, which eventually calls temp_output_buffer_setup,
    > which then runs the hook variable `temp-buffer-setup-hook', whose value
    > defaults to the single symbol `help-mode-setup'.

Why does encode_coding_string call temp_output_buffer_setup?
That is meant for buffers that are going to be displayed.
It should not be used for temporary buffers used for internal
purposes.  I don't think encode_coding_string should call it.

Re: recursive load case in openp

[Kenichi Handa]

Richard Stallman <rms@gnu.org> writes:
> Why does encode_coding_string call temp_output_buffer_setup?

To run post-read-conversion and pre-write-conversion
functions of a coding system.  We must make a temporary
working buffer for them.

> That is meant for buffers that are going to be displayed.
> It should not be used for temporary buffers used for internal
> purposes.

I didn't know that.

> I don't think encode_coding_string should call it.

Ok.  Then, let's make a function which does almost the same
thing as temp_output_buffer_setup.  That function doesn't
run temp-buffer-setup-hook, and instead of binding
Qstandard_output, it returns a working buffer.

And, change run_pre_post_conversion_on_str and
Finsert_file_contents to call it.

---
Ken'ichi HANDA
handa@etl.go.jp

Re: recursive load case in openp

[Richard Stallman]

    > Why does encode_coding_string call temp_output_buffer_setup?

    To run post-read-conversion and pre-write-conversion
    functions of a coding system.  We must make a temporary
    working buffer for them.

I don't see anything like that in the code of
temp_output_buffer_setup.  What exactly in the code of
temp_output_buffer_setup do you actually want to do here?

Re: recursive load case in openp

[Kenichi Handa]

Richard Stallman <rms@gnu.org> writes:
>>  Why does encode_coding_string call
>> temp_output_buffer_setup?
>     To run post-read-conversion and pre-write-conversion
> functions of a coding system.  We must make a temporary
> working buffer for them.

> I don't see anything like that in the code of
> temp_output_buffer_setup.  What exactly in the code of
> temp_output_buffer_setup do you actually want to do here?

Here's a copy of temp_output_buffer_setup.  I just need
lines from Fset_buffer to Ferase_buffer (inclusive).

void
temp_output_buffer_setup (bufname)
    char *bufname;
{
  int count = specpdl_ptr - specpdl;
  register struct buffer *old = current_buffer;
  register Lisp_Object buf;

  record_unwind_protect (set_buffer_if_live, Fcurrent_buffer ());

  Fset_buffer (Fget_buffer_create (build_string (bufname)));

  current_buffer->directory = old->directory;
  current_buffer->read_only = Qnil;
  current_buffer->filename = Qnil;
  current_buffer->undo_list = Qt;
  current_buffer->overlays_before = Qnil;
  current_buffer->overlays_after = Qnil;
  current_buffer->enable_multibyte_characters
    = buffer_defaults.enable_multibyte_characters;
  Ferase_buffer ();

  XSETBUFFER (buf, current_buffer);

  Frun_hooks (1, &Qtemp_buffer_setup_hook);

  unbind_to (count, Qnil);

  specbind (Qstandard_output, buf);
}

---
Ken'ichi HANDA
handa@etl.go.jp

Re: recursive load case in openp

[Stefan Monnier]

> Richard Stallman <rms@gnu.org> writes:
> >>  Why does encode_coding_string call
> >> temp_output_buffer_setup?
> >     To run post-read-conversion and pre-write-conversion
> > functions of a coding system.  We must make a temporary
> > working buffer for them.
> 
> > I don't see anything like that in the code of
> > temp_output_buffer_setup.  What exactly in the code of
> > temp_output_buffer_setup do you actually want to do here?
> 
> Here's a copy of temp_output_buffer_setup.  I just need
> lines from Fset_buffer to Ferase_buffer (inclusive).

I would argue that the `run-hooks' part of the function should be
taken out.  Typically, this hook contains help-mode-setup which is
either not what we want (when the buffer is only used internally) or
if it is what we want, it's not enough (we additionally need to call
help-setup-xref).

I've already removed most C calls to with-output-to-temp-buffer
by moving the corresponding code to elisp.

Currently temp_output_buffer_setup is called from bytecode.c,
coding.c and fileio.c (where the calls obviously would rather have
a temp_buffer_setup, with none of that Qtemp_buffer_setup_hook
business) and from Fwith_output_to_temp_buffer which could be recoded
in elisp anyway as well as internal_with_output_to_temp_buffer.

This last one is still used for several things, so we won't
be able to get rid of it immediately:
- " *Danger*" in alloc.c (should this really run temp_buffer_setup_hook ?).
- "*Backtrace*" in eval.c (only used when stack-trace-on-error is non-nil.
  Is this ever used ?  It seems that debug-on-error makes it obsolete).
- "*Help*" in keyboard.c for the help-form (I think this should be
  moved to elisp, but I'm not sure how).
- "*Completions*" in minibuf.c (I have all this code recoded and improved
  in elisp.  And I can't see why *Completions* should use help-mode-setup).
- "*Process List*" in proicess.c (again, should this use help-mode ?).

Of course, in the mean time we should just create
temp_buffer_setup and have temp_output_buffer_setup call it.


	Stefan

Re: recursive load case in openp

[Richard Stallman]

Does this do the job?

*** coding.c.~1.240.~	Mon Mar 11 22:53:04 2002
--- coding.c	Thu Apr 11 16:02:47 2002
***************
*** 5797,5813 ****
    int count = specpdl_ptr - specpdl;
    struct gcpro gcpro1;
    int multibyte = STRING_MULTIBYTE (str);
  
    record_unwind_protect (Fset_buffer, Fcurrent_buffer ());
    record_unwind_protect (code_convert_region_unwind, Qnil);
    GCPRO1 (str);
!   temp_output_buffer_setup (" *code-converting-work*");
!   set_buffer_internal (XBUFFER (Vstandard_output));
    /* We must insert the contents of STR as is without
       unibyte<->multibyte conversion.  For that, we adjust the
       multibyteness of the working buffer to that of STR.  */
    Ferase_buffer ();
!   current_buffer->enable_multibyte_characters = multibyte ? Qt : Qnil;
    insert_from_string (str, 0, 0,
  		      XSTRING (str)->size, STRING_BYTES (XSTRING (str)), 0);
    UNGCPRO;
--- 5797,5826 ----
    int count = specpdl_ptr - specpdl;
    struct gcpro gcpro1;
    int multibyte = STRING_MULTIBYTE (str);
+   Lisp_Object buffer;
+   struct buffer *buf;
  
    record_unwind_protect (Fset_buffer, Fcurrent_buffer ());
    record_unwind_protect (code_convert_region_unwind, Qnil);
    GCPRO1 (str);
! 
!   buffer = Fget_buffer_create (build_string (" *code-converting-work*"));
!   buf = XBUFFER (buffer);
! 
!   buf->directory = current_buffer->directory;
!   buf->read_only = Qnil;
!   buf->filename = Qnil;
!   buf->undo_list = Qt;
!   buf->overlays_before = Qnil;
!   buf->overlays_after = Qnil;
! 
!   set_buffer_internal (buf);
    /* We must insert the contents of STR as is without
       unibyte<->multibyte conversion.  For that, we adjust the
       multibyteness of the working buffer to that of STR.  */
    Ferase_buffer ();
!   buf->enable_multibyte_characters = multibyte ? Qt : Qnil;
! 
    insert_from_string (str, 0, 0,
  		      XSTRING (str)->size, STRING_BYTES (XSTRING (str)), 0);
    UNGCPRO;

Re: recursive load case in openp

[Richard Stallman]

    I would argue that the `run-hooks' part of the function should be
    taken out.

That call is correct for the intended purpose of this function, which
is to support with-output-to-temp-buffer.

Anything which calls temp_output_buffer_setup and does not want to run
that hook should not call temp_output_buffer_setup.  coding.c
and fileio.c should not call it.

    Of course, in the mean time we should just create
    temp_buffer_setup and have temp_output_buffer_setup call it.

The actual code needed in coding.c was too small to be worth
such a subroutine.  I will fix fileio.c also.

Re: recursive load case in openp

[Stefan Monnier]

>     Of course, in the mean time we should just create
>     temp_buffer_setup and have temp_output_buffer_setup call it.
> The actual code needed in coding.c was too small to be worth
> such a subroutine.  I will fix fileio.c also.

I disagree with your assessment of the various costs involved.
I find the cost of maintaining such duplicated code much
higher than the potential CPU and memory cost of a few spills.


	Stefan

Re: recursive load case in openp

[Kenichi Handa]

Richard Stallman <rms@gnu.org> writes:
> Does this do the job?

Yes at least with a few small tests.  We also have to change
Finsert_file_contents.

But, I think:

> !   buffer = Fget_buffer_create (build_string (" *code-converting-work*"));
> !   buf = XBUFFER (buffer);
> ! 
> !   buf->directory = current_buffer->directory;
> !   buf->read_only = Qnil;
> !   buf->filename = Qnil;
> !   buf->undo_list = Qt;
> !   buf->overlays_before = Qnil;
> !   buf->overlays_after = Qnil;
> ! 
> !   set_buffer_internal (buf);
[...]
>     Ferase_buffer ();

it is better that we keep this sequence in a separate
function (or macro), otherwise, for instance, we may forget
to set overlays_before/after to nil in the other place in
the future.

Then, we can call that function from coding.c,
Finsert_file_contents, and also from
temp_output_buffer_setup.

---
Ken'ichi HANDA
handa@etl.go.jp

Re: recursive load case in openp

[Kim F. Storm]

"Stefan Monnier" <monnier+gnu/emacs@RUM.cs.yale.edu> writes:

> >     Of course, in the mean time we should just create
> >     temp_buffer_setup and have temp_output_buffer_setup call it.
> > The actual code needed in coding.c was too small to be worth
> > such a subroutine.  I will fix fileio.c also.
> 
> I disagree with your assessment of the various costs involved.
> I find the cost of maintaining such duplicated code much
> higher than the potential CPU and memory cost of a few spills.

I agree 100%.

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk