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

[Jens Schmidt]

Jens Schmidt <jschmidt4gnu@vodafonemail.de> writes:

> [...]

The discussion in that other thread of this bug (more related to
bug#67141) is interesting, and I actually waited for it to result in
more commits.  But I think now I should draw attention back to my
original issue, or that what is left of it after the previous discussion
and Andrea's commit 46c2fff.

So I basically completed that commit 46c2fff by also modifying
`native-comp-never-optimize-functions' as previously agreed upon and
updating documentation accordingly:

 (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)
+  ;; Do not include functions `macroexpand' or `rename-buffer' as
+  ;; default values here.  Despite the previous "DO NOT REMOVE"
+  ;; warnings these are no longer needed.  See also the comment on
+  ;; `advice--add-function' and bug#67005.  FIXME: But do include
+  ;; `eval' as temporary (?) remedy for bug#67141.
+  '(eval)
   "Primitive functions to exclude from trampoline optimization.

In `addvice--add-function' I wanted to at least preserve the comment
from my initial patch (see attachment to
874jhv9921.fsf@sappc2.fritz.box/">https://yhetil.org/emacs-bugs/874jhv9921.fsf@sappc2.fritz.box/).  I
think it might help historical research if for that removal there is
something that a "git blame" could be hooked onto.

 (defun advice--add-function (how ref function props)
+  ;; Do not generate trampolines here for primitives, since function
+  ;; `fset' called by `setf' below does that as well.  Plus do not
+  ;; handle `rename-buffer' special w.r.t. trampoline generation,
+  ;; since it is no longer advised by uniquify.el.  `macroexpand' does
+  ;; not require any special handling here, either, since it is not
+  ;; advised during bootstrap (bug#67005).
   (let* ((name (cdr (assq 'name props)))

And in `advice-add' I also added some documentation plus the test on
function advices (not only subr advices!) during bootstrap, this time
conditioned on `purify-flag' as recommended by Stefan:

 <<>>"
+  ;; Actively disallow function advices (here) and advices in general
+  ;; on primitives (in `comp--install-trampoline') 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).
+  ;; - Native compilation does not generate trampolines for advised
+  ;;   primitives while loadup.el executes.
   ;; TODO:
   ;; - record the advice location, to display in describe-function.
-  ;; - change all defadvice in lisp/**/*.el.
-  ;; - obsolete advice.el.
+  (when purify-flag
+    (error "Invalid pre-dump advice on %s" symbol))
   (let* ((f (symbol-function symbol))

What do you think?

>From c8d98cfaebf32dff2ba4e4532e4bc7620ef79dca 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.  Add documentation.
* lisp/emacs-lisp/nadvice.el (advice--add-function): Add
documentation.
(advice-add): Disallow advices during bootstrap.  (Bug#67005)
---
 lisp/emacs-lisp/comp-common.el | 11 ++++++-----
 lisp/emacs-lisp/nadvice.el     | 18 ++++++++++++++++--
 2 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/lisp/emacs-lisp/comp-common.el b/lisp/emacs-lisp/comp-common.el
index 1bdb7280399..f43e5c9fa0f 100644
--- a/lisp/emacs-lisp/comp-common.el
+++ b/lisp/emacs-lisp/comp-common.el
@@ -49,11 +49,12 @@ 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)
+  ;; Do not include functions `macroexpand' or `rename-buffer' as
+  ;; default values here.  Despite the previous "DO NOT REMOVE"
+  ;; warnings these are no longer needed.  See also the comment on
+  ;; `advice--add-function' and bug#67005.  FIXME: But do include
+  ;; `eval' as temporary (?) remedy for bug#67141.
+  '(eval)
   "Primitive functions to exclude from trampoline optimization.
 
 Primitive functions included in this list will not be called
diff --git a/lisp/emacs-lisp/nadvice.el b/lisp/emacs-lisp/nadvice.el
index 42027c01491..79572bbd12d 100644
--- a/lisp/emacs-lisp/nadvice.el
+++ b/lisp/emacs-lisp/nadvice.el
@@ -391,6 +391,12 @@ add-function
 
 ;;;###autoload
 (defun advice--add-function (how ref function props)
+  ;; Do not generate trampolines here for primitives, since function
+  ;; `fset' called by `setf' below does that as well.  Plus do not
+  ;; handle `rename-buffer' special w.r.t. trampoline generation,
+  ;; since it is no longer advised by uniquify.el.  `macroexpand' does
+  ;; not require any special handling here, either, since it is not
+  ;; advised during bootstrap (bug#67005).
   (let* ((name (cdr (assq 'name props)))
          (a (advice--member-p (or name function) (if name t) (gv-deref ref))))
     (when a
@@ -507,10 +513,18 @@ advice-add
 is defined as a macro, alias, command, ...
 HOW can be one of:
 <<>>"
+  ;; Actively disallow function advices (here) and advices in general
+  ;; on primitives (in `comp--install-trampoline') 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).
+  ;; - Native compilation does not generate trampolines for advised
+  ;;   primitives while loadup.el executes.
   ;; TODO:
   ;; - record the advice location, to display in describe-function.
-  ;; - change all defadvice in lisp/**/*.el.
-  ;; - obsolete advice.el.
+  (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))
-- 
2.30.2