bug#30529: 26.0.91; minibuffer errors change standard-output

bug#30529: 26.0.91; minibuffer errors change standard-output

[Francis Wright]

A user-error in the minibuffer can change the value of standard-output
(to t) within the environment that invoked the minibuffer, whereas I
think the minibuffer should rebind standard-output locally and not
change its global value. Here is a simple way to illustrate this
problem. Evaluate the following:

(defun STANDARD-LISP ()
  "Run Standard LISP with input via the minibuffer and output via a buffer."
  (interactive)
  (switch-to-buffer (get-buffer-create "*Standard LISP*"))
  (let (value
		(standard-output (current-buffer)))
	(while t
	  (terpri)
	  (princ "Eval: ")
	  (setq value (read))
	  ;; (read) errors change standard-output to *Messages* buffer,
	  ;; so...
	  ;; (setq standard-output (current-buffer))
	  (prin1 value) (terpri)
	  (setq value (eval value))
	  (terpri)
	  (princ "====> ") (princ value) (terpri))))

Run the command STANDARD-LISP and type 'foo RETURN in the
minibuffer. The input and output related to 'foo appear correctly in the
buffer *Standard LISP*. Now, in the minibuffer, press the down arrow
key, thereby generating an end of history user-error, then type 'bar
RETURN. The input and output related to 'bar appear incorrectly in the
*Messages* buffer. A workaround is to include the assignment (setq
standard-output (current-buffer)) that is commented out in the function
above.

The distributed binary (i.e. not built by me) MS-Windows version of
Emacs 25.2 shows the same behaviour.


In GNU Emacs 26.0.91 (build 1, x86_64-w64-mingw32)
 of 2018-01-23 built on MONOLITH
Windowing system distributor 'Microsoft Corp.', version 10.0.16299
Recent messages:
Loading delsel...done
Loading paren...done
Starting new Ispell process c:/cygwin64/bin/aspell.exe with british dictionary...
For information about GNU Emacs and the GNU system, type C-h C-a.

Configured using:
 'configure --prefix=/d/emacs/emacs-26.0.91 --without-imagemagick
 --without-dbus'

Configured features:
XPM JPEG TIFF GIF PNG RSVG SOUND NOTIFY ACL GNUTLS LIBXML2 ZLIB
TOOLKIT_SCROLL_BARS LCMS2

Important settings:
  value of $LANG: ENG
  locale-coding-system: cp1252

Major mode: Fundamental

Minor modes in effect:
  shell-dirtrack-mode: t
  show-paren-mode: t
  delete-selection-mode: t
  electric-pair-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  horizontal-scroll-bar-mode: t
  global-font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  buffer-read-only: t
  line-number-mode: t
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message rmc puny dired dired-loaddefs
rfc822 mml mml-sec epa derived epg gnus-util rmail rmail-loaddefs
mm-decode mm-bodies mm-encode mail-parse rfc2231 mailabbrev gmm-utils
mailheader sendmail rfc2047 rfc2045 ietf-drums mm-util mail-prsvr
mail-utils flyspell ispell imenu tramp tramp-compat tramp-loaddefs
trampver ucs-normalize shell pcomplete comint ansi-color ring parse-time
format-spec advice paren delsel cus-start cus-load finder-inf info
package easymenu epg-config url-handlers url-parse auth-source cl-seq
eieio eieio-core cl-macs eieio-loaddefs password-cache url-vars seq
byte-opt gv bytecomp byte-compile cconv cl-loaddefs cl-lib elec-pair
server time-date mule-util tooltip eldoc electric uniquify ediff-hook
vc-hooks lisp-float-type mwheel dos-w32 ls-lisp disp-table term/w32-win
w32-win w32-vars term/common-win tool-bar dnd fontset image regexp-opt
fringe tabulated-list replace newcomment text-mode elisp-mode lisp-mode
prog-mode register page menu-bar rfn-eshadow isearch timer select
scroll-bar mouse jit-lock font-lock syntax facemenu font-core
term/tty-colors frame cl-generic cham georgian utf-8-lang misc-lang
vietnamese tibetan thai tai-viet lao korean japanese eucjp-ms cp51932
hebrew greek romanian slovak czech european ethiopic indian cyrillic
chinese composite charscript charprop case-table epa-hook jka-cmpr-hook
help simple abbrev obarray minibuffer cl-preloaded nadvice loaddefs
button faces cus-face macroexp files text-properties overlay sha1 md5
base64 format env code-pages mule custom widget hashtable-print-readable
backquote w32notify w32 lcms2 multi-tty make-network-process emacs)

Memory information:
((conses 16 247714 13662)
 (symbols 56 24342 1)
 (miscs 48 90 172)
 (strings 32 43228 1567)
 (string-bytes 1 1184180)
 (vectors 16 40381)
 (vector-slots 8 774164 15144)
 (floats 8 70 337)
 (intervals 56 304 39)
 (buffers 992 13))

bug#30529: 26.0.91; minibuffer errors change standard-output

[Lars Ingebrigtsen]

Francis Wright <francis.j.wright@gmail.com> writes:

> A user-error in the minibuffer can change the value of standard-output
> (to t) within the environment that invoked the minibuffer, whereas I
> think the minibuffer should rebind standard-output locally and not
> change its global value. Here is a simple way to illustrate this
> problem. Evaluate the following:
>
> (defun STANDARD-LISP ()
>   "Run Standard LISP with input via the minibuffer and output via a buffer."
>   (interactive)
>   (switch-to-buffer (get-buffer-create "*Standard LISP*"))
>   (let (value
> 		(standard-output (current-buffer)))
> 	(while t
> 	  (terpri)
> 	  (princ "Eval: ")
> 	  (setq value (read))
> 	  ;; (read) errors change standard-output to *Messages* buffer,
> 	  ;; so...
> 	  ;; (setq standard-output (current-buffer))
> 	  (prin1 value) (terpri)
> 	  (setq value (eval value))
> 	  (terpri)
> 	  (princ "====> ") (princ value) (terpri))))

(This bug report unfortunately got no response at the time.)

This problem is still present in Emacs 28.  I tried following the code
logic for a couple of minutes to see just what it is that's (re)setting
`standard-output' here, but was unable to locate the location.

Anybody know?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#30529: 26.0.91; minibuffer errors change standard-output

[Basil L. Contovounesios]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Francis Wright <francis.j.wright@gmail.com> writes:
>
>> A user-error in the minibuffer can change the value of standard-output
>> (to t) within the environment that invoked the minibuffer, whereas I
>> think the minibuffer should rebind standard-output locally and not
>> change its global value. Here is a simple way to illustrate this
>> problem. Evaluate the following:
>>
>> (defun STANDARD-LISP ()
>>   "Run Standard LISP with input via the minibuffer and output via a buffer."
>>   (interactive)
>>   (switch-to-buffer (get-buffer-create "*Standard LISP*"))
>>   (let (value
>> 		(standard-output (current-buffer)))
>> 	(while t
>> 	  (terpri)
>> 	  (princ "Eval: ")
>> 	  (setq value (read))
>> 	  ;; (read) errors change standard-output to *Messages* buffer,
>> 	  ;; so...
>> 	  ;; (setq standard-output (current-buffer))
>> 	  (prin1 value) (terpri)
>> 	  (setq value (eval value))
>> 	  (terpri)
>> 	  (princ "====> ") (princ value) (terpri))))
>
> (This bug report unfortunately got no response at the time.)
>
> This problem is still present in Emacs 28.  I tried following the code
> logic for a couple of minutes to see just what it is that's (re)setting
> `standard-output' here, but was unable to locate the location.
>
> Anybody know?

Just from a quick grep, could it be recursive_edit_1?

Does error handling invoke a recursive edit e.g. for entering the
debugger?

-- 
Basil

bug#30529: 26.0.91; minibuffer errors change standard-output

[Lars Ingebrigtsen]

"Basil L. Contovounesios" <contovob@tcd.ie> writes:

>> This problem is still present in Emacs 28.  I tried following the code
>> logic for a couple of minutes to see just what it is that's (re)setting
>> `standard-output' here, but was unable to locate the location.
>>
>> Anybody know?
>
> Just from a quick grep, could it be recursive_edit_1?
>
> Does error handling invoke a recursive edit e.g. for entering the
> debugger?

I tried adding some instrumentation to recursive_edit_1, and it does not
seem to be called when the error is messaged (and then the output goes
to the wrong place).

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#30529: 26.0.91; minibuffer errors change standard-output

[Lars Ingebrigtsen]

"Basil L. Contovounesios" <contovob@tcd.ie> writes:

>> This problem is still present in Emacs 28.  I tried following the code
>> logic for a couple of minutes to see just what it is that's (re)setting
>> `standard-output' here, but was unable to locate the location.
>>
>> Anybody know?
>
> Just from a quick grep, could it be recursive_edit_1?

Found the culprit!  cmd_error sets standard_output to t!

The following patch fixes the reported issue (and doesn't lead to any
regressions in the test suite), but this code has been like this
forever, so I'm a bit leery about applying it.

And perhaps the 

  old_level = Vprint_level;
  old_length = Vprint_length;

stuff should also be changed to specbinding?  Or do we want to avoid
specbind here totally for some reason?  The commit for those lines just
says:

    (cmd_error): Bind Vprint_level and Vprint_length.

I mean, we had specbind before 1995, I think?

So...  any comments/insights here?

diff --git a/src/keyboard.c b/src/keyboard.c
index 820229cf8f..f44a2b9164 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -923,6 +923,7 @@ restore_kboard_configuration (int was_locked)
 static Lisp_Object
 cmd_error (Lisp_Object data)
 {
+  ptrdiff_t count = SPECPDL_INDEX ();
   Lisp_Object old_level, old_length;
   Lisp_Object conditions;
   char macroerror[sizeof "After..kbd macro iterations: "
@@ -951,8 +952,8 @@ cmd_error (Lisp_Object data)
       executing_kbd_macro = Qnil;
     }
 
-  Vstandard_output = Qt;
-  Vstandard_input = Qt;
+  specbind (Qstandard_output, Qt);
+  specbind (Qstandard_input, Qt);
   kset_prefix_arg (current_kboard, Qnil);
   kset_last_prefix_arg (current_kboard, Qnil);
   cancel_echoing ();
@@ -969,6 +970,7 @@ cmd_error (Lisp_Object data)
   Vquit_flag = Qnil;
   Vinhibit_quit = Qnil;
 
+  unbind_to (count, Qnil);
   return make_fixnum (0);
 }
 


-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#30529: 26.0.91; minibuffer errors change standard-output

[Andreas Schwab]

On Aug 12 2021, Lars Ingebrigtsen wrote:

> "Basil L. Contovounesios" <contovob@tcd.ie> writes:
>
>>> This problem is still present in Emacs 28.  I tried following the code
>>> logic for a couple of minutes to see just what it is that's (re)setting
>>> `standard-output' here, but was unable to locate the location.
>>>
>>> Anybody know?
>>
>> Just from a quick grep, could it be recursive_edit_1?
>
> Found the culprit!  cmd_error sets standard_output to t!
>
> The following patch fixes the reported issue (and doesn't lead to any
> regressions in the test suite), but this code has been like this
> forever, so I'm a bit leery about applying it.
>
> And perhaps the 
>
>   old_level = Vprint_level;
>   old_length = Vprint_length;
>
> stuff should also be changed to specbinding?  Or do we want to avoid
> specbind here totally for some reason?  The commit for those lines just
> says:
>
>     (cmd_error): Bind Vprint_level and Vprint_length.
>
> I mean, we had specbind before 1995, I think?
>
> So...  any comments/insights here?

I think the intention is to get out of any situation that may result in
repeated errors.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

bug#30529: 26.0.91; minibuffer errors change standard-output

[Lars Ingebrigtsen]

Andreas Schwab <schwab@linux-m68k.org> writes:

> I think the intention is to get out of any situation that may result in
> repeated errors.

Right -- if this is while handling the error itself, then specbinding it
should do the trick...

Eli Zaretskii <eliz@gnu.org> writes:

> I see no problems with the patch.  As we add calls into Lisp in more
> and more places in the internals, we hit code that never imagined it
> could be interrupted, so it didn't bother with unwinding (which
> doesn't come for free).

So I've now pushed the change.  I've done some testing (to see whether I
can make it behave oddly if doing backtraces from an error and the
like), but everything seems to behave as before.

But we should be on the lookout for oddities in this area -- the
function may have been setting standard-output permanently for some
obscure reason...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#30529: 26.0.91; minibuffer errors change standard-output

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Thu, 12 Aug 2021 16:34:20 +0200
> Cc: f.j.wright@live.co.uk, Francis Wright <francis.j.wright@gmail.com>,
>  30529@debbugs.gnu.org
> 
> Found the culprit!  cmd_error sets standard_output to t!
> 
> The following patch fixes the reported issue (and doesn't lead to any
> regressions in the test suite), but this code has been like this
> forever, so I'm a bit leery about applying it.
> 
> And perhaps the 
> 
>   old_level = Vprint_level;
>   old_length = Vprint_length;
> 
> stuff should also be changed to specbinding?  Or do we want to avoid
> specbind here totally for some reason?  The commit for those lines just
> says:
> 
>     (cmd_error): Bind Vprint_level and Vprint_length.
> 
> I mean, we had specbind before 1995, I think?
> 
> So...  any comments/insights here?

I see no problems with the patch.  As we add calls into Lisp in more
and more places in the internals, we hit code that never imagined it
could be interrupted, so it didn't bother with unwinding (which
doesn't come for free).