bug#67005: 30.0.50; improve nadivce/comp/trampoline handling

[Andrea Corallo]

Jens Schmidt <jschmidt4gnu@vodafonemail.de> writes:

> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
>>> Only slightly off-topic: When changing the default value of user option
>>> `native-comp-never-optimize-functions', which now both Andrea and I have
>>> done, does one also need to bump the :version?  Or is this taken care of
>>> automagically?  If it needs to be done manually, to which value should
>>> we/I set it?
>>
>> I guess you should change it to the version in which the new value will
>> be released (i.e. "30.1").
>
> Andrea has already seen to that, thanks.
>
>>> And here is another comment question: Do you think this snippet that
>>> made me so upset:
>>>
>>>       (let* ((f (if (symbolp callee)
>>>                     (symbol-function callee)
>>>                   (cl-assert (byte-code-function-p callee))
>>>                   callee))
>>>              (subrp (subrp f))
>>>              (comp-func-callee (comp-func-in-unit callee)))
>>>
>>> deserves some explanation?
>>
>> A comment would make sense, since the code got you confused, yes.
>
> That comment added, others removed and shortened.  Please review.
>
> Thanks
>
>>From 8758a6b4f7eff990dea45b651eaa2b7e94db730e Mon Sep 17 00:00:00 2001
> From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
> Date: Mon, 20 Nov 2023 23:42:01 +0100
> Subject: [PATCH] Improve handling of advices and trampoline generation
>
> * lisp/emacs-lisp/comp-common.el
> (native-comp-never-optimize-functions): Remove macroexpand and
> rename-buffer from default value.
> * lisp/emacs-lisp/comp.el (comp-call-optim-form-call): Document call
> optimization for advised primitives.
> * lisp/emacs-lisp/nadvice.el (advice-add): Disallow advices during
> bootstrap.  (Bug#67005)
> ---
>  lisp/emacs-lisp/comp-common.el | 9 ++++-----
>  lisp/emacs-lisp/comp.el        | 8 ++++++++
>  lisp/emacs-lisp/nadvice.el     | 9 +++++++--
>  3 files changed, 19 insertions(+), 7 deletions(-)
>
> diff --git a/lisp/emacs-lisp/comp-common.el b/lisp/emacs-lisp/comp-common.el
> index 6d94d1bd82e..cc7f21900e8 100644
> --- a/lisp/emacs-lisp/comp-common.el
> +++ b/lisp/emacs-lisp/comp-common.el
> @@ -49,11 +49,10 @@ native-comp-verbose
>    :version "28.1")
>
>  (defcustom native-comp-never-optimize-functions
> -  '(eval
> -    ;; The following two are mandatory for Emacs to be working
> -    ;; correctly (see comment in `advice--add-function'). DO NOT
> -    ;; REMOVE.
> -    macroexpand rename-buffer)
> +  ;; We used to list those functions here that were advised during
> +  ;; preload, but we now prefer to disallow preload advices in
> +  ;; `advice-add' (bug#67005).
> +  '(eval)
>    "Primitive functions to exclude from trampoline optimization.
>
>  Primitive functions included in this list will not be called
> diff --git a/lisp/emacs-lisp/comp.el b/lisp/emacs-lisp/comp.el
> index 73764eb1d79..9d537cbcfa7 100644
> --- a/lisp/emacs-lisp/comp.el
> +++ b/lisp/emacs-lisp/comp.el
> @@ -2783,6 +2783,14 @@ comp-call-optim-form-call
>                      (symbol-function callee)
>                    (cl-assert (byte-code-function-p callee))
>                    callee))
> +             ;; Below call to `subrp' returns nil on an advised
> +             ;; primitive F, so that we do not optimize calls to F
> +             ;; with the funcall trampoline removal below.  But if F
> +             ;; is advised while we compile its call, it is very
> +             ;; likely to be advised also when that call is executed.
> +             ;; And in that case an "unoptimized" call to F is
> +             ;; actually cheaper since it avoids the call to the
> +             ;; intermediate native trampoline (bug#67005).
>               (subrp (subrp f))
>               (comp-func-callee (comp-func-in-unit callee)))
>          (cond
> diff --git a/lisp/emacs-lisp/nadvice.el b/lisp/emacs-lisp/nadvice.el
> index 42027c01491..c43d1b86752 100644
> --- a/lisp/emacs-lisp/nadvice.el
> +++ b/lisp/emacs-lisp/nadvice.el
> @@ -507,10 +507,15 @@ advice-add
>  is defined as a macro, alias, command, ...
>  HOW can be one of:
>  <<>>"
> +  ;; Actively disallow function advices during bootstrap for the
> +  ;; following reasons:
> +  ;; - Advices in Emacs' core are generally considered bad style.
> +  ;; - `Snarf-documentation' looses docstrings of advised dumped
> +  ;;   functions (bug#66032#20).
>    ;; TODO:
>    ;; - record the advice location, to display in describe-function.
> -  ;; - change all defadvice in lisp/**/*.el.
> -  ;; - obsolete advice.el.

You might want mention/explain this hunk removal in the Changelog.

> +  (when purify-flag
> +    (error "Invalid pre-dump advice on %s" symbol))
>    (let* ((f (symbol-function symbol))
>        (nf (advice--normalize symbol f)))
>      (unless (eq f nf) (fset symbol nf))

The patch LGTM thanks, just one question: should we guard in advice.el
as well just to stay on the safe side?

BR

  Andrea