Re: Lisp-friendly backtraces [was: Lispy backtraces]

[Eli Zaretskii]

> From: Clément Pit--Claudel <address@hidden>
> Date: Mon, 5 Dec 2016 09:14:38 -0500
> 
> On 2016-12-05 08:20, Stefan Monnier wrote:
> >> (defun backtrace ()
> >>   "Print a trace of Lisp function calls currently active.
> >> Output stream used is value of `standard-output'."
> >>   (mapbacktrace #'~/backtrace-1 1))
> > 
> > Have you tried it both byte-compiled and interpreted?  Maybe this
> > function is just simple enough that the result is the same in both
> > cases, but in my experience, the stack is sufficiently different in the
> > two cases that a constant nskip doesn't cut it (hence the use of `base`
> > in backtrace-frame).
> 
> Thanks; I attached an updated patch.  Removing `backtrace' from eval.c makes 
> the patch much harder to read, so I'll do that later.

Thanks, allow me a few additional comments:

> ;; -*- lexical-binding: t -*-
> 
> (defun backtrace-1 (evald func args flags)
>   "Print a trace of a single stack frame to `standard-output'.
> EVALD, FUNC, ARGS, FLAGS are as in `mapbacktrace'."
>   (let ((print-level (or print-level 8)))
>     (princ (if (plist-get flags :debug-on-exit) "* " "  "))
>     (cond
>      ((and evald (not debugger-stack-frame-as-list))
>       (prin1 func)
>       (if args (prin1 args) (princ "()")))
>      (t
>       (prin1 (cons func args))))
>     (princ "\n")))
> 
> (defun backtrace ()
>   "Print a trace of Lisp function calls currently active.
> Output stream used is value of `standard-output'."
>   (mapbacktrace #'backtrace-1 'backtrace))
> 
> (backtrace)
> 
> (defun backtrace-frames ()
>   "Collect all frames of current backtrace into a list."
>   (let ((frames nil))
>     (mapbacktrace (lambda (&rest frame) (push frame frames)) 
> 'backtrace-frames)
>     (nreverse frames)))
> 
> (backtrace-frames)
> 
> (defun ~/backtrace-frame (nframes &optional base)
>   "Return the function and arguments NFRAMES up from current execution point.
> If that frame has not evaluated the arguments yet (or is a special form),
> the value is (nil FUNCTION ARG-FORMS...).
> If that frame has evaluated its arguments and called its function already,
> the value is (t FUNCTION ARG-VALUES...).
> A &rest arg is represented as the tail of the list ARG-VALUES.
> FUNCTION is whatever was supplied as car of evaluated list,
> or a lambda expression for macro calls.
> If NFRAMES is more than the number of frames, the value is nil.
> If BASE is non-nil, it should be a function and NFRAMES counts from its
> nearest activation frame."

It is better to move the description of BASE to the 2nd line, as it's
an argument of this function, while the rest describes the details of
what the function does.  It is plausible that someone would like to
read the doc string just as a reminder of the API, so we had better
not force them to read the entire doc string.

>   (let ((frame nil))
>     (mapbacktrace (lambda (evald func args _)
>                     (when (and base (eq func base))
>                       (setq base nil))
>                     (unless base
>                       (when (eq nframes 0)
>                         (setq frame `(,evald ,func ,@args)))
>                       (setq nframes (1- nframes))))
>                   '~/backtrace-frame)
>     frame))

These functions should go to subr.el, I think.

> +DEFUN ("mapbacktrace", Fmapbacktrace, Smapbacktrace, 1, 2, 0,
> +       doc: /* Call FUNCTION for each frame in backtrace.

Likewise here: BASE is better described on the 2nd line.

> +FUNCTION is called with 4 arguments EVALD FUNC ARGS FLAGS.  If a frame
                                      ^
Colon ':' here, please.

> +      if (backtrace_debug_on_exit (pdl))
> +        {
> +          flags = Fcons (QCdebug_on_exit, Fcons (Qt, Qnil));
> +        }

No need for braces if there's only one statement in the 'if' clause.

> +      if (backtrace_nargs (pdl) == UNEVALLED)
> +        {
> +          call4 (function, Qnil, backtrace_function (pdl), *backtrace_args 
> (pdl), flags);
> +        }

Same here.

Last, but not least: it would be nice to have a couple of tests for
this functionality.

Thanks again for working on this.