edebug: regrettable loss of information in Emacs 26.1 when printing a variable's value.

edebug: regrettable loss of information in Emacs 26.1 when printing a variable's value.

[Alan Mackenzie]

Hello, Emacs.

In the upcoming Emacs 26.1, I'm in edebug.  In the program I'm
debugging, there is something nasty in the variable
`syntax-propertize-function', and I need to find out what.

So I do "e" and type in that variable name.

What comes back is this:

    #f(compiled-function (start end) #<bytecode 0x191eb39>)

.  This is thoroughly unhelpful.  In previous versions of Emacs, this
would have printed out the function in enough detail to give variable
names, function names, etc., which could be used to search through the
Emacs source with.

What am I supposed to do with "0x191eb39"?  Can I give that as an
argument to some *Help* function which will give me more details?

I've searched through NEWS for details of this change, in the hope of
finding an option to reverse it, but found nothing.  I searched for
"edebug", then "debug".  Why is there nothing in NEWS about this
somewhat significant change?

So, can I get edebug to print out the contents of a variable containing
a function?  If so how?

I can remember vaguely this topic being discussed on emacs-devel, and
wish I'd paid more attention at the time.

It looks like I might need to go back to Emacs 25.3 to do debugging.
This isn't good.

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: edebug: regrettable loss of information in Emacs 26.1 when printing a variable's value.

[Noam Postavsky]

> So, can I get edebug to print out the contents of a variable containing
> a function?  If so how?

(fset 'edebug-prin1-to-string #'prin1-to-string) ; gives Emacs 25 behaviour

Or

(setq cl-print-compiled 'static) ; shows constants array, but not bytecode

Re: edebug: regrettable loss of information in Emacs 26.1 when printing a variable's value.

[Alan Mackenzie]

Hello, Noam.

On Mon, May 21, 2018 at 13:47:53 -0400, Noam Postavsky wrote:
> > So, can I get edebug to print out the contents of a variable containing
> > a function?  If so how?

> (fset 'edebug-prin1-to-string #'prin1-to-string) ; gives Emacs 25 behaviour

Thanks, this works!

> Or

> (setq cl-print-compiled 'static) ; shows constants array, but not bytecode

I'll need to try that, sometime.

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: edebug: regrettable loss of information in Emacs 26.1 when printing a variable's value.

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@gmail.com>
> Date: Mon, 21 May 2018 13:47:53 -0400
> Cc: Emacs developers <emacs-devel@gnu.org>
> 
> > So, can I get edebug to print out the contents of a variable containing
> > a function?  If so how?
> 
> (fset 'edebug-prin1-to-string #'prin1-to-string) ; gives Emacs 25 behaviour
> 
> Or
> 
> (setq cl-print-compiled 'static) ; shows constants array, but not bytecode

It's IMO not nice to have this customizable on such a low level, let
alone in a different package.  I think we will need Edebug defcustom's
for tailoring this behavior.  (And given the terse output of the
current default in this case, I wonder why we changed the default
behavior from what it was in Emacs 25.)

Maybe it's too late to introduce defcustom's before 26.1 is released,
but in any case, this should be called out in NEWS.  Could you please
add an entry about this?

Thanks.

Re: edebug: regrettable loss of information in Emacs 26.1 when printing a variable's value.

[Noam Postavsky]

On 21 May 2018 at 14:30, Eli Zaretskii <eliz@gnu.org> wrote:
>> From: Noam Postavsky <npostavs@gmail.com>
>> Date: Mon, 21 May 2018 13:47:53 -0400
>> Cc: Emacs developers <emacs-devel@gnu.org>
>>
>> > So, can I get edebug to print out the contents of a variable containing
>> > a function?  If so how?
>>
>> (fset 'edebug-prin1-to-string #'prin1-to-string) ; gives Emacs 25 behaviour

Alternatively (setq cl-print-readably t) has the same (though it
affects non-edebug usage of cl-print as well).

>>
>> Or
>>
>> (setq cl-print-compiled 'static) ; shows constants array, but not bytecode
>
> It's IMO not nice to have this customizable on such a low level, let
> alone in a different package.  I think we will need Edebug defcustom's
> for tailoring this behavior.  (And given the terse output of the
> current default in this case, I wonder why we changed the default
> behavior from what it was in Emacs 25.)

I think it was due to overly verbose output for EIEIO objects (e.g., Bug#25295).

> Maybe it's too late to introduce defcustom's before 26.1 is released,
> but in any case, this should be called out in NEWS.  Could you please
> add an entry about this?

Yup, done.

[1: d65430f6cb]: 2018-05-21 17:11:29 -0400
  * etc/NEWS: Mention change in `edebug-prin1-to-string'.
  https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=d65430f6cb48d009c28cc27c5171f6fc82c79663

Re: edebug: regrettable loss of information in Emacs 26.1 when printing a variable's value.

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@gmail.com>
> Date: Mon, 21 May 2018 17:20:08 -0400
> Cc: Alan Mackenzie <acm@muc.de>, Emacs developers <emacs-devel@gnu.org>
> 
> > Maybe it's too late to introduce defcustom's before 26.1 is released,
> > but in any case, this should be called out in NEWS.  Could you please
> > add an entry about this?
> 
> Yup, done.

Thanks.

Re: edebug: regrettable loss of information in Emacs 26.1 when printing a variable's value.

[Eli Zaretskii]

> Date: Mon, 21 May 2018 17:10:19 +0000
> From: Alan Mackenzie <acm@muc.de>
> 
> In the upcoming Emacs 26.1, I'm in edebug.  In the program I'm
> debugging, there is something nasty in the variable
> `syntax-propertize-function', and I need to find out what.
> 
> So I do "e" and type in that variable name.
> 
> What comes back is this:
> 
>     #f(compiled-function (start end) #<bytecode 0x191eb39>)
> 
> .  This is thoroughly unhelpful.  In previous versions of Emacs, this
> would have printed out the function in enough detail to give variable
> names, function names, etc., which could be used to search through the
> Emacs source with.

Did you try setting cl-print-compiled to the value 'disassemble' or
'static'?

Re: edebug: regrettable loss of information in Emacs 26.1 when printing a variable's value.

[Alan Mackenzie]

Hello, Eli.

On Mon, May 21, 2018 at 20:58:27 +0300, Eli Zaretskii wrote:
> > Date: Mon, 21 May 2018 17:10:19 +0000
> > From: Alan Mackenzie <acm@muc.de>

> > In the upcoming Emacs 26.1, I'm in edebug.  In the program I'm
> > debugging, there is something nasty in the variable
> > `syntax-propertize-function', and I need to find out what.

> > So I do "e" and type in that variable name.

> > What comes back is this:

> >     #f(compiled-function (start end) #<bytecode 0x191eb39>)

> > .  This is thoroughly unhelpful.  In previous versions of Emacs, this
> > would have printed out the function in enough detail to give variable
> > names, function names, etc., which could be used to search through the
> > Emacs source with.

> Did you try setting cl-print-compiled to the value 'disassemble' or
> 'static'?

Er, no.  :-)  I'm trying to simulate an ordinary user at the moment.  Now
that I know what to look for, I found an entry in NEWS.

[ Comments about this entry withheld, after seeing you asked Noam for
another entry in NEWS. ]

Thanks for acting on this so rapidly.

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: edebug: regrettable loss of information in Emacs 26.1 when printing a variable's value.

[Stefan Monnier]

> What comes back is this:
>
>     #f(compiled-function (start end) #<bytecode 0x191eb39>)

In an ideal world, you should be able to click on the "#<bytecode
 0x191eb39>" and have it displayed in disassembled form.  This already
works in several contexts, and we should work to make it work in
more cases.


        Stefan

Re: edebug: regrettable loss of information in Emacs 26.1 when printing a variable's value.

[Noam Postavsky]

On 21 May 2018 at 14:05, Stefan Monnier <monnier@iro.umontreal.ca> wrote:
>> What comes back is this:
>>
>>     #f(compiled-function (start end) #<bytecode 0x191eb39>)
>
> In an ideal world, you should be able to click on the "#<bytecode
>  0x191eb39>" and have it displayed in disassembled form.  This already
> works in several contexts, and we should work to make it work in
> more cases.

The output in the minibuffer looks clickable (i.e., underlined and
blue), but it isn't, not sure why. The button property doesn't seem to
make it into *Messages* at all, we would probably need some special
casing for that.

Re: edebug: regrettable loss of information in Emacs 26.1 when printing a variable's value.

[Stefan Monnier]

> The output in the minibuffer looks clickable (i.e., underlined and
> blue), but it isn't, not sure why.

The problem is that as soon as you try to click, the *Echo area* buffer
(which contains the message) is replaced with the *Minibuf* buffer.

I don't know why we have this weird system of constant buffer-switching
in the mini-window, but I'd be surprised if it's easy to change it.


        Stefan

Re: edebug: regrettable loss of information in Emacs 26.1 when printing a variable's value.

[Stefan Monnier]

>> The output in the minibuffer looks clickable (i.e., underlined and
>> blue), but it isn't, not sure why.
> The problem is that as soon as you try to click, the *Echo area* buffer
> (which contains the message) is replaced with the *Minibuf* buffer.

Actually, no.  Not yet sure where is the problem, but maybe it's linked
to the fact that before running the command associated
with this "click", we clear the echo area (tho it's doesn't seem to be
just that either).


        Stefan

Message's text-properties in *Messages* (was: edebug: regrettable loss of information in Emacs 26.1 when printing a variable's value)

[Stefan Monnier]

> blue), but it isn't, not sure why. The button property doesn't seem to
> make it into *Messages* at all, we would probably need some special
> casing for that.

Indeed, text-properties aren't copied to the *Messages* buffer.
I think this should be fixed.

I'm playing with the work-in-progress below for that, but I have
a question about message_dolog.  Its comment says:

    /* Add a string M of length NBYTES to the message log, optionally
       terminated with a newline when NLFLAG is true.  MULTIBYTE, if
       true, means interpret the contents of M as multibyte.  This
       function calls low-level routines in order to bypass text property
       hooks, etc. which might not be safe to run.
    
       This may GC (insert may run before/after change hooks),
       so the buffer M must NOT point to a Lisp string.  */

I find this somewhat confusing:
- Not sure which text property hooks it's referring to.
- It first says "... hooks, etc. which might not be safe to run" but later
  "(insert may run before/after change hooks)".
  Aren't before/after hooks just as dangerous as others?
- The code actually doesn't seem to run before/after change hooks.
- But the code does call `messages-buffer-mode` (when (re)creating the
  buffer), so it does run potentially arbitrary Lisp code.
- Why would arbitrary Lisp code be dangerous (I understand that message_dolog
  can be called from within redisplay, but redisplay runs Elisp code
  at several places, so "from redidplay" doesn't inherently imply you
  can't run Elisp code).

The work-in-progress patch below adds a new message_dolog_lisp which
uses higher-level operations which preserve text-properties.  It tries
to avoid running hooks by temporarily binding inhibit-<foo>-hooks.

To a large extent my question is whether those inhibit-<foo>-hooks would
make it safe enough to replace message_dolog with message_dolog_lisp
(aka rather reimplement message_dolog on top of message_dolog_lisp), or
whether we really need to keep the "safer" message_dolog (in which case
I'll have to work harder at sharing more code between the two).


        Stefan


diff --git a/src/lisp.h b/src/lisp.h
index 23e3989c27..ed163fa86c 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -3737,6 +3737,7 @@ extern void message1_nolog (const char *);
 extern void message3 (Lisp_Object);
 extern void message3_nolog (Lisp_Object);
 extern void message_dolog (const char *, ptrdiff_t, bool, bool);
+extern void message_dolog_lisp (Lisp_Object s, bool nlflag);
 extern void message_with_string (const char *, Lisp_Object, bool);
 extern void message_log_maybe_newline (void);
 extern void update_echo_area (void);
diff --git a/src/xdisp.c b/src/xdisp.c
index f2f4392493..2af0bbc02f 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -10334,6 +10334,21 @@ message_log_maybe_newline (void)
     message_dolog ("", 0, true, false);
 }
 
+static void message_postprocess_log (void);
+
+static void message_set_log_buffer (void)
+{
+  /* Ensure the Messages buffer exists, and switch to it.
+     If we created it, set the major-mode.  */
+  bool newbuffer = NILP (Fget_buffer (Vmessages_buffer_name));
+  Fset_buffer (Fget_buffer_create (Vmessages_buffer_name));
+  if (newbuffer
+      && !NILP (Ffboundp (intern ("messages-buffer-mode"))))
+    call0 (intern ("messages-buffer-mode"));
+
+  bset_undo_list (current_buffer, Qt);
+  bset_cache_long_scans (current_buffer, Qnil);
+}
 
 /* Add a string M of length NBYTES to the message log, optionally
    terminated with a newline when NLFLAG is true.  MULTIBYTE, if
@@ -10355,40 +10370,21 @@ message_dolog (const char *m, ptrdiff_t nbytes, bool nlflag, bool multibyte)
   if (!NILP (Vmessage_log_max))
     {
       struct buffer *oldbuf;
-      Lisp_Object oldpoint, oldbegv, oldzv;
+      Lisp_Object oldpoint, oldzv;
       int old_windows_or_buffers_changed = windows_or_buffers_changed;
-      ptrdiff_t point_at_end = 0;
-      ptrdiff_t zv_at_end = 0;
-      Lisp_Object old_deactivate_mark;
-
-      old_deactivate_mark = Vdeactivate_mark;
+      Lisp_Object old_deactivate_mark = Vdeactivate_mark;
       oldbuf = current_buffer;
 
-      /* Ensure the Messages buffer exists, and switch to it.
-         If we created it, set the major-mode.  */
-      bool newbuffer = NILP (Fget_buffer (Vmessages_buffer_name));
-      Fset_buffer (Fget_buffer_create (Vmessages_buffer_name));
-      if (newbuffer
-	  && !NILP (Ffboundp (intern ("messages-buffer-mode"))))
-	call0 (intern ("messages-buffer-mode"));
-
-      bset_undo_list (current_buffer, Qt);
-      bset_cache_long_scans (current_buffer, Qnil);
+      message_set_log_buffer ();
 
       oldpoint = message_dolog_marker1;
       set_marker_restricted_both (oldpoint, Qnil, PT, PT_BYTE);
-      oldbegv = message_dolog_marker2;
-      set_marker_restricted_both (oldbegv, Qnil, BEGV, BEGV_BYTE);
       oldzv = message_dolog_marker3;
       set_marker_restricted_both (oldzv, Qnil, ZV, ZV_BYTE);
 
-      if (PT == Z)
-	point_at_end = 1;
-      if (ZV == Z)
-	zv_at_end = 1;
+      bool point_at_end = (PT == Z);
+      bool zv_at_end = (ZV == Z);
 
-      BEGV = BEG;
-      BEGV_BYTE = BEG_BYTE;
       ZV = Z;
       ZV_BYTE = Z_BYTE;
       TEMP_SET_PT_BOTH (Z, Z_BYTE);
@@ -10432,58 +10428,7 @@ message_dolog (const char *m, ptrdiff_t nbytes, bool nlflag, bool multibyte)
 		       true, false, false);
 
       if (nlflag)
-	{
-	  ptrdiff_t this_bol, this_bol_byte, prev_bol, prev_bol_byte;
-	  printmax_t dups;
-
-	  insert_1_both ("\n", 1, 1, true, false, false);
-
-	  scan_newline (Z, Z_BYTE, BEG, BEG_BYTE, -2, false);
-	  this_bol = PT;
-	  this_bol_byte = PT_BYTE;
-
-	  /* See if this line duplicates the previous one.
-	     If so, combine duplicates.  */
-	  if (this_bol > BEG)
-	    {
-	      scan_newline (PT, PT_BYTE, BEG, BEG_BYTE, -2, false);
-	      prev_bol = PT;
-	      prev_bol_byte = PT_BYTE;
-
-	      dups = message_log_check_duplicate (prev_bol_byte,
-                                                  this_bol_byte);
-	      if (dups)
-		{
-		  del_range_both (prev_bol, prev_bol_byte,
-				  this_bol, this_bol_byte, false);
-		  if (dups > 1)
-		    {
-		      char dupstr[sizeof " [ times]"
-				  + INT_STRLEN_BOUND (printmax_t)];
-
-		      /* If you change this format, don't forget to also
-			 change message_log_check_duplicate.  */
-		      int duplen = sprintf (dupstr, " [%"pMd" times]", dups);
-		      TEMP_SET_PT_BOTH (Z - 1, Z_BYTE - 1);
-		      insert_1_both (dupstr, duplen, duplen,
-				     true, false, true);
-		    }
-		}
-	    }
-
-	  /* If we have more than the desired maximum number of lines
-	     in the *Messages* buffer now, delete the oldest ones.
-	     This is safe because we don't have undo in this buffer.  */
-
-	  if (NATNUMP (Vmessage_log_max))
-	    {
-	      scan_newline (Z, Z_BYTE, BEG, BEG_BYTE,
-			    -XFASTINT (Vmessage_log_max) - 1, false);
-	      del_range_both (BEG, BEG_BYTE, PT, PT_BYTE, false);
-	    }
-	}
-      BEGV = marker_position (oldbegv);
-      BEGV_BYTE = marker_byte_position (oldbegv);
+        message_postprocess_log ();
 
       if (zv_at_end)
 	{
@@ -10505,7 +10450,6 @@ message_dolog (const char *m, ptrdiff_t nbytes, bool nlflag, bool multibyte)
 			  marker_byte_position (oldpoint));
 
       unchain_marker (XMARKER (oldpoint));
-      unchain_marker (XMARKER (oldbegv));
       unchain_marker (XMARKER (oldzv));
 
       /* We called insert_1_both above with its 5th argument (PREPARE)
@@ -10524,6 +10468,129 @@ message_dolog (const char *m, ptrdiff_t nbytes, bool nlflag, bool multibyte)
     }
 }
 
+static void
+message_postprocess_log (void)
+{
+  ptrdiff_t this_bol, this_bol_byte, prev_bol, prev_bol_byte;
+  printmax_t dups;
+
+  Lisp_Object oldbegv = message_dolog_marker2;
+  set_marker_restricted_both (oldbegv, Qnil, BEGV, BEGV_BYTE);
+  BEGV = BEG;
+  BEGV_BYTE = BEG_BYTE;
+
+  insert_1_both ("\n", 1, 1, true, false, false);
+
+  scan_newline (Z, Z_BYTE, BEG, BEG_BYTE, -2, false);
+  this_bol = PT;
+  this_bol_byte = PT_BYTE;
+
+  /* See if this line duplicates the previous one.
+	     If so, combine duplicates.  */
+  if (this_bol > BEG)
+    {
+      scan_newline (PT, PT_BYTE, BEG, BEG_BYTE, -2, false);
+      prev_bol = PT;
+      prev_bol_byte = PT_BYTE;
+
+      dups = message_log_check_duplicate (prev_bol_byte,
+                                          this_bol_byte);
+      if (dups)
+	{
+	  del_range_both (prev_bol, prev_bol_byte,
+			  this_bol, this_bol_byte, false);
+	  if (dups > 1)
+	    {
+	      char dupstr[sizeof " [ times]"
+			  + INT_STRLEN_BOUND (printmax_t)];
+
+	      /* If you change this format, don't forget to also
+			 change message_log_check_duplicate.  */
+	      int duplen = sprintf (dupstr, " [%"pMd" times]", dups);
+	      TEMP_SET_PT_BOTH (Z - 1, Z_BYTE - 1);
+	      insert_1_both (dupstr, duplen, duplen,
+			     true, false, true);
+	    }
+	}
+    }
+
+  /* If we have more than the desired maximum number of lines
+	     in the *Messages* buffer now, delete the oldest ones.
+	     This is safe because we don't have undo in this buffer.  */
+
+  if (NATNUMP (Vmessage_log_max))
+    {
+      scan_newline (Z, Z_BYTE, BEG, BEG_BYTE,
+		    -XFASTINT (Vmessage_log_max) - 1, false);
+      del_range_both (BEG, BEG_BYTE, PT, PT_BYTE, false);
+    }
+
+  BEGV = marker_position (oldbegv);
+  BEGV_BYTE = marker_byte_position (oldbegv);
+
+  unchain_marker (XMARKER (oldbegv));
+}
+
+/* Add a string S to the message log, optionally
+   terminated with a newline when NLFLAG is true.  */
+void message_dolog_lisp (Lisp_Object s, bool nlflag)
+{
+  if (!NILP (Vmemory_full) || NILP (Vmessage_log_max))
+    return;
+
+  struct buffer *oldbuf = current_buffer;
+  ptrdiff_t count = SPECPDL_INDEX ();
+  Lisp_Object old_deactivate_mark = Vdeactivate_mark;
+
+  message_set_log_buffer ();
+  specbind (Qinhibit_modification_hooks, Qt);
+  specbind (Qinhibit_read_only, Qt);
+  specbind (Qinhibit_point_motion_hooks, Qt);
+
+  Lisp_Object oldpoint = message_dolog_marker1;
+  set_marker_restricted_both (oldpoint, Qnil, PT, PT_BYTE);
+  Lisp_Object oldzv = message_dolog_marker3;
+  set_marker_restricted_both (oldzv, Qnil, ZV, ZV_BYTE);
+
+  bool point_at_end = PT == Z;
+  bool zv_at_end = (ZV == Z);
+
+  ZV = Z;
+  ZV_BYTE = Z_BYTE;
+  TEMP_SET_PT_BOTH (ZV, ZV_BYTE);
+  insert_from_string (s, 0, 0, SCHARS (s), SBYTES (s), true);
+
+  if (nlflag)
+    message_postprocess_log ();
+
+  if (zv_at_end)
+    {
+      ZV = Z;
+      ZV_BYTE = Z_BYTE;
+    }
+  else
+    {
+      ZV = marker_position (oldzv);
+      ZV_BYTE = marker_byte_position (oldzv);
+    }
+
+  if (point_at_end)
+    TEMP_SET_PT_BOTH (ZV, ZV_BYTE);
+  else
+    /* We can't do Fgoto_char (oldpoint) because it will run some
+       Lisp code.  */
+    TEMP_SET_PT_BOTH (marker_position (oldpoint),
+		      marker_byte_position (oldpoint));
+
+  unchain_marker (XMARKER (oldpoint));
+  unchain_marker (XMARKER (oldzv));
+
+  set_buffer_internal (oldbuf);
+  message_log_need_newline = !nlflag;
+  Vdeactivate_mark = old_deactivate_mark;
+  unbind_to (count, Qnil);
+}
+
 
 /* We are at the end of the buffer after just having inserted a newline.
    (Note: We depend on the fact we won't be crossing the gap.)
@@ -10577,15 +10644,7 @@ message3 (Lisp_Object m)
   /* First flush out any partial line written with print.  */
   message_log_maybe_newline ();
   if (STRINGP (m))
-    {
-      ptrdiff_t nbytes = SBYTES (m);
-      bool multibyte = STRING_MULTIBYTE (m);
-      char *buffer;
-      USE_SAFE_ALLOCA;
-      SAFE_ALLOCA_STRING (buffer, m);
-      message_dolog (buffer, nbytes, true, multibyte);
-      SAFE_FREE ();
-    }
+    message_dolog_lisp (m, true);
   if (! inhibit_message)
     message3_nolog (m);
 }
@@ -32567,6 +32626,10 @@ They are still logged to the *Messages* buffer.  */);
   staticpro (&message_dolog_marker2);
   message_dolog_marker3 = Fmake_marker ();
   staticpro (&message_dolog_marker3);
+  /* marker1 and marker3 are used for PT and ZV respectively and insertion
+     should "push them down".  */
+  XMARKER (message_dolog_marker1)->insertion_type = true;
+  XMARKER (message_dolog_marker3)->insertion_type = true;
 
   defsubr (&Sset_buffer_redisplay);
 #ifdef GLYPH_DEBUG

RE: Message's text-properties in *Messages* (was: edebug: regrettable loss of information in Emacs 26.1 when printing a variable's value)

[Drew Adams]

> Indeed, text-properties aren't copied to the *Messages* buffer.
> I think this should be fixed.

OK, but please add a user option for whether it is the case,
i.e., to inhibit copying them there.

Re: Message's text-properties in *Messages*

[Stefan Monnier]

> OK, but please add a user option for whether it is the case,
> i.e., to inhibit copying them there.

Just because Drew asked for it?  Not a sufficient reason, no.


        Stefan

Re: Message's text-properties in *Messages*

[John Wiegley]

>>>>> "SM" == Stefan Monnier <monnier@iro.umontreal.ca> writes:

SM> Just because Drew asked for it? Not a sufficient reason, no.

How about instead of phrasing that way, we ask if others would also like to
see this as an option, and if no responds, then we can thank Drew for his
thoughtful suggestion.

-- 
John Wiegley                  GPG fingerprint = 4710 CF98 AF9B 327B B80F
http://newartisans.com                          60E1 46C4 BD1A 7AC1 4BA2

Re: Message's text-properties in *Messages*

[Stefan Monnier]

> SM> Just because Drew asked for it? Not a sufficient reason, no.
> How about instead of phrasing that way, we ask if others would also like to
> see this as an option, and if no responds, then we can thank Drew for his
> thoughtful suggestion.

I reality, I should just ignore those messages from Drew,


        Stefan

Re: Message's text-properties in *Messages*

[Amin Bandali]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

> Just because Drew asked for it?  Not a sufficient reason, no.
> [...]
> I reality, I should just ignore those messages from Drew,

I'm a relatively recent subscriber to emacs-devel, so please
excuse my being out of the loop; but do you care to elaborate
what's the issue with asking, nicely, for a user-facing option?
Of course ideally one would contribute a patch, but I can't see
how that request warrants this tone of response.

-amin

Re: Message's text-properties in *Messages*

[John Wiegley]

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

>>>>> "AB" == Amin Bandali <amin@aminb.org> writes:

AB> I'm a relatively recent subscriber to emacs-devel, so please excuse my
AB> being out of the loop; but do you care to elaborate what's the issue with
AB> asking, nicely, for a user-facing option? Of course ideally one would
AB> contribute a patch, but I can't see how that request warrants this tone of
AB> response.

I appreciate the question, Amin. Everyone should feel welcome to express
whatever thoughts he or she may have as to what could benefit Emacs, with the
understanding that the maintainers and contributors are free to disregard any
suggestion they feel is either unwarranted, or not worth adding to the
complexity of Emacs.

I hope everyone will respect the well-meaning intention behind suggestions
such as the one Drew has made, independent of technical content. I understand
how easily frustrations can arise, so I challenge everyone to do what they can
to keep the well-being of our small community foremost in mind when replying.

Thank you,
--
John Wiegley                  GPG fingerprint = 4710 CF98 AF9B 327B B80F
http://newartisans.com                          60E1 46C4 BD1A 7AC1 4BA2

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 658 bytes --]

Re: Message's text-properties in *Messages*

[Amin Bandali]

John Wiegley <johnw@gnu.org> writes:

> I appreciate the question, Amin. Everyone should feel welcome to express
> whatever thoughts he or she may have as to what could benefit Emacs, with the
> understanding that the maintainers and contributors are free to disregard any
> suggestion they feel is either unwarranted, or not worth adding to the
> complexity of Emacs.
>
> I hope everyone will respect the well-meaning intention behind suggestions
> such as the one Drew has made, independent of technical content. I understand
> how easily frustrations can arise, so I challenge everyone to do what they can
> to keep the well-being of our small community foremost in mind when replying.
>
> Thank you,

Thank you for the reply, John.  That's reassuring to hear.
Indeed it doesn't seem like anyone had any malintentions, but
perhaps slight frustrations :)

My apologies again for jumping in middle of a thread without
having the context, and thank you all for all your hard work.
Looking forward to being a part of the community.

Best,

-amin

Re: Message's text-properties in *Messages* (was: edebug: regrettable loss of information in Emacs 26.1 when printing a variable's value)

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Wed, 23 May 2018 12:03:45 -0400
> 
> - Why would arbitrary Lisp code be dangerous (I understand that message_dolog
>   can be called from within redisplay, but redisplay runs Elisp code
>   at several places, so "from redidplay" doesn't inherently imply you
>   can't run Elisp code).

When Lisp code is called from redisplay, we always do that via
safe_call.  Anything else is not safe, AFAIR.

Re: Message's text-properties in *Messages*

[Stefan Monnier]

>> - Why would arbitrary Lisp code be dangerous (I understand that message_dolog
>>   can be called from within redisplay, but redisplay runs Elisp code
>>   at several places, so "from redisplay" doesn't inherently imply you
>>   can't run Elisp code).
> When Lisp code is called from redisplay, we always do that via
> safe_call.  Anything else is not safe, AFAIR.

I see.  This doesn't answer all the questions, tho:

- The current code still does

    call0 (intern ("messages-buffer-mode"));

  hence doesn't use safe_call here.  This is likely a bug (introduced
  when we added messages-buffer-mode) and it hasn't bitten us simply
  because it's run rarely enough (only after you kill the *Messages*
  buffer) and by default it probably doesn't do anything dangerous, so
  to trigger this bug, we'd need a "perfect storm" (i.e. set
  a nasty messages-buffer-mode-hook, then kill *Messages*, then arrange
  for the next message to come from within the redisplay code).

- AFAICT message_dolog avoids running before/after change functions, so
  the comment about the GC being sometimes called because we run
  before/after change hooks seems odd.

- If after/before change functions do end up called, are they really
  called via safe_call?  I don't see any trace of that.

- Would let-binding inhibit-foo-hooks good enough to avoid running
  Elisp code unsafely?


        Stefan

Re: Message's text-properties in *Messages*

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Wed, 23 May 2018 13:41:36 -0400
> 
> >> - Why would arbitrary Lisp code be dangerous (I understand that message_dolog
> >>   can be called from within redisplay, but redisplay runs Elisp code
> >>   at several places, so "from redisplay" doesn't inherently imply you
> >>   can't run Elisp code).
> > When Lisp code is called from redisplay, we always do that via
> > safe_call.  Anything else is not safe, AFAIR.
> 
> I see.  This doesn't answer all the questions, tho:

I didn't mean to answer all of them yet, I will need some time to look
at the code.  However, one other thing is pretty much clear:

> - The current code still does
> 
>     call0 (intern ("messages-buffer-mode"));
> 
>   hence doesn't use safe_call here.

This is called in message_dolog, which doesn't enter redisplay.
Redisplay is triggered in message3_nolog.  So maybe we are mixing two
different issues here, and that comment is not about redisplay at all.
If I'm right, there's no issue with calling Lisp from message_dolog
per se.

Re: Message's text-properties in *Messages*

[Eli Zaretskii]

> Date: Wed, 23 May 2018 21:07:37 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: emacs-devel@gnu.org
> 
> > > When Lisp code is called from redisplay, we always do that via
> > > safe_call.  Anything else is not safe, AFAIR.
> > 
> > I see.  This doesn't answer all the questions, tho:
> 
> I didn't mean to answer all of them yet, I will need some time to look
> at the code.

Sorry for the long delay in responding.

> I'm playing with the work-in-progress below for that, but I have
> a question about message_dolog.  Its comment says:
> 
>     /* Add a string M of length NBYTES to the message log, optionally
>        terminated with a newline when NLFLAG is true.  MULTIBYTE, if
>        true, means interpret the contents of M as multibyte.  This
>        function calls low-level routines in order to bypass text property
>        hooks, etc. which might not be safe to run.
>     
>        This may GC (insert may run before/after change hooks),
>        so the buffer M must NOT point to a Lisp string.  */

My first comment is that the first paragraph of this is very old, it
predates the complete rewrite of message_dolog during development of
Emacs 21.  So it is quite possible that parts of it could no longer be
relevant.  But see below.

> I find this somewhat confusing:
> - Not sure which text property hooks it's referring to.

My conclusion from looking at the code and its history is that the
comment refers to before-change-functions and maybe to the rest of
stuff done by signal_before_change.

> - It first says "... hooks, etc. which might not be safe to run" but later
>   "(insert may run before/after change hooks)".
>   Aren't before/after hooks just as dangerous as others?

This part tries to explain why the code avoids calling 'insert': it
does that precisely so as to avoid triggering before-change-functions,
the "text property hooks" being talked about here.  IOW, the part in
the parentheses wants to explain why message_dolog does NOT call
'insert', and instead uses the lower-level insert_1_both.

> - The code actually doesn't seem to run before/after change hooks.

It explicitly avoids running before-change-functions, by calling
functions that won't, yes.  But it does call after-change-functions,
if del_range_both is called by message_dolog.  Given my conclusion
(below), I think I understand why only the before-change-functions are
feared of.

> - But the code does call `messages-buffer-mode` (when (re)creating the
>   buffer), so it does run potentially arbitrary Lisp code.

The addition of messages-buffer-mode is relatively new, we have it
only since Sep 2013.  If that mode runs arbitrary Lisp, I think it
indeed could potentially cause trouble, and we are just lucky that we
didn't see it yet.

> - Why would arbitrary Lisp code be dangerous (I understand that message_dolog
>   can be called from within redisplay, but redisplay runs Elisp code
>   at several places, so "from redisplay" doesn't inherently imply you
>   can't run Elisp code).

As I wrote previously, redisplay is not the issue here, as
message_dolog doesn't call any redisplay entry points.

> To a large extent my question is whether those inhibit-<foo>-hooks would
> make it safe enough to replace message_dolog with message_dolog_lisp
> (aka rather reimplement message_dolog on top of message_dolog_lisp), or
> whether we really need to keep the "safer" message_dolog (in which case
> I'll have to work harder at sharing more code between the two).

The problem as I see it is actually explained in another place, in the
commentary to 'insert':

   DO NOT use this for the contents of a Lisp string or a Lisp buffer!
   prepare_to_modify_buffer could relocate the text.  */

This is the reason why message_dolog avoids calling 'insert': it wants
to support the use case where its first argument is a pointer to some
buffer's text or to contents of a Lisp string (as opposed to a C
string).  'insert' calls prepare_to_modify_buffer, which could cause
GC, which could relocate buffer or string text, and thus invalidate
the C pointer passed to message_dolog.  I see at least one call of
message_dolog, via strout, which passes a pointer to contents of a
Lisp string, so we still have such uses, and I'm not sure we want to
restrict message_dolog to not support such cases (I'm not sure we can
do that, even if we tried).

I think the same problem could happen as result of calling
messages-buffer-mode, if indeed it calls arbitrary Lisp and causes GC.

By contrast, after-change-functions are called when the text to add to
the log is no longer need, so we no longer care about GC and potential
relocations.

Coming back to your patch, my comments are:

  . the new function message_dolog_lisp is safe because it accepts a
    Lisp string, not a C 'char *' pointer
  . however, the invocation of messages-buffer-mode in message_dolog
    is dangerous, and I think we want to prevent any potential
    breakage due to GC there (but this is not caused by your current
    patch)
  . therefore, maybe we should move that call to messages-buffer-mode
    to the new function(?)

HTH

Re: Message's text-properties in *Messages*

[Stefan Monnier]

>> I didn't mean to answer all of them yet, I will need some time to look
>> at the code.
> Sorry for the long delay in responding.

Come on, it has barely been a week.

>> - Why would arbitrary Lisp code be dangerous (I understand that message_dolog
>>   can be called from within redisplay, but redisplay runs Elisp code
>>   at several places, so "from redisplay" doesn't inherently imply you
>>   can't run Elisp code).
> As I wrote previously, redisplay is not the issue here, as
> message_dolog doesn't call any redisplay entry points.

Hmm... indeed I'm not worried about message_dolog calling redisplay,
instead I think the worry is about redisplay calling message_dolog
(probably via `message` or some variant thereof).

>   . the new function message_dolog_lisp is safe because it accepts a
>     Lisp string, not a C 'char *' pointer

If there's no risk of message_dolog(_lisp) being called *from*
redisplay, then indeed message_dolog_lisp should be perfectly safe and
could even skip the inhibit-*-hooks dance (tho we might want to be
careful about those hooks calling `message` recursively).

And in that case, we could also replace message_dolog with
message_dolog_lisp (if needed, building a fresh new Lisp string from
a char*).


        Stefan

Re: Message's text-properties in *Messages*

[Eli Zaretskii]

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Cc: emacs-devel@gnu.org
> Date: Wed, 30 May 2018 15:44:08 -0400
> 
> > As I wrote previously, redisplay is not the issue here, as
> > message_dolog doesn't call any redisplay entry points.
> 
> Hmm... indeed I'm not worried about message_dolog calling redisplay,
> instead I think the worry is about redisplay calling message_dolog
> (probably via `message` or some variant thereof).

I don't think this happens, and I don't think it could work for
redisplay to call 'message' or its ilk.  Do you see any such calls?

> And in that case, we could also replace message_dolog with
> message_dolog_lisp (if needed, building a fresh new Lisp string from
> a char*).

Probably, although it could be cumbersome with some of the current
callers.

Re: Message's text-properties in *Messages*

[Stefan Monnier]

>> > As I wrote previously, redisplay is not the issue here, as
>> > message_dolog doesn't call any redisplay entry points.
>> Hmm... indeed I'm not worried about message_dolog calling redisplay,
>> instead I think the worry is about redisplay calling message_dolog
>> (probably via `message` or some variant thereof).
>
> I don't think this happens, and I don't think it could work for
> redisplay to call 'message' or its ilk.  Do you see any such calls?

I haven't checked: I just assumed it was part of the worry.

BTW now I see that message_dolog indeed runs the after-change-functions
as well, so it already runs arbitrary Lisp code (and if its
after-change-functions call `message` you get funny results).

>> And in that case, we could also replace message_dolog with
>> message_dolog_lisp (if needed, building a fresh new Lisp string from
>> a char*).
> Probably, although it could be cumbersome with some of the current
> callers.

I'll see how it comes out,


        Stefan

Re: Message's text-properties in *Messages*

[Eli Zaretskii]

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Cc: emacs-devel@gnu.org
> Date: Wed, 30 May 2018 22:09:50 -0400
> 
> BTW now I see that message_dolog indeed runs the after-change-functions
> as well, so it already runs arbitrary Lisp code (and if its
> after-change-functions call `message` you get funny results).

That's Emacs giving you enough rope to hang yourself.  The important
part is that by the time these hooks run, we are done using the C
pointer to the message, so we no longer care about any GC effects.