From 8a03acc1f6e7e6ec12862931050b7e2190025511 Mon Sep 17 00:00:00 2001
From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
Date: Wed, 8 Nov 2023 23:03:14 +0100
Subject: [PATCH] Improve handling of advices and trampoline generation

* lisp/emacs-lisp/comp.el (native-comp-never-optimize-functions):
Remove default value.
(comp-call-optim-form-call): Operate on advised functions, not on
advices.
* lisp/emacs-lisp/nadvice.el (advice--add-function): Remove trampoline
generation for primitives including the special-cased handling of
`rename-buffer' and `macroexpand'.
(advice-add): Disallow advices during bootstrap.
---
 lisp/emacs-lisp/comp.el    | 18 ++++++++++--------
 lisp/emacs-lisp/nadvice.el | 28 ++++++++++------------------
 2 files changed, 20 insertions(+), 26 deletions(-)

diff --git a/lisp/emacs-lisp/comp.el b/lisp/emacs-lisp/comp.el
index 7fd9543d2ba..71decbe175b 100644
--- a/lisp/emacs-lisp/comp.el
+++ b/lisp/emacs-lisp/comp.el
@@ -119,10 +119,11 @@ native-comp-bootstrap-deny-list
   :version "28.1")

 (defcustom native-comp-never-optimize-functions
-  '(;; 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#XXXXX.
+  '()
   "Primitive functions to exclude from trampoline optimization.

 Primitive functions included in this list will not be called
@@ -3412,7 +3413,10 @@ comp-call-optim-form-call
                    (gethash callee (comp-ctxt-byte-func-to-func-h comp-ctxt)))
                (not (memq callee native-comp-never-optimize-functions)))
       (let* ((f (if (symbolp callee)
-                    (symbol-function callee)
+                    ;; Do not operate on advices, but on the advised
+                    ;; function.  Otherwise, calls to advised
+                    ;; primitives will not be optimized (bug#XXXXX).
+                    (advice--cd*r (symbol-function callee))
                   (cl-assert (byte-code-function-p callee))
                   callee))
              (subrp (subrp f))
@@ -3422,9 +3426,7 @@ comp-call-optim-form-call
           ;; Trampoline removal.
           (let* ((callee (intern (subr-name f))) ; Fix aliased names.
                  (maxarg (cdr (subr-arity f)))
-                 (call-type (if (if subrp
-                                    (not (numberp maxarg))
-                                  (comp-nargs-p comp-func-callee))
+                 (call-type (if (not (numberp maxarg))
                                 'callref
                               'call))
                  (args (if (eq call-type 'callref)
diff --git a/lisp/emacs-lisp/nadvice.el b/lisp/emacs-lisp/nadvice.el
index ce5467f3c5c..144f59faa27 100644
--- a/lisp/emacs-lisp/nadvice.el
+++ b/lisp/emacs-lisp/nadvice.el
@@ -393,22 +393,14 @@ add-function

 ;;;###autoload
 (defun advice--add-function (how ref function props)
-  (when (and (featurep 'native-compile)
-             (subr-primitive-p (gv-deref ref)))
-    (let ((subr-name (intern (subr-name (gv-deref ref)))))
-      ;; Requiring the native compiler to advice `macroexpand' cause a
-      ;; circular dependency in eager macro expansion.  uniquify is
-      ;; advising `rename-buffer' while being loaded in loadup.el.
-      ;; This would require the whole native compiler machinery but we
-      ;; don't want to include it in the dump.  Because these two
-      ;; functions are already handled in
-      ;; `native-comp-never-optimize-functions' we hack the problem
-      ;; this way for now :/
-      (unless (memq subr-name '(macroexpand rename-buffer))
-        ;; Must require explicitly as during bootstrap we have no
-        ;; autoloads.
-        (require 'comp)
-        (comp-subr-trampoline-install subr-name))))
+  ;; 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.  And to avoid similar trouble in
+  ;; future, actively disallow function advices during bootstrap in
+  ;; `advice-add' (bug#XXXXX).
   (let* ((name (cdr (assq 'name props)))
          (a (advice--member-p (or name function) (if name t) (gv-deref ref))))
     (when a
@@ -527,8 +519,8 @@ advice-add
 <<>>"
   ;; TODO:
   ;; - record the advice location, to display in describe-function.
-  ;; - change all defadvice in lisp/**/*.el.
-  ;; - obsolete advice.el.
+  (when dump-mode
+    (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