On Tue, Nov 14, 2017 at 5:01 PM, Aidan Kehoe <kehoea@parhasard.net> wrote:

 Ar an triú lá déag de mí na Samhain, scríobh Raymond Toy:

 > Run xemacs -vanilla and using the menu, select
 > Options->Customize->Emacs->Emacs.  This crashes xemacs because of default
 > widget set (no athena installed).  gdb backtrace appended below.

Thanks, Raymond!

I can’t reproduce, probably because I do have the Athena widgets installed and
when I try to lead the configure script away from them it picks up Motif, but
it’s clear enough that we shouldn’t crash when printing a recursive specifier.

​I tested this fix on the machine where this crash was done.  No more crash!  And the customize buffer shows up as expected.

However, I do get warnings like this:

(3) (specifier/warning) error: (invalid-argument (Image instantiator format is invalid in this locale. [button :descriptor Set :face gui-button-face :callback-ex (lambda (image-instance event) (funcall #<compiled-function (from "/home/rtoy/src/XEmacs/beta/build/lisp/gui.elc") (instance action user-data) "...(20)" [user-data action instance domain image-instance-domain windowp window-buffer] 2 0x983> image-instance (quote widget-gui-action) (quote (push-button :to #<marker at 193 in *Customize Group: Emacs* 0x22cf> :from #<marker at 192 in *Customize Group: Emacs* insertion-type=t 0x22ce> :button-extent #<extent 0x98> :glyph-instantiator [button :descriptor Set :face gui-button-face :callback-ex #2] :glyph-up #<glyph (buffer) #<image-specifier global=(((x) . #1) ((tty) . #1) ((stream) . #1)) fallback=((nil . [nothing])) 0x749>0x159> :action #<compiled-function (from "/home/rtoy/src/XEmacs/beta/build/lisp/cus-edit.elc") (widget &optional event) "...(3)" [Custom-set] 1 0xdb0> :help-echo Make your editing in this buffer take effect for this session :tag Set))))]))

Backtrace follows:

nil



APPROVE COMMIT

NOTE: This patch has been committed

# HG changeset patch
# User Aidan Kehoe <kehoea@parhasard.net>
# Date 1510706657 0
#      Wed Nov 15 00:44:17 2017 +0000
# Node ID 1a2f697b59a7e7443133636304f4dd62c5303120
# Parent  00726cf7425524d7ad434590c8594ae05ee4eda4
Don't blow the stack when printing recursive spec-lists, print_specifier()

src/ChangeLog addition:

2017-11-15  Aidan Kehoe  <kehoea@parhasard.net>

        * specifier.c (print_specifier):
        Cons less when printing the spec-list of a specifier, avoiding
        stack overflow with recursive specifiers due to the COPY_TREE_P
        argument passed by #'specifier-specs to
        specifier_get_external_inst_list().
        Thank you for the bug report, Raymond Toy!

diff -r 00726cf74255 -r 1a2f697b59a7 src/ChangeLog
--- a/src/ChangeLog     Tue Nov 14 22:13:17 2017 +0000
+++ b/src/ChangeLog     Wed Nov 15 00:44:17 2017 +0000
@@ -1,3 +1,12 @@
+2017-11-15  Aidan Kehoe  <kehoea@parhasard.net>
+
+       * specifier.c (print_specifier):
+       Cons less when printing the spec-list of a specifier, avoiding
+       stack overflow with recursive specifiers due to the COPY_TREE_P
+       argument passed by #'specifier-specs to
+       specifier_get_external_inst_list().
+       Thank you for the bug report, Raymond Toy!
+
 2017-11-13  Aidan Kehoe  <kehoea@parhasard.net>

        * bytecode.c (bytecode_arithcompare):
diff -r 00726cf74255 -r 1a2f697b59a7 src/specifier.c
--- a/src/specifier.c   Tue Nov 14 22:13:17 2017 +0000
+++ b/src/specifier.c   Wed Nov 15 00:44:17 2017 +0000
@@ -269,12 +269,18 @@
     }
 }

+static Lisp_Object specifier_get_external_inst_list (Lisp_Object specifier,
+                                                     Lisp_Object locale,
+                                                     enum spec_locale_type,
+                                                     Lisp_Object tag_set,
+                                                     int exact_p,
+                                                     int short_p,
+                                                     int copy_tree_p);
+
 static void
-print_specifier (Lisp_Object obj, Lisp_Object printcharfun,
-                int UNUSED (escapeflag))
+print_specifier (Lisp_Object obj, Lisp_Object printcharfun, int escapeflag)
 {
   Lisp_Specifier *sp = XSPECIFIER (obj);
-  int count = specpdl_depth ();
   Lisp_Object the_specs;

   if (print_readably)
@@ -289,17 +295,36 @@
   specbind (Qprint_string_length, make_fixnum (100));
   specbind (Qprint_length, make_fixnum (5));
 #endif
-  the_specs = Fspecifier_specs (obj, Qglobal, Qnil, Qnil);
+
+  /* This is preferable to calling Fspecifier_specs() because we can specify
+     COPY_TREE_P as zero, avoiding copying possibly-recursive structures. See
+     Raymond Toye's bug report of
+     https://mid.xemacs.org/CAG14z1F7b90rt22P-XXjk+0qT42DrS6E=OAcUSekqoPsQnD9aw@mail.gmail.com
+
+     specifier_get_external_inst_list() does cons, even with a zero
+     COPY_TREE_P, but that's OK, in exchange for that we get the short form
+     when appropriate, and the instance list has been checked for circularity
+     when it was specified initially, we won't blow the stack with it. */
+  the_specs = specifier_get_external_inst_list (obj, Qglobal, LOCALE_GLOBAL,
+                                                Qnil, 0, 1, 0);
   if (NILP (the_specs))
-    /* there are no global specs */
-    write_ascstring (printcharfun, "<unspecified>");
+    {
+      /* there are no global specs */
+      write_ascstring (printcharfun, "<unspecified>");
+    }
   else
-    print_internal (the_specs, printcharfun, 1);
+    {
+      /* print_internal() does its own circularity checking, we don't have
+         to. */
+      print_internal (the_specs, printcharfun, escapeflag);
+    }
+
   if (!NILP (sp->fallback))
     {
-      write_fmt_string_lisp (printcharfun, " fallback=%S", sp->fallback);
+      write_ascstring (printcharfun, " fallback=");
+      print_internal (sp->fallback, printcharfun, escapeflag);
     }
-  unbind_to (count);
+
   write_fmt_string (printcharfun, " 0x%x>", LISP_OBJECT_UID (obj));
 }




--
‘As I sat looking up at the Guinness ad, I could never figure out /
How your man stayed up on the surfboard after forty pints of stout’
(C. Moore)



--
Ray