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

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

[Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1: Type: text/plain, Size: 12796 bytes --]

X-Debbugs-Cc: Stefan Monnier <monnier@iro.umontreal.ca>, Andrea Corallo <acorallo@gnu.org>
Severity: minor


While digging around in nadvice.el, mainly still because of
bug#66032, I came across the special-cased handling of
`rename-buffer' and `macroexpand' in `advice--add-function'.
From there I came to `native-comp-never-optimize-functions' in
comp.el.  So I started fiddeling away and came to the conclusion
that these are probably no longer needed, and then I found more
issues ... please check whether my reasoning below is correct.

The attached patch is probably a nice TL;DR.


Some notes:

- Nothing described below is really a bug (except probably item
  4b), but fixing these issues should improve long-term
  maintainability, I think.

- All patches have been built on (somewhat randomly chosen)
  commit b819b8d6e90337b4cb36b35c2c6d0112c90a8e24, just to have a
  reference that builds and tests fine on my system.

- When I write "bootstrap & check: OK" below I mean the obvious: I did a
  "make bootstrap && make check" and that came through without errors.
  This is for sure no proof of correctness, but hopefully at least an
  indication that I'm not completely wrong.

- All diffs in 1, ..., 6 are to be successively cumulated, while the
  attached patch is again to be applied directly on commit
  b819b8d6e90337b4cb36b35c2c6d0112c90a8e24.

- When whatever changes have been decided on, I will be glad to
  write unit tests for whatever makes sense to test.

- It's a lengthy essay, sorry.  I hope it's worth reading, though.


1. Since Stefan has removed the advices on uniquify functions in
   1a724cc2d2e7, that special handling of `rename-buffer' in
   `advice--add-function' and `native-comp-never-optimize-functions'
   seems to be obsolete.  Away with it:

------------------------- snip(1) -------------------------
diff --git a/lisp/emacs-lisp/comp.el b/lisp/emacs-lisp/comp.el
index 7fd9543d2ba..d94c19c20fa 100644
--- a/lisp/emacs-lisp/comp.el
+++ b/lisp/emacs-lisp/comp.el
@@ -119,10 +119,9 @@ 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)
+  '(;; The following is mandatory for Emacs to be working correctly
+    ;; (see comment in `advice--add-function'). DO NOT REMOVE.
+    macroexpand)
   "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 ce5467f3c5c..946ca43f1cf 100644
--- a/lisp/emacs-lisp/nadvice.el
+++ b/lisp/emacs-lisp/nadvice.el
@@ -397,14 +397,12 @@ advice--add-function
              (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))
+      ;; circular dependency in eager macro expansion.  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))
         ;; Must require explicitly as during bootstrap we have no
         ;; autoloads.
         (require 'comp)
------------------------- snip(1) -------------------------

   => bootstrap & check: OK


2. The comment in `advice--add-function' says that `macroexpand' causes
   a circular dependency, and there *are* still advices done on
   `macroexpand' (at least in `cl-symbol-macrolet').  But probably these
   are not executed during the bootstrap?  Let's try.

------------------------- snip(2) -------------------------
diff --git a/lisp/emacs-lisp/comp.el b/lisp/emacs-lisp/comp.el
index d94c19c20fa..5bbbabe548f 100644
--- a/lisp/emacs-lisp/comp.el
+++ b/lisp/emacs-lisp/comp.el
@@ -119,9 +119,7 @@ native-comp-bootstrap-deny-list
   :version "28.1")

 (defcustom native-comp-never-optimize-functions
-  '(;; The following is mandatory for Emacs to be working correctly
-    ;; (see comment in `advice--add-function'). DO NOT REMOVE.
-    macroexpand)
+  '()
   "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 946ca43f1cf..e1945cc2b1b 100644
--- a/lisp/emacs-lisp/nadvice.el
+++ b/lisp/emacs-lisp/nadvice.el
@@ -396,17 +396,7 @@ advice--add-function
   (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.  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))
-        ;; Must require explicitly as during bootstrap we have no
-        ;; autoloads.
-        (require 'comp)
-        (comp-subr-trampoline-install subr-name))))
+      (comp-subr-trampoline-install subr-name)))
   (let* ((name (cdr (assq 'name props)))
          (a (advice--member-p (or name function) (if name t) (gv-deref ref))))
     (when a
------------------------- snip(2) -------------------------

   => bootstrap & check: OK


3. But then on the other hand, function Ffset also calls
   `comp-subr-trampoline-install', and if we have condition

     `(subr-primitive-p (gv-deref ref))'

   fulfilled in `advice--add-function', then the following `(setf
   (gv-deref ref) ...)' should ultimately call `fset'.  So probably we
   can completely remove that `comp-subr-trampoline-install' call from
   `advice--add-function', like this:

------------------------- snip(3) -------------------------
diff --git a/lisp/emacs-lisp/nadvice.el b/lisp/emacs-lisp/nadvice.el
index e1945cc2b1b..e7027bb36a7 100644
--- a/lisp/emacs-lisp/nadvice.el
+++ b/lisp/emacs-lisp/nadvice.el
@@ -393,10 +393,6 @@ 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)))))
-      (comp-subr-trampoline-install subr-name)))
   (let* ((name (cdr (assq 'name props)))
          (a (advice--member-p (or name function) (if name t) (gv-deref ref))))
     (when a
------------------------- snip(3) -------------------------

   => bootstrap & check: OK


4. At this stage, what happens if we reactivate (a different) advice on
   `rename-buffer' in uniquify.el?

------------------------- snip(4) -------------------------
diff --git a/lisp/uniquify.el b/lisp/uniquify.el
index 7119ae7eac3..24d6ccaefad 100644
--- a/lisp/uniquify.el
+++ b/lisp/uniquify.el
@@ -489,6 +489,14 @@ uniquify-kill-buffer-function
 ;; rename-buffer and create-file-buffer.  (Setting find-file-hook isn't
 ;; sufficient.)

+(defconst uniquify--rename-buffer-history '())
+
+(advice-add 'rename-buffer :around #'uniquify--the-real-rename-buffer-advice)
+(defun uniquify--the-real-rename-buffer-advice (origfun newname &optional unique)
+  (setq uniquify--rename-buffer-history
+        (cons newname uniquify--rename-buffer-history))
+  (funcall origfun newname unique))
+
 ;; (advice-add 'rename-buffer :around #'uniquify--rename-buffer-advice)
 (defun uniquify--rename-buffer-advice (newname &optional unique)
   ;; BEWARE: This is called directly from `buffer.c'!
------------------------- snip(4) -------------------------

  => bootstrap & check: OK

  Plus the advice gets properly called, even from natively compiled
  code!  Huh?

  I think this works without problems since there is already a second
  line of defense as follows:

  a) loadup.el sets `native-comp-enable-subr-trampolines' to t only
     after all files have been loaded, so Ffset, which specifically
     tests for `native-comp-enable-subr-trampolines', never will call
     `comp-subr-trampoline-install' during bootstrap.

  b) As soon as `rename-buffer' got advised (which *is* soon in the
     bootstrap), the test on `subrp' in function
     `comp-call-optim-form-call' evals to nil, since the `subrp' only
     "sees" the surrounding advice, and not the inner primitive.  Which
     means that call optimizations won't take place for that.


5. And actually I think that b) above probably is a bug: It means that
   for advised subrs, the optimzation to indirect calls in function
   `comp-call-optim-form-call' never takes place.

   So I tried to recognize subrs in function `comp-call-optim-form-call'
   even when they are advised:

------------------------- snip(5) -------------------------
diff --git a/lisp/emacs-lisp/comp.el b/lisp/emacs-lisp/comp.el
index 5bbbabe548f..e60e5a0fa61 100644
--- a/lisp/emacs-lisp/comp.el
+++ b/lisp/emacs-lisp/comp.el
@@ -3409,7 +3409,7 @@ 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)
+                    (advice--cd*r (symbol-function callee))
                   (cl-assert (byte-code-function-p callee))
                   callee))
              (subrp (subrp f))
@@ -3419,9 +3419,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)
------------------------- snip(5) -------------------------

   (The second diff in that function is a minor optimization - subrp
   should be always t in that branch of the `cond'.)

   => bootstrap & check: OK

   But: Natively compiled code now does NOT execute the advice on
   `rename-buffer' added in step 4, since its call gets optimized but no
   trampoline has been created for it.


6. So there is the question whether we should actively disallow advices
   during bootstrap, now that we are free of them.  Like this:

------------------------- snip(6.1) -------------------------
diff --git a/lisp/emacs-lisp/nadvice.el b/lisp/emacs-lisp/nadvice.el
index e7027bb36a7..b6085c4c74b 100644
--- a/lisp/emacs-lisp/nadvice.el
+++ b/lisp/emacs-lisp/nadvice.el
@@ -511,8 +511,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))
------------------------- snip(6.1) -------------------------

   (As a bonus I removed two TODOs which habe been completed already.)

   => bootstrap fails due to the advice added in step 4, as expected:

   Error: error ("Invalid pre-dump advice on rename-buffer")
   [...]

   When removing that test advice on `rename-buffer' added in step 4, we
   again get:

   => bootstrap & check: OK

   It would have been more satisfying to add the check on `dump-mode' at
   a deeper level, for example in function `advice--add-function'.
   However, such a check must error out only when function-like objects
   are advised, since there are valid non-function advices added during
   bootstrap.  But in `advice--add-function' we only have gv's pointing
   to the object to be advised, and I don't know a way how to determine
   that a gv originates from something function-like.

What do you think?



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Improve-handling-of-advices-and-trampoline-generatio.patch --]
[-- Type: text/x-diff, Size: 5048 bytes --]

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

[-- Attachment #3: Type: text/plain, Size: 16322 bytes --]



In GNU Emacs 30.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version
 3.24.24, cairo version 1.16.0) of 2023-10-05 built on sappc2
Repository revision: b819b8d6e90337b4cb36b35c2c6d0112c90a8e24
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12011000
System Description: Debian GNU/Linux 11 (bullseye)

Configured using:
 'configure -C --with-native-compilation --with-mailutils'

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG
JSON LCMS2 LIBOTF LIBSELINUX LIBSYSTEMD LIBXML2 M17N_FLT MODULES
NATIVE_COMP NOTIFY INOTIFY PDUMPER PNG RSVG SECCOMP SOUND THREADS TIFF
TOOLKIT_SCROLL_BARS X11 XDBE XIM XINPUT2 XPM GTK3 ZLIB

Important settings:
  value of $LC_COLLATE: POSIX
  value of $LC_TIME: POSIX
  value of $LANG: en_US.UTF-8
  value of $XMODIFIERS: @im=ibus
  locale-coding-system: utf-8-unix

Major mode: Lisp Interaction

Minor modes in effect:
  display-time-mode: t
  delete-selection-mode: t
  show-paren-mode: t
  mouse-wheel-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  minibuffer-regexp-mode: t
  line-number-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t

Load-path shadows:
/home/jschmidt/.emacs.d/elpa/transient-20230919.2146/transient hides /home/jschmidt/work/emacs-master/lisp/transient
/home/jschmidt/work/org-mode/lisp/org-attach hides /home/jschmidt/work/emacs-master/lisp/org/org-attach
/home/jschmidt/work/org-mode/lisp/ob-scheme hides /home/jschmidt/work/emacs-master/lisp/org/ob-scheme
/home/jschmidt/work/org-mode/lisp/ob-processing hides /home/jschmidt/work/emacs-master/lisp/org/ob-processing
/home/jschmidt/work/org-mode/lisp/ob-gnuplot hides /home/jschmidt/work/emacs-master/lisp/org/ob-gnuplot
/home/jschmidt/work/org-mode/lisp/org-element hides /home/jschmidt/work/emacs-master/lisp/org/org-element
/home/jschmidt/work/org-mode/lisp/ob-org hides /home/jschmidt/work/emacs-master/lisp/org/ob-org
/home/jschmidt/work/org-mode/lisp/ob-sql hides /home/jschmidt/work/emacs-master/lisp/org/ob-sql
/home/jschmidt/work/org-mode/lisp/oc-bibtex hides /home/jschmidt/work/emacs-master/lisp/org/oc-bibtex
/home/jschmidt/work/org-mode/lisp/ox-koma-letter hides /home/jschmidt/work/emacs-master/lisp/org/ox-koma-letter
/home/jschmidt/work/org-mode/lisp/ox-icalendar hides /home/jschmidt/work/emacs-master/lisp/org/ox-icalendar
/home/jschmidt/work/org-mode/lisp/ol-gnus hides /home/jschmidt/work/emacs-master/lisp/org/ol-gnus
/home/jschmidt/work/org-mode/lisp/ob-sqlite hides /home/jschmidt/work/emacs-master/lisp/org/ob-sqlite
/home/jschmidt/work/org-mode/lisp/ob-clojure hides /home/jschmidt/work/emacs-master/lisp/org/ob-clojure
/home/jschmidt/work/org-mode/lisp/org-indent hides /home/jschmidt/work/emacs-master/lisp/org/org-indent
/home/jschmidt/work/org-mode/lisp/org-persist hides /home/jschmidt/work/emacs-master/lisp/org/org-persist
/home/jschmidt/work/org-mode/lisp/ol-eww hides /home/jschmidt/work/emacs-master/lisp/org/ol-eww
/home/jschmidt/work/org-mode/lisp/ol-info hides /home/jschmidt/work/emacs-master/lisp/org/ol-info
/home/jschmidt/work/org-mode/lisp/ol-rmail hides /home/jschmidt/work/emacs-master/lisp/org/ol-rmail
/home/jschmidt/work/org-mode/lisp/oc-basic hides /home/jschmidt/work/emacs-master/lisp/org/oc-basic
/home/jschmidt/work/org-mode/lisp/ob-forth hides /home/jschmidt/work/emacs-master/lisp/org/ob-forth
/home/jschmidt/work/org-mode/lisp/org-timer hides /home/jschmidt/work/emacs-master/lisp/org/org-timer
/home/jschmidt/work/org-mode/lisp/ob-makefile hides /home/jschmidt/work/emacs-master/lisp/org/ob-makefile
/home/jschmidt/work/org-mode/lisp/ob-fortran hides /home/jschmidt/work/emacs-master/lisp/org/ob-fortran
/home/jschmidt/work/org-mode/lisp/ox-html hides /home/jschmidt/work/emacs-master/lisp/org/ox-html
/home/jschmidt/work/org-mode/lisp/ob-lob hides /home/jschmidt/work/emacs-master/lisp/org/ob-lob
/home/jschmidt/work/org-mode/lisp/org-plot hides /home/jschmidt/work/emacs-master/lisp/org/org-plot
/home/jschmidt/work/org-mode/lisp/ob-js hides /home/jschmidt/work/emacs-master/lisp/org/ob-js
/home/jschmidt/work/org-mode/lisp/ob-R hides /home/jschmidt/work/emacs-master/lisp/org/ob-R
/home/jschmidt/work/org-mode/lisp/org-inlinetask hides /home/jschmidt/work/emacs-master/lisp/org/org-inlinetask
/home/jschmidt/work/org-mode/lisp/ol-mhe hides /home/jschmidt/work/emacs-master/lisp/org/ol-mhe
/home/jschmidt/work/org-mode/lisp/ob-dot hides /home/jschmidt/work/emacs-master/lisp/org/ob-dot
/home/jschmidt/work/org-mode/lisp/ol-docview hides /home/jschmidt/work/emacs-master/lisp/org/ol-docview
/home/jschmidt/work/org-mode/lisp/ob-C hides /home/jschmidt/work/emacs-master/lisp/org/ob-C
/home/jschmidt/work/org-mode/lisp/ol-man hides /home/jschmidt/work/emacs-master/lisp/org/ol-man
/home/jschmidt/work/org-mode/lisp/org-cycle hides /home/jschmidt/work/emacs-master/lisp/org/org-cycle
/home/jschmidt/work/org-mode/lisp/org-pcomplete hides /home/jschmidt/work/emacs-master/lisp/org/org-pcomplete
/home/jschmidt/work/org-mode/lisp/org-faces hides /home/jschmidt/work/emacs-master/lisp/org/org-faces
/home/jschmidt/work/org-mode/lisp/org hides /home/jschmidt/work/emacs-master/lisp/org/org
/home/jschmidt/work/org-mode/lisp/ol hides /home/jschmidt/work/emacs-master/lisp/org/ol
/home/jschmidt/work/org-mode/lisp/ob-haskell hides /home/jschmidt/work/emacs-master/lisp/org/ob-haskell
/home/jschmidt/work/org-mode/lisp/ob-lisp hides /home/jschmidt/work/emacs-master/lisp/org/ob-lisp
/home/jschmidt/work/org-mode/lisp/org-mobile hides /home/jschmidt/work/emacs-master/lisp/org/org-mobile
/home/jschmidt/work/org-mode/lisp/org-agenda hides /home/jschmidt/work/emacs-master/lisp/org/org-agenda
/home/jschmidt/work/org-mode/lisp/ob-perl hides /home/jschmidt/work/emacs-master/lisp/org/ob-perl
/home/jschmidt/work/org-mode/lisp/org-lint hides /home/jschmidt/work/emacs-master/lisp/org/org-lint
/home/jschmidt/work/org-mode/lisp/org-id hides /home/jschmidt/work/emacs-master/lisp/org/org-id
/home/jschmidt/work/org-mode/lisp/ox-man hides /home/jschmidt/work/emacs-master/lisp/org/ox-man
/home/jschmidt/work/org-mode/lisp/ol-bbdb hides /home/jschmidt/work/emacs-master/lisp/org/ol-bbdb
/home/jschmidt/work/org-mode/lisp/ob-lilypond hides /home/jschmidt/work/emacs-master/lisp/org/ob-lilypond
/home/jschmidt/work/org-mode/lisp/org-archive hides /home/jschmidt/work/emacs-master/lisp/org/org-archive
/home/jschmidt/work/org-mode/lisp/ox-publish hides /home/jschmidt/work/emacs-master/lisp/org/ox-publish
/home/jschmidt/work/org-mode/lisp/ob-core hides /home/jschmidt/work/emacs-master/lisp/org/ob-core
/home/jschmidt/work/org-mode/lisp/ob-groovy hides /home/jschmidt/work/emacs-master/lisp/org/ob-groovy
/home/jschmidt/work/org-mode/lisp/ox-org hides /home/jschmidt/work/emacs-master/lisp/org/ox-org
/home/jschmidt/work/org-mode/lisp/org-entities hides /home/jschmidt/work/emacs-master/lisp/org/org-entities
/home/jschmidt/work/org-mode/lisp/org-goto hides /home/jschmidt/work/emacs-master/lisp/org/org-goto
/home/jschmidt/work/org-mode/lisp/ob-awk hides /home/jschmidt/work/emacs-master/lisp/org/ob-awk
/home/jschmidt/work/org-mode/lisp/ol-eshell hides /home/jschmidt/work/emacs-master/lisp/org/ol-eshell
/home/jschmidt/work/org-mode/lisp/org-num hides /home/jschmidt/work/emacs-master/lisp/org/org-num
/home/jschmidt/work/org-mode/lisp/oc-csl hides /home/jschmidt/work/emacs-master/lisp/org/oc-csl
/home/jschmidt/work/org-mode/lisp/org-capture hides /home/jschmidt/work/emacs-master/lisp/org/org-capture
/home/jschmidt/work/org-mode/lisp/ob-ref hides /home/jschmidt/work/emacs-master/lisp/org/ob-ref
/home/jschmidt/work/org-mode/lisp/org-list hides /home/jschmidt/work/emacs-master/lisp/org/org-list
/home/jschmidt/work/org-mode/lisp/org-macro hides /home/jschmidt/work/emacs-master/lisp/org/org-macro
/home/jschmidt/work/org-mode/lisp/org-clock hides /home/jschmidt/work/emacs-master/lisp/org/org-clock
/home/jschmidt/work/org-mode/lisp/ob-table hides /home/jschmidt/work/emacs-master/lisp/org/ob-table
/home/jschmidt/work/org-mode/lisp/org-datetree hides /home/jschmidt/work/emacs-master/lisp/org/org-datetree
/home/jschmidt/work/org-mode/lisp/org-mouse hides /home/jschmidt/work/emacs-master/lisp/org/org-mouse
/home/jschmidt/work/org-mode/lisp/ob-latex hides /home/jschmidt/work/emacs-master/lisp/org/ob-latex
/home/jschmidt/work/org-mode/lisp/org-keys hides /home/jschmidt/work/emacs-master/lisp/org/org-keys
/home/jschmidt/work/org-mode/lisp/org-compat hides /home/jschmidt/work/emacs-master/lisp/org/org-compat
/home/jschmidt/work/org-mode/lisp/org-habit hides /home/jschmidt/work/emacs-master/lisp/org/org-habit
/home/jschmidt/work/org-mode/lisp/org-tempo hides /home/jschmidt/work/emacs-master/lisp/org/org-tempo
/home/jschmidt/work/org-mode/lisp/org-refile hides /home/jschmidt/work/emacs-master/lisp/org/org-refile
/home/jschmidt/work/org-mode/lisp/ob-ruby hides /home/jschmidt/work/emacs-master/lisp/org/ob-ruby
/home/jschmidt/work/org-mode/lisp/org-attach-git hides /home/jschmidt/work/emacs-master/lisp/org/org-attach-git
/home/jschmidt/work/org-mode/lisp/org-loaddefs hides /home/jschmidt/work/emacs-master/lisp/org/org-loaddefs
/home/jschmidt/work/org-mode/lisp/org-duration hides /home/jschmidt/work/emacs-master/lisp/org/org-duration
/home/jschmidt/work/org-mode/lisp/ob-ocaml hides /home/jschmidt/work/emacs-master/lisp/org/ob-ocaml
/home/jschmidt/work/org-mode/lisp/org-fold hides /home/jschmidt/work/emacs-master/lisp/org/org-fold
/home/jschmidt/work/org-mode/lisp/ox-ascii hides /home/jschmidt/work/emacs-master/lisp/org/ox-ascii
/home/jschmidt/work/org-mode/lisp/ob-css hides /home/jschmidt/work/emacs-master/lisp/org/ob-css
/home/jschmidt/work/org-mode/lisp/ob-tangle hides /home/jschmidt/work/emacs-master/lisp/org/ob-tangle
/home/jschmidt/work/org-mode/lisp/ob-python hides /home/jschmidt/work/emacs-master/lisp/org/ob-python
/home/jschmidt/work/org-mode/lisp/org-crypt hides /home/jschmidt/work/emacs-master/lisp/org/org-crypt
/home/jschmidt/work/org-mode/lisp/ol-bibtex hides /home/jschmidt/work/emacs-master/lisp/org/ol-bibtex
/home/jschmidt/work/org-mode/lisp/oc-biblatex hides /home/jschmidt/work/emacs-master/lisp/org/oc-biblatex
/home/jschmidt/work/org-mode/lisp/org-protocol hides /home/jschmidt/work/emacs-master/lisp/org/org-protocol
/home/jschmidt/work/org-mode/lisp/org-feed hides /home/jschmidt/work/emacs-master/lisp/org/org-feed
/home/jschmidt/work/org-mode/lisp/ob-maxima hides /home/jschmidt/work/emacs-master/lisp/org/ob-maxima
/home/jschmidt/work/org-mode/lisp/org-colview hides /home/jschmidt/work/emacs-master/lisp/org/org-colview
/home/jschmidt/work/org-mode/lisp/ol-w3m hides /home/jschmidt/work/emacs-master/lisp/org/ol-w3m
/home/jschmidt/work/org-mode/lisp/ob-ditaa hides /home/jschmidt/work/emacs-master/lisp/org/ob-ditaa
/home/jschmidt/work/org-mode/lisp/ob-plantuml hides /home/jschmidt/work/emacs-master/lisp/org/ob-plantuml
/home/jschmidt/work/org-mode/lisp/org-src hides /home/jschmidt/work/emacs-master/lisp/org/org-src
/home/jschmidt/work/org-mode/lisp/ob-sed hides /home/jschmidt/work/emacs-master/lisp/org/ob-sed
/home/jschmidt/work/org-mode/lisp/ox-latex hides /home/jschmidt/work/emacs-master/lisp/org/ox-latex
/home/jschmidt/work/org-mode/lisp/ob-exp hides /home/jschmidt/work/emacs-master/lisp/org/ob-exp
/home/jschmidt/work/org-mode/lisp/ob-lua hides /home/jschmidt/work/emacs-master/lisp/org/ob-lua
/home/jschmidt/work/org-mode/lisp/ox hides /home/jschmidt/work/emacs-master/lisp/org/ox
/home/jschmidt/work/org-mode/lisp/org-footnote hides /home/jschmidt/work/emacs-master/lisp/org/org-footnote
/home/jschmidt/work/org-mode/lisp/ol-doi hides /home/jschmidt/work/emacs-master/lisp/org/ol-doi
/home/jschmidt/work/org-mode/lisp/ob-emacs-lisp hides /home/jschmidt/work/emacs-master/lisp/org/ob-emacs-lisp
/home/jschmidt/work/org-mode/lisp/ox-odt hides /home/jschmidt/work/emacs-master/lisp/org/ox-odt
/home/jschmidt/work/org-mode/lisp/ob-eval hides /home/jschmidt/work/emacs-master/lisp/org/ob-eval
/home/jschmidt/work/org-mode/lisp/ob-matlab hides /home/jschmidt/work/emacs-master/lisp/org/ob-matlab
/home/jschmidt/work/org-mode/lisp/ob-sass hides /home/jschmidt/work/emacs-master/lisp/org/ob-sass
/home/jschmidt/work/org-mode/lisp/ob-java hides /home/jschmidt/work/emacs-master/lisp/org/ob-java
/home/jschmidt/work/org-mode/lisp/ob-julia hides /home/jschmidt/work/emacs-master/lisp/org/ob-julia
/home/jschmidt/work/org-mode/lisp/org-version hides /home/jschmidt/work/emacs-master/lisp/org/org-version
/home/jschmidt/work/org-mode/lisp/ob-calc hides /home/jschmidt/work/emacs-master/lisp/org/ob-calc
/home/jschmidt/work/org-mode/lisp/org-table hides /home/jschmidt/work/emacs-master/lisp/org/org-table
/home/jschmidt/work/org-mode/lisp/ol-irc hides /home/jschmidt/work/emacs-master/lisp/org/ol-irc
/home/jschmidt/work/org-mode/lisp/ob-eshell hides /home/jschmidt/work/emacs-master/lisp/org/ob-eshell
/home/jschmidt/work/org-mode/lisp/org-fold-core hides /home/jschmidt/work/emacs-master/lisp/org/org-fold-core
/home/jschmidt/work/org-mode/lisp/org-macs hides /home/jschmidt/work/emacs-master/lisp/org/org-macs
/home/jschmidt/work/org-mode/lisp/ob-comint hides /home/jschmidt/work/emacs-master/lisp/org/ob-comint
/home/jschmidt/work/org-mode/lisp/ox-texinfo hides /home/jschmidt/work/emacs-master/lisp/org/ox-texinfo
/home/jschmidt/work/org-mode/lisp/oc-natbib hides /home/jschmidt/work/emacs-master/lisp/org/oc-natbib
/home/jschmidt/work/org-mode/lisp/ob-screen hides /home/jschmidt/work/emacs-master/lisp/org/ob-screen
/home/jschmidt/work/org-mode/lisp/ox-beamer hides /home/jschmidt/work/emacs-master/lisp/org/ox-beamer
/home/jschmidt/work/org-mode/lisp/ob-octave hides /home/jschmidt/work/emacs-master/lisp/org/ob-octave
/home/jschmidt/work/org-mode/lisp/ob hides /home/jschmidt/work/emacs-master/lisp/org/ob
/home/jschmidt/work/org-mode/lisp/ob-shell hides /home/jschmidt/work/emacs-master/lisp/org/ob-shell
/home/jschmidt/work/org-mode/lisp/ox-md hides /home/jschmidt/work/emacs-master/lisp/org/ox-md
/home/jschmidt/work/org-mode/lisp/oc hides /home/jschmidt/work/emacs-master/lisp/org/oc
/home/jschmidt/work/org-mode/lisp/org-ctags hides /home/jschmidt/work/emacs-master/lisp/org/org-ctags

Features:
(shadow sort mail-extr emacsbug message yank-media puny dired
dired-loaddefs rfc822 mml mml-sec epa derived epg rfc6068 epg-config
gnus-util time-date mm-decode mm-bodies mm-encode mail-parse rfc2231
mailabbrev gmm-utils mailheader sendmail rfc2047 rfc2045 ietf-drums
mm-util mail-prsvr mail-utils compile text-property-search comint
ansi-osc ansi-color ring comp comp-cstr warnings icons rx cl-extra
help-mode time delsel cus-load advice finder-inf pcase info package
browse-url url url-proxy url-privacy url-expand url-methods url-history
url-cookie generate-lisp-file url-domsuf url-util mailcap url-handlers
url-parse auth-source cl-seq eieio eieio-core cl-macs password-cache
json subr-x map byte-opt gv bytecomp byte-compile url-vars cl-loaddefs
cl-lib rmc iso-transl tooltip cconv eldoc paren electric uniquify
ediff-hook vc-hooks lisp-float-type elisp-mode mwheel term/x-win x-win
term/common-win x-dnd touch-screen tool-bar dnd fontset image regexp-opt
fringe tabulated-list replace newcomment text-mode lisp-mode prog-mode
register page tab-bar menu-bar rfn-eshadow isearch easymenu timer select
scroll-bar mouse jit-lock font-lock syntax font-core term/tty-colors
frame minibuffer nadvice seq simple cl-generic indonesian philippine
cham georgian utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao
korean japanese eucjp-ms cp51932 hebrew greek romanian slovak czech
european ethiopic indian cyrillic chinese composite emoji-zwj charscript
charprop case-table epa-hook jka-cmpr-hook help abbrev obarray oclosure
cl-preloaded button loaddefs theme-loaddefs faces cus-face macroexp
files window text-properties overlay sha1 md5 base64 format env
code-pages mule custom widget keymap hashtable-print-readable backquote
threads dbusbind inotify lcms2 dynamic-setting system-font-setting
font-render-setting cairo gtk x-toolkit xinput2 x multi-tty move-toolbar
make-network-process native-compile emacs)

Memory information:
((conses 16 183456 15026) (symbols 48 12113 0)
 (strings 32 53831 3178) (string-bytes 1 1793792)
 (vectors 16 33549) (vector-slots 8 534071 15677)
 (floats 8 62 27) (intervals 56 312 0) (buffers 984 11))

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> 1. Since Stefan has removed the advices on uniquify functions in
>    1a724cc2d2e7, that special handling of `rename-buffer' in
>    `advice--add-function' and `native-comp-never-optimize-functions'
>    seems to be obsolete.  Away with it:

+1

> 2. The comment in `advice--add-function' says that `macroexpand' causes
>    a circular dependency, and there *are* still advices done on
>    `macroexpand' (at least in `cl-symbol-macrolet').  But probably these
>    are not executed during the bootstrap?  Let's try.

+1

> 3. But then on the other hand, function Ffset also calls
>    `comp-subr-trampoline-install', and if we have condition
>
>      `(subr-primitive-p (gv-deref ref))'
>
>    fulfilled in `advice--add-function', then the following `(setf
>    (gv-deref ref) ...)' should ultimately call `fset'.  So probably we
>    can completely remove that `comp-subr-trampoline-install' call from
>    `advice--add-function', like this:

It looks good to me, tho I must admit I do not understand why we have
this `comp-subr-trampoline-install` in addition to the one in `Ffset`,
so maybe I'm missing something.

> 5. And actually I think that b) above probably is a bug: It means that
>    for advised subrs, the optimzation to indirect calls in function
>    `comp-call-optim-form-call' never takes place.
>
>    So I tried to recognize subrs in function `comp-call-optim-form-call'
>    even when they are advised:

I think this is not a correct optimization, as you mention:

>    But: Natively compiled code now does NOT execute the advice on
>    `rename-buffer' added in step 4, since its call gets optimized but no
>    trampoline has been created for it.

So it's a -1 from me on this patch.

> 6. So there is the question whether we should actively disallow advices
>    during bootstrap, now that we are free of them.  Like this:

Rather than `dump-mode`, I'd test `purify-flag`.
This is because `purify-flag` is not set while building `src/bootstrap-emacs`.

Part of the issue here is that during the build of `src/bootstrap-emacs`
we load a lot more ELisp code than in the "real" build so it's good to
restrict this constraint to the "real build".


        Stefan

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

[Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

[...]

Thanks a lot for your feedback!  I'm trying to come up with a reply, in
particular to your one "-1", but this all gets more and more funny.
Please stay tuned!

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

[Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Thanks to both of you for taking the time to read through this!  I tried
my best to combine both replies in one mail.

>>>>> "SM" == Stefan Monnier <monnier@iro.umontreal.ca> writes:
>>>>> "AC" == Andrea Corallo <acorallo@gnu.org> writes:

 >> 1. Since Stefan has removed the advices on uniquify functions in
 >>    1a724cc2d2e7, that special handling of `rename-buffer' in
 >>    `advice--add-function' and `native-comp-never-optimize-functions'
 >>    seems to be obsolete.  Away with it:

 SM> +1

 AC> +1


 >> 2. The comment in `advice--add-function' says that `macroexpand'
 >>    causes a circular dependency, and there *are* still advices done
 >>    on `macroexpand' (at least in `cl-symbol-macrolet').  But
 >>    probably these are not executed during the bootstrap?  Let's try.

 SM> +1

 AC> +1


 >> 3. But then on the other hand, function Ffset also calls
 >>    `comp-subr-trampoline-install', and if we have condition
 >>
 >>      `(subr-primitive-p (gv-deref ref))'
 >>
 >>    fulfilled in `advice--add-function', then the following `(setf
 >>    (gv-deref ref) ...)' should ultimately call `fset'.  So probably
 >>    we can completely remove that `comp-subr-trampoline-install' call
 >>    from `advice--add-function', like this:

 SM> It looks good to me, tho I must admit I do not understand why we
 SM> have this `comp-subr-trampoline-install` in addition to the one in
 SM> `Ffset`, so maybe I'm missing something.

 AC> Unfortanatelly I can't remember why we have this specific handling,
 AC> I guess there was a reason but anyway... If there is still a good
 AC> reason we'll discover it.

I might have found a reason ... see section "The Bigger Picture" below.


 >> 5. And actually I think that b) above probably is a bug: It means
 >>    that for advised subrs, the optimzation to indirect calls in
 >>    function `comp-call-optim-form-call' never takes place.
 >>
 >>    So I tried to recognize subrs in function
 >>    `comp-call-optim-form-call' even when they are advised:

 SM> I think this is not a correct optimization, as you mention:

 >>    But: Natively compiled code now does NOT execute the advice on
 >>    `rename-buffer' added in step 4, since its call gets optimized
 >>    but no trampoline has been created for it.

 SM> So it's a -1 from me on this patch.

 AC> It's good we optimize the call but we can't apply this patch if the
 AC> this issue is not solved.

Please let me try to address your concerns.  TL;DR:

- If `rename-buffer' is advised *during* bootstrap (as with patch 4), no
  native trampoline gets created for it because of
  `native-comp-enable-subr-trampolines' being nil during loadup.el.

- Without native trampoline, the fix that I propose in patch 5 lead to
  the advice not being called.

However:

- If `rename-buffer' (or any other primitive) is advised *after*
  bootstrap, a trampoline gets properly created and the advice is being
  called even with my fix in patch 5.

Full version: See section "More Background Information ..."


 >> 6. So there is the question whether we should actively disallow
 >>    advices during bootstrap, now that we are free of them.  Like
 >>    this:

 AC> Others can have different opinions but if it's not a big limitation
 AC> for the future it sounds like a good idea to me.

 SM> Rather than `dump-mode`, I'd test `purify-flag`.
 SM> This is because `purify-flag` is not set while building
 SM> `src/bootstrap-emacs`.

 SM> Part of the issue here is that during the build of
 SM> `src/bootstrap-emacs` we load a lot more ELisp code than in the
 SM> "real" build so it's good to restrict this constraint to the "real
 SM> build".

Will do.


More Background Information on that Suspicous Fix ...
-----------------------------------------------------

Now for more background on why I think the current way of detecting
primitives in function `comp-call-optim-form-call' is buggy.  I provide
that background not to lecture anybody, but to make my reasoning
hopefully more transparent.

There are two different items in the native compilation area called
"trampoline":

- There are "funcall trampolines", which are instructions to call a
  function from natively compiled code through a `funcall' indirection.
  A call of a primitive through such a funcall trampoline is pretty
  close to a function call of that primitive from interpreted Elisp and
  always executes advices.

- And there are "native trampolines", which are natively compiled
  wrappers for primitives.  If and only if a primitive has such a native
  trampoline, advices on the primitive are executed even if the
  primitive is called from natively compiled code *without* a funcall
  trampoline.

To see the difference, start with a vanilla master emacs, without any of
my patches, and rewrite the function `comp-call-optim' with extended
logging to a file test.el as follows:

------------------------- test.el -------------------------
(with-eval-after-load 'comp
(defun comp-call-optim (_)
  "Try to optimize out funcall trampoline usage when possible."
  (maphash (lambda (_ f)
             (when (and (>= (comp-func-speed f) 2)
                        (comp-func-l-p f))
               (let ((comp-func f))
                 (when (eq (comp-func-name f) 'foo)
                   (comp-log "\nPre comp-call-optim" 0)
                   (comp-log-func f 0))
                 (comp-call-optim-func)
                 (when (eq (comp-func-name f) 'foo)
                   (comp-log "\nPost comp-call-optim" 0)
                   (comp-log-func f 0)))))
           (comp-ctxt-funcs-h comp-ctxt))))
------------------------- test.el -------------------------

And the following to a file foo.el:

------------------------- foo.el -------------------------
;;; -*- lexical-binding: t -*-
(defun foo (x)
  (sqrt x))
------------------------- foo.el -------------------------

Then start "./src/emacs -Q -l test.el" and evaluate the following in
*scratch*:

------------------------- snip(A) -------------------------
(with-current-buffer
    (get-buffer-create "*Native-compile-Log*")
  (setq buffer-read-only nil)
  (erase-buffer))
(load-file (native-compile "foo.el"))
(pop-to-buffer "*Native-compile-Log*")
------------------------- snip(A) -------------------------

The interesting part in the resulting *Native-compile-Log* is how the
call to `sqrt' in function `foo' gets optimized.  (I chose that as
example primitive because it is not as often called as `rename-buffer'
and causes less noise during tests.)

------------------------- LIMPEL -------------------------
Pre comp-call-optim

(set #(mvar 23578551540518 1 float) (callref funcall #(mvar 23578553446684 1 (member sqrt)) #(mvar 23578554498624 2 t)))
                                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Post comp-call-optim

(set #(mvar 23578551540518 1 float) (call sqrt #(mvar 23578554498624 2 t)))
                                     ^^^^
------------------------- LIMPEL -------------------------

So the indirect `callref funcall' got optimized to a direct `call', and:
An advice on `sqrt' is executed even if the natively compiled `foo' is
called:

------------------------- snip(B) -------------------------
(defun sqrt-advice (&rest _)
  (message "Advice called"))
(advice-add #'sqrt :before #'sqrt-advice)
(foo 0)

=> "Advice called" echoed in message area
------------------------- snip(B) -------------------------

Now re-evaluate the snip(A) again and see how `foo' is *not* optimized,
this time because of the advice on `sqrt' and because of what I consider
the bug in `comp-call-optim-form-call':

------------------------- LIMPEL -------------------------
Pre comp-call-optim

(set #(mvar 23487733705560 1 float) (callref funcall #(mvar 23487733185614 1 (member sqrt)) #(mvar 23487733838350 2 t)))
                                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Post comp-call-optim

(set #(mvar 23487733705560 1 float) (callref funcall #(mvar 23487733185614 1 (member sqrt)) #(mvar 23487733838350 2 t)))
                                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
------------------------- LIMPEL -------------------------


To summarize the current, unpatched master behavior:

- The result of native compilation of a call to a primitive depends on
  whether the primitive is advised or not.

- If it is not advised, the result is an optimized call that executes
  advices if a native trampoline exists.

- If it is advised, the result is a non-optimized call (that always
  executes advices).

In my opinion, we should instead:

- always use optimized calls, unconditionally whether an advice exists
  at the time of the compilation or not;

- ensure that native trampolines get installed when a primitive is
  advised;

- ensure that advices are ruled out when native trampolines cannot be
  installed during bootstrap.


The Bigger Picture
------------------

I might be wrong with my following litany, after all things have been
working fine for quite some time.  In that case I apologize in advance
to whom I'll suspect in wrong-doing.  But to me it seems that things are
working only "by chance" and without a, well, bigger picture.

So for me the bigger picture or problem is that it is not always enough
to rely on a simple `(symbol-function 'foo)' when you want to process
function `foo'.  It is enough if you just want to call it, but not if
you want to do fancy things with it, like compiling it.

In these fancy cases you might really want to dig through all possibly
existing advices on `foo' and process the _original_ function instead.

One instance is the detection of primitives in
`comp-call-optim-form-call', which I have tried to address above.
Another might be the following (on emacs master, without my fixes):

------------------------- snip -------------------------
(defun bar () nil)
(advice-add 'bar :before 'ignore)
(native-compile 'bar)

=>

comp--native-compile: Native compiler error: bar, "can't native compile an already byte-compiled function"
------------------------- snip -------------------------

Here `native-compile' falsely (?) detects and reports `bar' as already
byte-compiled, again because it just inspects `(symbol-function
function-name)' in `comp-spill-lap-function ((function-name symbol))'.

However, here the solution is not as simple as just slapping
`advice--cd*r' onto the problem, since function `byte-compile' *also*
recognizes the advised `bar' as "already compiled":

------------------------- snip -------------------------
(defun bar () nil)
(advice-add 'bar :before 'ignore)
(byte-compile 'bar)

=>

Function bar is already compiled
------------------------- snip -------------------------

When you slap `advice--cd*r' onto *that* problem as well, function `bar'
both natively compiles and byte-compiles even if advised.

Another suspectible spot is the following from function `Ffset':

  if (!NILP (Vnative_comp_enable_subr_trampolines)
      && SUBRP (function)
      && !SUBR_NATIVE_COMPILEDP (function))
    CALLN (Ffuncall, Qcomp_subr_trampoline_install, symbol);

Here FUNCTION would not be recognized as primitive once advised.  This
is not a problem if FUNCTION is properly recognized as a primitive
*before* it is advised for the first time.  But if that moment gets
missed, you first have to remove all advices before a trampoline is
generated on the next advice addition.

I wanted to demonstrate that on an unpatched master Emacs by tweaking
variable `native-comp-enable-subr-trampoline' in the right fashion, but
failed to do so: In that scenario `advice--add-function' also calls
`comp-subr-trampoline-install'!  And it does so even if
`native-comp-enable-subr-trampoline' equals nil!  So probably Andrea
added that second unconditional call to `comp-subr-trampoline-install'
as a second line of defense for "funny cases".

Which would be OK on the one side, but also problematical on the other,
since with `native-comp-enable-subr-trampoline' equaling nil no
trampolines should be generated according to the docstring of that
variable.

Oh my.

What do you think?

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

[Andrea Corallo]

Jens Schmidt <jschmidt4gnu@vodafonemail.de> writes:

> Thanks to both of you for taking the time to read through this!  I tried
> my best to combine both replies in one mail.
>
>>>>>> "SM" == Stefan Monnier <monnier@iro.umontreal.ca> writes:
>>>>>> "AC" == Andrea Corallo <acorallo@gnu.org> writes:
>
>  >> 1. Since Stefan has removed the advices on uniquify functions in
>  >>    1a724cc2d2e7, that special handling of `rename-buffer' in
>  >>    `advice--add-function' and `native-comp-never-optimize-functions'
>  >>    seems to be obsolete.  Away with it:
>
>  SM> +1
>
>  AC> +1
>
>
>  >> 2. The comment in `advice--add-function' says that `macroexpand'
>  >>    causes a circular dependency, and there *are* still advices done
>  >>    on `macroexpand' (at least in `cl-symbol-macrolet').  But
>  >>    probably these are not executed during the bootstrap?  Let's try.
>
>  SM> +1
>
>  AC> +1
>
>
>  >> 3. But then on the other hand, function Ffset also calls
>  >>    `comp-subr-trampoline-install', and if we have condition
>  >>
>  >>      `(subr-primitive-p (gv-deref ref))'
>  >>
>  >>    fulfilled in `advice--add-function', then the following `(setf
>  >>    (gv-deref ref) ...)' should ultimately call `fset'.  So probably
>  >>    we can completely remove that `comp-subr-trampoline-install' call
>  >>    from `advice--add-function', like this:
>
>  SM> It looks good to me, tho I must admit I do not understand why we
>  SM> have this `comp-subr-trampoline-install` in addition to the one in
>  SM> `Ffset`, so maybe I'm missing something.
>
>  AC> Unfortanatelly I can't remember why we have this specific handling,
>  AC> I guess there was a reason but anyway... If there is still a good
>  AC> reason we'll discover it.
>
> I might have found a reason ... see section "The Bigger Picture" below.
>
>
>  >> 5. And actually I think that b) above probably is a bug: It means
>  >>    that for advised subrs, the optimzation to indirect calls in
>  >>    function `comp-call-optim-form-call' never takes place.
>  >>
>  >>    So I tried to recognize subrs in function
>  >>    `comp-call-optim-form-call' even when they are advised:
>
>  SM> I think this is not a correct optimization, as you mention:
>
>  >>    But: Natively compiled code now does NOT execute the advice on
>  >>    `rename-buffer' added in step 4, since its call gets optimized
>  >>    but no trampoline has been created for it.
>
>  SM> So it's a -1 from me on this patch.
>
>  AC> It's good we optimize the call but we can't apply this patch if the
>  AC> this issue is not solved.
>
> Please let me try to address your concerns.  TL;DR:
>
> - If `rename-buffer' is advised *during* bootstrap (as with patch 4), no
>   native trampoline gets created for it because of
>   `native-comp-enable-subr-trampolines' being nil during loadup.el.
>
> - Without native trampoline, the fix that I propose in patch 5 lead to
>   the advice not being called.
>
> However:
>
> - If `rename-buffer' (or any other primitive) is advised *after*
>   bootstrap, a trampoline gets properly created and the advice is being
>   called even with my fix in patch 5.
>
> Full version: See section "More Background Information ..."
>
>
>  >> 6. So there is the question whether we should actively disallow
>  >>    advices during bootstrap, now that we are free of them.  Like
>  >>    this:
>
>  AC> Others can have different opinions but if it's not a big limitation
>  AC> for the future it sounds like a good idea to me.
>
>  SM> Rather than `dump-mode`, I'd test `purify-flag`.
>  SM> This is because `purify-flag` is not set while building
>  SM> `src/bootstrap-emacs`.
>
>  SM> Part of the issue here is that during the build of
>  SM> `src/bootstrap-emacs` we load a lot more ELisp code than in the
>  SM> "real" build so it's good to restrict this constraint to the "real
>  SM> build".
>
> Will do.
>
>
> More Background Information on that Suspicous Fix ...
> -----------------------------------------------------
>
> Now for more background on why I think the current way of detecting
> primitives in function `comp-call-optim-form-call' is buggy.  I provide
> that background not to lecture anybody, but to make my reasoning
> hopefully more transparent.
>
> There are two different items in the native compilation area called
> "trampoline":
>
> - There are "funcall trampolines", which are instructions to call a
>   function from natively compiled code through a `funcall' indirection.
>   A call of a primitive through such a funcall trampoline is pretty
>   close to a function call of that primitive from interpreted Elisp and
>   always executes advices.
>
> - And there are "native trampolines", which are natively compiled
>   wrappers for primitives.  If and only if a primitive has such a native
>   trampoline, advices on the primitive are executed even if the
>   primitive is called from natively compiled code *without* a funcall
>   trampoline.
>
> To see the difference, start with a vanilla master emacs, without any of
> my patches, and rewrite the function `comp-call-optim' with extended
> logging to a file test.el as follows:
>
> ------------------------- test.el -------------------------
> (with-eval-after-load 'comp
> (defun comp-call-optim (_)
>   "Try to optimize out funcall trampoline usage when possible."
>   (maphash (lambda (_ f)
>              (when (and (>= (comp-func-speed f) 2)
>                         (comp-func-l-p f))
>                (let ((comp-func f))
>                  (when (eq (comp-func-name f) 'foo)
>                    (comp-log "\nPre comp-call-optim" 0)
>                    (comp-log-func f 0))
>                  (comp-call-optim-func)
>                  (when (eq (comp-func-name f) 'foo)
>                    (comp-log "\nPost comp-call-optim" 0)
>                    (comp-log-func f 0)))))
>            (comp-ctxt-funcs-h comp-ctxt))))
> ------------------------- test.el -------------------------
>
> And the following to a file foo.el:
>
> ------------------------- foo.el -------------------------
> ;;; -*- lexical-binding: t -*-
> (defun foo (x)
>   (sqrt x))
> ------------------------- foo.el -------------------------
>
> Then start "./src/emacs -Q -l test.el" and evaluate the following in
> *scratch*:
>
> ------------------------- snip(A) -------------------------
> (with-current-buffer
>     (get-buffer-create "*Native-compile-Log*")
>   (setq buffer-read-only nil)
>   (erase-buffer))
> (load-file (native-compile "foo.el"))
> (pop-to-buffer "*Native-compile-Log*")
> ------------------------- snip(A) -------------------------
>
> The interesting part in the resulting *Native-compile-Log* is how the
> call to `sqrt' in function `foo' gets optimized.  (I chose that as
> example primitive because it is not as often called as `rename-buffer'
> and causes less noise during tests.)
>
> ------------------------- LIMPEL -------------------------
> Pre comp-call-optim
>
> (set #(mvar 23578551540518 1 float) (callref funcall #(mvar 23578553446684 1 (member sqrt)) #(mvar 23578554498624 2 t)))
>                                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> Post comp-call-optim
>
> (set #(mvar 23578551540518 1 float) (call sqrt #(mvar 23578554498624 2 t)))
>                                      ^^^^
> ------------------------- LIMPEL -------------------------
>
> So the indirect `callref funcall' got optimized to a direct `call', and:
> An advice on `sqrt' is executed even if the natively compiled `foo' is
> called:
>
> ------------------------- snip(B) -------------------------
> (defun sqrt-advice (&rest _)
>   (message "Advice called"))
> (advice-add #'sqrt :before #'sqrt-advice)
> (foo 0)
>
> => "Advice called" echoed in message area
> ------------------------- snip(B) -------------------------
>
> Now re-evaluate the snip(A) again and see how `foo' is *not* optimized,
> this time because of the advice on `sqrt' and because of what I consider
> the bug in `comp-call-optim-form-call':
>
> ------------------------- LIMPEL -------------------------
> Pre comp-call-optim
>
> (set #(mvar 23487733705560 1 float) (callref funcall #(mvar 23487733185614 1 (member sqrt)) #(mvar 23487733838350 2 t)))
>                                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> Post comp-call-optim
>
> (set #(mvar 23487733705560 1 float) (callref funcall #(mvar 23487733185614 1 (member sqrt)) #(mvar 23487733838350 2 t)))
>                                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> ------------------------- LIMPEL -------------------------
>
>
> To summarize the current, unpatched master behavior:
>
> - The result of native compilation of a call to a primitive depends on
>   whether the primitive is advised or not.

That's correct.

> - If it is not advised, the result is an optimized call that executes
>   advices if a native trampoline exists.

If it's installed I'd say.

> - If it is advised, the result is a non-optimized call (that always
>   executes advices).

Correct.

IIRC I have not considered this a big deal for two reasons.

1- If one advised a primitives is probably not very much interested in
having it called directly from native code for performance reasons.

2- Compilation happens in the vast majority of the time as in a async
process where the primitive is not advised anyway.

That said I agree would be nice to fix this corner case in order to have
the compiler producing reproducible elns.

> In my opinion, we should instead:
>
> - always use optimized calls, unconditionally whether an advice exists
>   at the time of the compilation or not;

Agreed

> - ensure that native trampolines get installed when a primitive is
>   advised;

Agreed

> - ensure that advices are ruled out when native trampolines cannot be
>   installed during bootstrap.

This would simply things so I like the idea, not sure it's acceptable
tho.

>
> The Bigger Picture
> ------------------
>
> I might be wrong with my following litany, after all things have been
> working fine for quite some time.  In that case I apologize in advance
> to whom I'll suspect in wrong-doing.  But to me it seems that things are
> working only "by chance" and without a, well, bigger picture.
>
> So for me the bigger picture or problem is that it is not always enough
> to rely on a simple `(symbol-function 'foo)' when you want to process
> function `foo'.  It is enough if you just want to call it, but not if
> you want to do fancy things with it, like compiling it.
>
> In these fancy cases you might really want to dig through all possibly
> existing advices on `foo' and process the _original_ function instead.

Yep

> One instance is the detection of primitives in
> `comp-call-optim-form-call', which I have tried to address above.
> Another might be the following (on emacs master, without my fixes):
>
> ------------------------- snip -------------------------
> (defun bar () nil)
> (advice-add 'bar :before 'ignore)
> (native-compile 'bar)
>
> =>
>
> comp--native-compile: Native compiler error: bar, "can't native compile an already byte-compiled function"
> ------------------------- snip -------------------------
>
> Here `native-compile' falsely (?) detects and reports `bar' as already
> byte-compiled, again because it just inspects `(symbol-function
> function-name)' in `comp-spill-lap-function ((function-name symbol))'.
>
> However, here the solution is not as simple as just slapping
> `advice--cd*r' onto the problem, since function `byte-compile' *also*
> recognizes the advised `bar' as "already compiled":

I think this is unrelared to this bug report.  Probably we should have a
dedicated bug report to discuss if this is a real bug o not.

Thanks!

  Andrea

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

[Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Thanks again ...

Andrea Corallo <acorallo@gnu.org> writes:

> [...]

I will prepare a new patch according to what has been discussed so far.
Unless Stefan or anybody else objects.

Stefan, Andrea has already hinted on this in his first mail: I'd need to
call `advice--cd*r' outside of its home package.  For the other issue(s)
it might also be useful to have `advice--p' available.  For example, to
detect advices before actually processing them where they shouldn't be
processed and report more meaningful errors.

Should I just use the double-dash functions or should I "export" them
first in nadvice.el?  If yes to "export", how would that be done?  Just
define a single-dash alias and improve docstrings, if required?

> I think this is unrelared to this bug report.  Probably we should have a
> dedicated bug report to discuss if this is a real bug o not.

Agreed, will do.

Thanks.

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> There are two different items in the native compilation area called
> "trampoline":
>
> - There are "funcall trampolines", which are instructions to call a
>   function from natively compiled code through a `funcall' indirection.
>   A call of a primitive through such a funcall trampoline is pretty
>   close to a function call of that primitive from interpreted Elisp and
>   always executes advices.

IIUC this is not a separate piece of code, tho.
It's just the use of the "normal" `funcall` instead of calling
a `subr` directly.

E.g. this is not something installed with `comp-subr-trampoline-install`.

> - And there are "native trampolines", which are natively compiled
>   wrappers for primitives.  If and only if a primitive has such a native
>   trampoline, advices on the primitive are executed even if the
>   primitive is called from natively compiled code *without* a funcall
>   trampoline.

And IIUC these are the thingies manipulated/generated/installed with
`comp-subr-trampoline-install`.

> ------------------------- LIMPEL -------------------------
> Pre comp-call-optim
>
> (set #(mvar 23578551540518 1 float) (callref funcall #(mvar 23578553446684 1 (member sqrt)) #(mvar 23578554498624 2 t)))
>                                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

IIUC this generates a piece of native call which will use `Ffuncall`
passing it the symbol `sqrt`, so it will always behave exactly like
the byte-compiler would.

> Post comp-call-optim
>
> (set #(mvar 23578551540518 1 float) (call sqrt #(mvar 23578554498624 2 t)))
>                                      ^^^^
> ------------------------- LIMPEL -------------------------

This instead generates a piece of code which calls the `Fsqrt` function
via a C-level function pointer kept in a `link_table`.

> So the indirect `callref funcall' got optimized to a direct `call', and:
> An advice on `sqrt' is executed even if the natively compiled `foo' is
> called:
>
> ------------------------- snip(B) -------------------------
> (defun sqrt-advice (&rest _)
>   (message "Advice called"))
> (advice-add #'sqrt :before #'sqrt-advice)
> (foo 0)
>
> => "Advice called" echoed in message area
> ------------------------- snip(B) -------------------------

Indeed, when an advice is installed `comp-subr-trampoline-install` is
called which replaces the pointer to `Fsqrt` to a pointer to
a "trampoline" which calls `Ffuncall` with `sqrt` as argument instead.

> Now re-evaluate the snip(A) again and see how `foo' is *not* optimized,
> this time because of the advice on `sqrt' and because of what I consider
> the bug in `comp-call-optim-form-call':
>
> ------------------------- LIMPEL -------------------------
> Pre comp-call-optim
>
> (set #(mvar 23487733705560 1 float) (callref funcall #(mvar 23487733185614 1 (member sqrt)) #(mvar 23487733838350 2 t)))
>                                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> Post comp-call-optim
>
> (set #(mvar 23487733705560 1 float) (callref funcall #(mvar 23487733185614 1 (member sqrt)) #(mvar 23487733838350 2 t)))
>                                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

This seems OK: if there's an advice when the code is compiled, there's
a good chance that the advice will also be present when the code is
executed, in which case it's more efficient to use a straight `funcall`
than a "direct" call that gets redirected to `funcall`.

BTW, this direct call and trampoline business becomes even more fun when
you consider things like:

    (defun my-fun1 ...)
    (defalias 'my-fun2 (symbol-function 'my-fun1))

in which case both `my-fun1` and `my-fun2` will contain the same subr,
so you'd ideally want to generate direct calls both for calls to
`my-fun1` and for calls to `my-fun2` but they can't use the same slot in
the `link_table` because they have to obey separately to redefinitions
`my-fun1` or `my-fun2`.

> So for me the bigger picture or problem is that it is not always enough
> to rely on a simple `(symbol-function 'foo)' when you want to process
> function `foo'.  It is enough if you just want to call it, but not if
> you want to do fancy things with it, like compiling it.

Hmm... in the example above, we don't want to compile it, we just want
to generate an efficient call to it.

> In these fancy cases you might really want to dig through all possibly
> existing advices on `foo' and process the _original_ function instead.

While it could occasionally be beneficial, I think in more cases it'll
be detrimental.  And in any case this is a very rare occurrence so the
choice probably doesn't actually matter at all in practice.

> ------------------------- snip -------------------------
> (defun bar () nil)
> (advice-add 'bar :before 'ignore)
> (native-compile 'bar)
>
> =>
>
> comp--native-compile: Native compiler error: bar, "can't native compile an already byte-compiled function"
> ------------------------- snip -------------------------

Here the question is: what do you mean by (native-compile 'bar)?
Do you mean to compile the advice wrapper (a byte-compiled function)
or some of its content (and if so which one(s))?

We could tweak `native-compile` and `byte-compile` so they look through
pieces of advice to try and find the underlying not-yet-compiled
functions, compile them and re-install them where they were found, but
obviously, noone asked for that until now and hence noone bothered to do
that work, most likely because things like (native-compile 'bar) and
(byte-compile 'bar) are operations used extremely rarely.

> Another suspectible spot is the following from function `Ffset':
>
>   if (!NILP (Vnative_comp_enable_subr_trampolines)
>       && SUBRP (function)
>       && !SUBR_NATIVE_COMPILEDP (function))
>     CALLN (Ffuncall, Qcomp_subr_trampoline_install, symbol);

Actually, I don't understand this code now that I re-(re-)*read it.
Why do we negate the SUBR_NATIVE_COMPILEDP (function)?


        Stefan

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

[Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> [...]

"Yes, I think you're right" to all your previous statements.

> This seems OK: if there's an advice when the code is compiled, there's
> a good chance that the advice will also be present when the code is
> executed, in which case it's more efficient to use a straight `funcall`
> than a "direct" call that gets redirected to `funcall`.

Interesting point.  I thought a native trampoline would be somehow more
efficient than a straight `funcall'.  I agree now to your "-1".

>> ------------------------- snip -------------------------
>> (defun bar () nil)
>> (advice-add 'bar :before 'ignore)
>> (native-compile 'bar)
>>
>> =>
>>
>> comp--native-compile: Native compiler error: bar, "can't native
>> compile an already byte-compiled function"
>> ------------------------- snip -------------------------

> We could tweak `native-compile` and `byte-compile` so they look through
> pieces of advice to try and find the underlying not-yet-compiled
> functions, compile them and re-install them where they were found, but
> obviously, noone asked for that until now and hence noone bothered to do
> that work, most likely because things like (native-compile 'bar) and
> (byte-compile 'bar) are operations used extremely rarely.

TBH, I think it would be sufficient and nice to throw at least a more
meaningful error message along the lines of "(native|byte) compilation
of advised functions not supported".  Subject to a different bug?

>> Another suspectible spot is the following from function `Ffset':
>>
>>   if (!NILP (Vnative_comp_enable_subr_trampolines)
>>       && SUBRP (function)
>>       && !SUBR_NATIVE_COMPILEDP (function))
>>     CALLN (Ffuncall, Qcomp_subr_trampoline_install, symbol);
>
> Actually, I don't understand this code now that I re-(re-)*read it.
> Why do we negate the SUBR_NATIVE_COMPILEDP (function)?

Do you mean "why do we negate it" or "why do we have it at all"?  As for
the negation, we have

  SUBR_NATIVE_COMPILEDP (Lisp_Object a)
  {
    return SUBRP (a) && !NILP (XSUBR (a)->native_comp_u);
  }

so a condition

  !NILP (Vnative_comp_enable_subr_trampolines)
  && SUBRP (function)
  && SUBR_NATIVE_COMPILEDP (function))

does not make much sense.

As for the existence of "!SUBR_NATIVE_COMPILEDP" I'll try an answer, but
this is of course again Andrea's domain ... I guess it mirrors the
condition

        (cond
         ((and subrp (not (subr-native-elisp-p f)))
          ;; Trampoline removal.

from `comp-call-optim-form-call'.

But probably your question was why we distinguish in *both* these places
between primitives and native compiled Elisp?  FWIW, I tried removing
both conditions and got errors when making bootstrap:

  Error: native-ice ("../lisp/emacs-lisp/macroexp.el" "missing function
  declaration" byte-compile-constant)

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

[Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1: Type: text/plain, Size: 2898 bytes --]

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
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?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Improve-handling-of-advices-and-trampoline-generatio.patch --]
[-- Type: text/x-diff, Size: 3344 bytes --]

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

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>  (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.

I'm not sure I like the idea of keeping the whole history in comments.
I suggest you try and trim it down to the part that seems likely
to reoccur, like "We used to list those functions that were advised
during preload but we now prefer to disallow them in `advice-add`".

> In `addvice--add-function' I wanted to at least preserve the comment
> from my initial patch (see attachment to
> 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.

`C-x v h` is your friend.  It was weird in the fist place to put the
trampoline stuff there (e.g. it's specific to functions stored in symbols so
it would have been more logical to put it into `advice-add` instead), so
it doesn't seem very likely that this will re-occur.

>  <<>>"
> +  ;; 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.

I don't think this last point is true/relevant, is it?
IIUC it would use a "normal funcall", which doesn't need a trampoline.

>    ;; 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?

Beside the comments about the comments, it's a +1 from me.


        Stefan

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

[Andrea Corallo]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>  (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.
>
> I'm not sure I like the idea of keeping the whole history in comments.
> I suggest you try and trim it down to the part that seems likely
> to reoccur, like "We used to list those functions that were advised
> during preload but we now prefer to disallow them in `advice-add`".
>
>> In `addvice--add-function' I wanted to at least preserve the comment
>> from my initial patch (see attachment to
>> 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.
>
> `C-x v h` is your friend.  It was weird in the fist place to put the
> trampoline stuff there (e.g. it's specific to functions stored in symbols so
> it would have been more logical to put it into `advice-add` instead), so
> it doesn't seem very likely that this will re-occur.
>
>>  <<>>"
>> +  ;; 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.
>
> I don't think this last point is true/relevant, is it?
> IIUC it would use a "normal funcall", which doesn't need a trampoline.
>
>>    ;; 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?
>
> Beside the comments about the comments, it's a +1 from me.

I agree with all the comments made by Stefan, with those implemented +1
for me as well.

Thanks

  Andrea

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

[Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Thanks, Stefan and Andrea, for your review.

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>  (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.
>
> I'm not sure I like the idea of keeping the whole history in comments.
> I suggest you try and trim it down to the part that seems likely
> to reoccur, like "We used to list those functions that were advised
> during preload but we now prefer to disallow them in `advice-add`".

Ok.

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?

>> In `addvice--add-function' I wanted to at least preserve the comment
>> from my initial patch (see attachment to
>> 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.
>
> `C-x v h` is your friend.  It was weird in the fist place to put the
> trampoline stuff there (e.g. it's specific to functions stored in symbols so
> it would have been more logical to put it into `advice-add` instead), so
> it doesn't seem very likely that this will re-occur.

Also Ok.  `C-x v h` is my *new* friend.

>>  <<>>"
>> +  ;; 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.
>
> I don't think this last point is true/relevant, is it?
> IIUC it would use a "normal funcall", which doesn't need a trampoline.

Yup, I got it messed up, again.  BTW, it was this test in function
`Fcomp__install_trampoline' which put me on the wrong track:

  /* FIXME: add a post dump load trampoline machinery to remove this
     check.  */
  if (will_dump_p ())
    signal_error ("Trying to advice unexpected primitive before dumping",
		  subr_name);

I think during normal bootstrap this test should never fire, since this
function never should be called, since its call is protected by
`native-comp-enable-subr-trampolines'.

>>    ;; 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?
>
> Beside the comments about the comments, it's a +1 from me.

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?  Along the lines of `subrp' not working as
one might expect on advised primitives, which is exactly what we need
here ...

Will provide a new patch.

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> 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").

> 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.


        Stefan

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

[Andrea Corallo]

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").

Good to know I updated it since I changed the vaule first. Thanks Jens
for noticing that.

  Andrea

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>>> Only slightly off-topic: When changing the default value of user option
>>> `native-comp-never-optimize-functions', which now both Andrea and I have
[...]
> Good to know I updated it since I changed the vaule first. Thanks Jens
> for noticing that.

BTW, why is it a `defcustom` rather than a `defvar`?
Do we really expect "normal users" to change it?


        Stefan

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

[Eli Zaretskii]

> Cc: Jens Schmidt <jschmidt4gnu@vodafonemail.de>, 67005@debbugs.gnu.org
> Date: Wed, 22 Nov 2023 11:07:38 -0500
> From:  Stefan Monnier via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> >>> Only slightly off-topic: When changing the default value of user option
> >>> `native-comp-never-optimize-functions', which now both Andrea and I have
> [...]
> > Good to know I updated it since I changed the vaule first. Thanks Jens
> > for noticing that.
> 
> BTW, why is it a `defcustom` rather than a `defvar`?
> Do we really expect "normal users" to change it?

Yes, we do.  I think they actually did already, maybe Andrea remembers
which functions and why.

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

[Andrea Corallo]

Eli Zaretskii <eliz@gnu.org> writes:

>> Cc: Jens Schmidt <jschmidt4gnu@vodafonemail.de>, 67005@debbugs.gnu.org
>> Date: Wed, 22 Nov 2023 11:07:38 -0500
>> From:  Stefan Monnier via "Bug reports for GNU Emacs,
>>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>> 
>> >>> Only slightly off-topic: When changing the default value of user option
>> >>> `native-comp-never-optimize-functions', which now both Andrea and I have
>> [...]
>> > Good to know I updated it since I changed the vaule first. Thanks Jens
>> > for noticing that.
>> 
>> BTW, why is it a `defcustom` rather than a `defvar`?
>> Do we really expect "normal users" to change it?
>
> Yes, we do.  I think they actually did already, maybe Andrea remembers
> which functions and why.

I remember as well they did it, but can't remember precisely the reason.
Actually I suspect it might have been before we had the trampoline
system up and running.  ATM I can't picture any reason why a user would
like to use this customize other than improving a specific backtrack or
preventing a certain trampoline to be created, but indeed I might miss
something.

  Andrea

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>>> BTW, why is it a `defcustom` rather than a `defvar`?
>>> Do we really expect "normal users" to change it?
>> Yes, we do.  I think they actually did already, maybe Andrea remembers
>> which functions and why.
> I remember as well they did it, but can't remember precisely the reason.
> Actually I suspect it might have been before we had the trampoline
> system up and running.  ATM I can't picture any reason why a user would
> like to use this customize other than improving a specific backtrack or
> preventing a certain trampoline to be created, but indeed I might miss
> something.

Part of the question is also: would those users benefit from using
Custom to set this var, instead of using a plain `add-to-list` or
`setq`?


        Stefan

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

[Andrea Corallo]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>>> BTW, why is it a `defcustom` rather than a `defvar`?
>>>> Do we really expect "normal users" to change it?
>>> Yes, we do.  I think they actually did already, maybe Andrea remembers
>>> which functions and why.
>> I remember as well they did it, but can't remember precisely the reason.
>> Actually I suspect it might have been before we had the trampoline
>> system up and running.  ATM I can't picture any reason why a user would
>> like to use this customize other than improving a specific backtrack or
>> preventing a certain trampoline to be created, but indeed I might miss
>> something.
>
> Part of the question is also: would those users benefit from using
> Custom to set this var, instead of using a plain `add-to-list` or
> `setq`?

I fear I dont' have an answer to this.  Another question would be what's
the downside of keep on having it as custom variable?

Thanks

  Andrea

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

[Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1: Type: text/plain, Size: 1069 bytes --]

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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Improve-handling-of-advices-and-trampoline-generatio.patch --]
[-- Type: text/x-diff, Size: 3417 bytes --]

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.
+  (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

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

[Eli Zaretskii]

> Cc: andrea corallo <acorallo@gnu.org>, 67005@debbugs.gnu.org
> Date: Wed, 22 Nov 2023 23:01:44 +0100
> From:  Jens Schmidt via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> 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.

Does this patch resolve the 2 TODO items you are now deleting?  Or
were they already resolved by previous changes, and we just failed to
keep the comments up-to-date?

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

[Andrea Corallo]

Eli Zaretskii <eliz@gnu.org> writes:

>> Cc: andrea corallo <acorallo@gnu.org>, 67005@debbugs.gnu.org
>> Date: Wed, 22 Nov 2023 23:01:44 +0100
>> From:  Jens Schmidt via "Bug reports for GNU Emacs,
>>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>> 
>> 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.
>
> Does this patch resolve the 2 TODO items you are now deleting?  Or
> were they already resolved by previous changes, and we just failed to
> keep the comments up-to-date?

I'd say the second.

Regards

  Andrea

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

[Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Andrea Corallo <acorallo@gnu.org> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>> Does this patch resolve the 2 TODO items you are now deleting?  Or
>> were they already resolved by previous changes, and we just failed to
>> keep the comments up-to-date?
>
> I'd say the second.

Exactly.  Sorry for not mentioning.  My reasoning for removing these
TODO items was as follows, easy one (?) first:

>> -  ;; - obsolete advice.el.

- The main entry point to "advice.el", macro `defadvice', is declared
  obsolete as follows:

  (declare (doc-string 3) (indent 2)
           (obsolete "use `advice-add' or `define-advice'" "30.1")

- The Elisp manual mentions that as well in "(elisp) Porting Old
  Advice".

- Users are complaining about obsoletion on emacs-devel (but that is
  probably not a good criterion).

>> -  ;; - change all defadvice in lisp/**/*.el.

This one is a bit harder.  So first a bit of find and grep, commented
and lacking the required line continuation for better readability:

  find . -type f
       ! -path ./lisp/emacs-lisp/advice.el
       # testing defadvice compatibility
       ! -path ./test/lisp/emacs-lisp/nadvice-tests.el
       # generated files
       ! -path ./lisp/TAGS
       ! -path ./lisp/ldefs-boot.el
       ! -path ./lisp/loaddefs.el
       ! -name '*.info'
       ! -name '*.el[cn]'
       ! -name '*.pdmp'
       # collateral
       ! -name '*ChangeLog*'
       ! -name '*NEWS*'
       -exec grep --color=auto -nH --null -e defadvice '{}' +

Then I manually traced the results to find references to old advices,
where they are not explicitly referred to as old ones.  There are some
such references in comments, docstrings, font lock defs, etc., and some
of these probably should even be cared about.  But there are only two
instances where an advice is defined by means of `defadvice':

  # I didn't count that one as living in lisp/**/*.el
  ./test/manual/cedet/tests/test.el:75

  # this one is explicitly tagged as backward compatibility hack for
  # "Emacs < 22 and XEmacs"
  ./lisp/progmodes/cc-mode.el:2733

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

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> +  (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?

Since `defadvice` is now marked as obsolete, it seems highly unlikely
that we'd end up adding an old-style advice to a preloaded file.

But this reminds me that some people do their own preload/dump,
sometimes with a crapload of extra packages, in which case the
probability that some of them use advice-add is rather high.
IIUC the the above `error` is not technically indispensable (the code
will still work mostly right, beside some issues about the docstrings
that affect only actual C-code subrs and not native-compiled subrs), so
it would be better to demote the above `error` to a warning.


        Stefan

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>>> +  (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?
>
> Since `defadvice` is now marked as obsolete, it seems highly unlikely
> that we'd end up adding an old-style advice to a preloaded file.

More to the point: no it's not necessary since `advice.el` uses
`advice-add` to install its pieces of advice, so the test in
`nadvice.el` already covers it.


        Stefan

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

[Andrea Corallo]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>>> +  (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?
>>
>> Since `defadvice` is now marked as obsolete, it seems highly unlikely
>> that we'd end up adding an old-style advice to a preloaded file.
>
> More to the point: no it's not necessary since `advice.el` uses
> `advice-add` to install its pieces of advice, so the test in
> `nadvice.el` already covers it.

Great thanks for clarifying.

Regards

  Andrea

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

[Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> But this reminds me that some people do their own preload/dump,
> sometimes with a crapload of extra packages, in which case the
> probability that some of them use advice-add is rather high.

Ok.

> IIUC the the above `error` is not technically indispensable (the code
> will still work mostly right, beside some issues about the docstrings
> that affect only actual C-code subrs and not native-compiled subrs), so
> it would be better to demote the above `error` to a warning.

Eh, just used the first function that came to my mind, `warn', with the
following result:

  Error: error ("Attempt to autoload warn while preparing to dump")
    mapbacktrace(#[1028 "\1\4\203\24\0\301\302!\210\300\4!\210\301\303!\210\202\35\0\301\304!\210\3\3B\262\1\211\2035\0\300\1@!\210\211A\211\262\2\2035\0\301\305!\210\202!\0\301\306!\207" [prin1 princ "  " "(" "  (" " " ")\n"] 7 "\n\n(fn EVALD FUNC ARGS FLAGS)"])
    debug-early-backtrace()
    debug-early(error (error "Attempt to autoload warn while preparing to dump"))
    warn("Invalid pre-dump advice on %s" emacs-lisp-byte-compile)
    advice-add(emacs-lisp-byte-compile :before ignore)
    load("progmodes/elisp-mode")
    load("loadup.el")

How would one warn during bootstrap?  And would this be noticed?  Or do
we have a chance to distinguish Emacs-only builds from those with extra
packages?

Thanks.

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> How would one warn during bootstrap?

`message`

> And would this be noticed?

Yes.  The build should not emit any warning, so unless the warning is
particularly discrete, we do notice it.


        Stefan

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

[Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> How would one warn during bootstrap?
>
> `message`

That simple.

>> And would this be noticed?
>
> Yes.  The build should not emit any warning, so unless the warning is
> particularly discrete, we do notice it.

What about this one?  Too indiscrete?

  (when purify-flag
    (message "\a\nWarning: Invalid pre-dump advice on %s\n" symbol))


Plus another idea: Instead of placing the test into `advice-add', how
about checking functions for advices in loadup.el?  We could place that
check just before site-load.el gets loaded, to avoid trouble for Emacs
modders.  I tried the following, and it seemed to do the trick:

diff --git a/lisp/loadup.el b/lisp/loadup.el
index 07895228d0d..49d675dc5cb 100644
--- a/lisp/loadup.el
+++ b/lisp/loadup.el
@@ -393,6 +393,11 @@
 ;; from the repository.  It is generated just after temacs is built.
 (load "leim/leim-list.el" t)

+(mapatoms
+ (lambda (f)
+   (and (advice--p (symbol-function f))
+        (error "Pre-dump advice on %s" f))))
+
 ;; If you want additional libraries to be preloaded and their
 ;; doc strings kept in the DOC file rather than in core,
 ;; you may load them with a "site-load.el" file.


Thanks.

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> What about this one?  Too indiscrete?
>
>   (when purify-flag
>     (message "\a\nWarning: Invalid pre-dump advice on %s\n" symbol))

That works.

> Plus another idea: Instead of placing the test into `advice-add', how
> about checking functions for advices in loadup.el?  We could place that
> check just before site-load.el gets loaded, to avoid trouble for Emacs
> modders.  I tried the following, and it seemed to do the trick:
>
> +(mapatoms
> + (lambda (f)
> +   (and (advice--p (symbol-function f))
> +        (error "Pre-dump advice on %s" f))))

Even better!


        Stefan

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

[Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1: Type: text/plain, Size: 639 bytes --]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> Plus another idea: Instead of placing the test into `advice-add', how
>> about checking functions for advices in loadup.el?  We could place that
>> check just before site-load.el gets loaded, to avoid trouble for Emacs
>> modders.  I tried the following, and it seemed to do the trick:
>>
>> +(mapatoms
>> + (lambda (f)
>> +   (and (advice--p (symbol-function f))
>> +        (error "Pre-dump advice on %s" f))))
>
> Even better!

Patch reworked accordingly, also to include the additional commit
message Andreas has proposed.  Please review.

Would that merit a NEWS entry?

Thanks.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Update-handling-of-advices-during-preload.patch --]
[-- Type: text/x-diff, Size: 3892 bytes --]

From fc68d3e9084ede1fb8a109084398aac9e16f2e80 Mon Sep 17 00:00:00 2001
From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
Date: Mon, 20 Nov 2023 23:42:01 +0100
Subject: [PATCH] Update handling of advices during preload

* 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): Remove references to TODOs
that were completed already earlier.
* lisp/loadup.el: Disallow advices during preload.  (Bug#67005)
---
 lisp/emacs-lisp/comp-common.el | 9 ++++-----
 lisp/emacs-lisp/comp.el        | 8 ++++++++
 lisp/emacs-lisp/nadvice.el     | 2 --
 lisp/loadup.el                 | 9 +++++++++
 4 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/lisp/emacs-lisp/comp-common.el b/lisp/emacs-lisp/comp-common.el
index 6d94d1bd82e..b7a685223ed 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
+  ;; loadup.el (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..9f2b42f5765 100644
--- a/lisp/emacs-lisp/nadvice.el
+++ b/lisp/emacs-lisp/nadvice.el
@@ -509,8 +509,6 @@ advice-add
 <<>>"
   ;; TODO:
   ;; - record the advice location, to display in describe-function.
-  ;; - change all defadvice in lisp/**/*.el.
-  ;; - obsolete advice.el.
   (let* ((f (symbol-function symbol))
 	 (nf (advice--normalize symbol f)))
     (unless (eq f nf) (fset symbol nf))
diff --git a/lisp/loadup.el b/lisp/loadup.el
index 07895228d0d..3b58d5fb9b7 100644
--- a/lisp/loadup.el
+++ b/lisp/loadup.el
@@ -393,6 +393,15 @@
 ;; from the repository.  It is generated just after temacs is built.
 (load "leim/leim-list.el" t)
 
+;; Actively disallow advised functions during preload since:
+;; - advices in Emacs's core are generally considered bad style;
+;; - `Snarf-documentation' looses docstrings of primitives advised
+;;   during preload (bug#66032#20).
+(mapatoms
+ (lambda (f)
+   (and (advice--p (symbol-function f))
+        (error "Preload advice on %s" f))))
+
 ;; If you want additional libraries to be preloaded and their
 ;; doc strings kept in the DOC file rather than in core,
 ;; you may load them with a "site-load.el" file.
-- 
2.30.2

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

[Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Jens Schmidt <jschmidt4gnu@vodafonemail.de> writes:

> Patch reworked accordingly, also to include the additional commit
> message Andreas has proposed.  Please review.

10 days (inclusive) have passed, so: Gentle Bump.  Is there anything
still missing here?

Thanks                                                          Jens

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

[Andrea Corallo]

Jens Schmidt <jschmidt4gnu@vodafonemail.de> writes:

> Jens Schmidt <jschmidt4gnu@vodafonemail.de> writes:
>
>> Patch reworked accordingly, also to include the additional commit
>> message Andreas has proposed.  Please review.
>
> 10 days (inclusive) have passed, so: Gentle Bump.  Is there anything
> still missing here?
>
> Thanks                                                          Jens

Hi Jens,

I've installed your patch into master with e670412a3e1.

I'm closing the bug as well, happy to re-poen if necessary.

Thanks

  Andrea

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

[Andy Moreton]

On Mon 04 Dec 2023, Andrea Corallo wrote:

> Jens Schmidt <jschmidt4gnu@vodafonemail.de> writes:
>
>> Jens Schmidt <jschmidt4gnu@vodafonemail.de> writes:
>>
>>> Patch reworked accordingly, also to include the additional commit
>>> message Andreas has proposed.  Please review.
>>
>> 10 days (inclusive) have passed, so: Gentle Bump.  Is there anything
>> still missing here?
>>
>> Thanks                                                          Jens
>
> Hi Jens,
>
> I've installed your patch into master with e670412a3e1.
>
> I'm closing the bug as well, happy to re-poen if necessary.
>
> Thanks
>
>   Andrea

Commit e670412a3e10 ("Update handling of advices during preload")
appears to break bootstrap on Windows with the 64bit mingw64
toolchain:

Error: error ("Preload advice on insert-directory")
  mapbacktrace((closure ((prin1 . cl-prin1) t) (evald func args _flags) (let ((args args)) (if evald (progn (princ "  ") (funcall prin1 func) (princ "(")) (progn (princ "  (\b
") (setq args (cons func args)))) (if args (while (progn (funcall prin1 (car args)) (setq args (cdr args)))(princ " "))) (princ ")\n"))))
  (let ((print-escape-newlines t) (print-escape-control-characters t) (print-escape-nonascii t) (prin1 (if (and (fboundp 'cl-prin1) (fboundp 'cl-defmethod) (condition-case  \bn
il (require 'cl-print) (error nil))) #'cl-prin1 #'prin1))) (mapbacktrace #'(lambda (evald func args _flags) (let ((args args)) (if evald (progn (princ "  ") (funcall prin1 f\b
unc) (princ "(")) (progn (princ "  (") (setq args (cons func args)))) (if args (while (progn (funcall prin1 (car args)) (setq args (cdr args))) (princ " "))) (princ ")\n")))\b
))
  debug-early-backtrace()
debug-early(error (error "Preload advice on insert-directory"))
  signal(error ("Preload advice on insert-directory"))
  error("Preload advice on %s" insert-directory)
  (and (advice--p (symbol-function f)) (error "Preload advice on %s" f))
  (closure (t) (f) (and (advice--p (symbol-function f)) (error "Preload advice on %s" f)))(insert-directory)
  mapatoms((closure(t) (f) (and (advice--p (symbol-function f)) (error "Preload advice on %s" f))))
  load("loadup.el")
Preload advice on insert-directory
make[2]: *** [Makefile:1014: bootstrap-emacs.pdmp] Error 127
make[1]: *** [Makefile:554: src] Error 2

Bootstrapping the same config with commit f5e45247081a ("comp: Fix mvar
dependency chain (bug#67239)") succeeds.

Both builds done from a clean checkout after "git clean -xdf".

    AndyM

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

[Andrea Corallo]

Andy Moreton <andrewjmoreton@gmail.com> writes:

> On Mon 04 Dec 2023, Andrea Corallo wrote:
>
>> Jens Schmidt <jschmidt4gnu@vodafonemail.de> writes:
>>
>>> Jens Schmidt <jschmidt4gnu@vodafonemail.de> writes:
>>>
>>>> Patch reworked accordingly, also to include the additional commit
>>>> message Andreas has proposed.  Please review.
>>>
>>> 10 days (inclusive) have passed, so: Gentle Bump.  Is there anything
>>> still missing here?
>>>
>>> Thanks                                                          Jens
>>
>> Hi Jens,
>>
>> I've installed your patch into master with e670412a3e1.
>>
>> I'm closing the bug as well, happy to re-poen if necessary.
>>
>> Thanks
>>
>>   Andrea
>
> Commit e670412a3e10 ("Update handling of advices during preload")
> appears to break bootstrap on Windows with the 64bit mingw64
> toolchain:
>
> Error: error ("Preload advice on insert-directory")
>   mapbacktrace((closure ((prin1 . cl-prin1) t) (evald func args _flags) (let ((args args)) (if evald (progn (princ "  ") (funcall prin1 func) (princ "(")) (progn (princ "  (\b
> ") (setq args (cons func args)))) (if args (while (progn (funcall prin1 (car args)) (setq args (cdr args)))(princ " "))) (princ ")\n"))))
>   (let ((print-escape-newlines t) (print-escape-control-characters t) (print-escape-nonascii t) (prin1 (if (and (fboundp 'cl-prin1) (fboundp 'cl-defmethod) (condition-case  \bn
> il (require 'cl-print) (error nil))) #'cl-prin1 #'prin1))) (mapbacktrace #'(lambda (evald func args _flags) (let ((args args)) (if evald (progn (princ "  ") (funcall prin1 f\b
> unc) (princ "(")) (progn (princ "  (") (setq args (cons func args)))) (if args (while (progn (funcall prin1 (car args)) (setq args (cdr args))) (princ " "))) (princ ")\n")))\b
> ))
>   debug-early-backtrace()
> debug-early(error (error "Preload advice on insert-directory"))
>   signal(error ("Preload advice on insert-directory"))
>   error("Preload advice on %s" insert-directory)
>   (and (advice--p (symbol-function f)) (error "Preload advice on %s" f))
>   (closure (t) (f) (and (advice--p (symbol-function f)) (error "Preload advice on %s" f)))(insert-directory)
>   mapatoms((closure(t) (f) (and (advice--p (symbol-function f)) (error "Preload advice on %s" f))))
>   load("loadup.el")
> Preload advice on insert-directory
> make[2]: *** [Makefile:1014: bootstrap-emacs.pdmp] Error 127
> make[1]: *** [Makefile:554: src] Error 2
>
> Bootstrapping the same config with commit f5e45247081a ("comp: Fix mvar
> dependency chain (bug#67239)") succeeds.
>
> Both builds done from a clean checkout after "git clean -xdf".
>
>     AndyM

Hi Andy,

thanks for reporting, is e670412a3e1 fixing this for you?

  Andrea

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

[Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Hi Andrea,

and thanks for comitting e670412a3e1 yesterday ...

Andrea Corallo <acorallo@gnu.org> writes:

> Hi Andy,
>
> thanks for reporting, is e670412a3e1 fixing this for you?
>
>   Andrea

... but that actually has introduced Andy's issue.  I have overlooked
two preload advices left in Emacs because I did not consider the
conditional `load' calls from loadup.el.  At least I had a second look
on loadup.el now and I think there aren't any other preload advices ...

Also thanks to Po for fixing that issue in 19a3b499f84.

We probably should leave this bug open for some more time.

                                                                Jens

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

[Andy Moreton]

On Tue 05 Dec 2023, Jens Schmidt via "Bug reports for GNU Emacs, the Swiss army knife of text editors" wrote:

> Hi Andrea,
>
> and thanks for comitting e670412a3e1 yesterday ...
>
> Andrea Corallo <acorallo@gnu.org> writes:
>
>> Hi Andy,
>>
>> thanks for reporting, is e670412a3e1 fixing this for you?
>>
>>   Andrea
>
> ... but that actually has introduced Andy's issue.  I have overlooked
> two preload advices left in Emacs because I did not consider the
> conditional `load' calls from loadup.el.  At least I had a second look
> on loadup.el now and I think there aren't any other preload advices ...
>
> Also thanks to Po for fixing that issue in 19a3b499f84.
>
> We probably should leave this bug open for some more time.
>
>                                                                 Jens

Hi,

Pulling from master and bootstrapping from commit dc744fe6f3cd ("ElDoc:
make eldoc-display-in-echo-are useful from M-x eldoc") was successful
for me with the same toolchain/config as previously.

It looks like the fix in commit 19a3b499f84b ("; * lisp/loadup.el: Don't
prohibit advice when ls-lisp is loaded.") has fixed this.

Thanks to all,

    AndyM

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

[Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

close 67005
thanks

Stefan has disarmed my patch by converting the `error' to a `message' in
commit 3b1fd42732f, so it should be save to close this now.

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

[Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Andrea Corallo <acorallo@gnu.org> writes:

> Jens Schmidt <jschmidt4gnu@vodafonemail.de> writes:
>
>> +  ;; 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.

Right, will do.

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

[Eli Zaretskii]

> Cc: andrea corallo <acorallo@gnu.org>, 67005@debbugs.gnu.org
> Date: Tue, 21 Nov 2023 23:10:52 +0100
> From:  Jens Schmidt via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> 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?

No, the :version tag must be changed manually in these cases.

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

[Andrea Corallo]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

[...]

>> Another suspectible spot is the following from function `Ffset':
>>
>>   if (!NILP (Vnative_comp_enable_subr_trampolines)
>>       && SUBRP (function)
>>       && !SUBR_NATIVE_COMPILEDP (function))
>>     CALLN (Ffuncall, Qcomp_subr_trampoline_install, symbol);
>
> Actually, I don't understand this code now that I re-(re-)*read it.
> Why do we negate the SUBR_NATIVE_COMPILEDP (function)?

"function" is the function we are redefining and we want to trigger the
trampoline thing only when that's a C primitive.  Those two conditions
are equivalent to 'subr-primitive-p'.

  Andrea

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> "function" is the function we are redefining and we want to trigger the
> trampoline thing only when that's a C primitive.  Those two conditions
> are equivalent to 'subr-primitive-p'.

Oh, right, because we don't implement that same optimization calls to
native-compiled functions, right?
Shouldn't we implement that optimization for calls to
native-compiled functions, tho?  At least for the case where that
function is defined in the same file as the call?


        Stefan

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

[Andrea Corallo]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> "function" is the function we are redefining and we want to trigger the
>> trampoline thing only when that's a C primitive.  Those two conditions
>> are equivalent to 'subr-primitive-p'.
>
> Oh, right, because we don't implement that same optimization calls to
> native-compiled functions, right?

That is correct

> Shouldn't we implement that optimization for calls to
> native-compiled functions, tho?  At least for the case where that
> function is defined in the same file as the call?

We could, we'd need a "link table" for each file.  Note that this would
make the backtrace even more sparse :)

We should add those good ideas into some TODO list to keep track of
them.

  Andrea

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> Shouldn't we implement that optimization for calls to
>> native-compiled functions, tho?  At least for the case where that
>> function is defined in the same file as the call?
>
> We could, we'd need a "link table" for each file.

Hmm... that's right.

> Note that this would make the backtrace even more sparse :)

You mean that it would push up the urgency to fix that bug?
Maybe fixing that bug could be used to simplify the trampolines
(because the symbol holding the function that we're calling would then
be available, so the trampoline could use it, making it possible to use
a fixed set of trampolines (one per calling convention) rather than one
per function).


        Stefan

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

[Andrea Corallo]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>> Shouldn't we implement that optimization for calls to
>>> native-compiled functions, tho?  At least for the case where that
>>> function is defined in the same file as the call?
>>
>> We could, we'd need a "link table" for each file.
>
> Hmm... that's right.
>
>> Note that this would make the backtrace even more sparse :)
>
> You mean that it would push up the urgency to fix that bug?
> Maybe fixing that bug could be used to simplify the trampolines
> (because the symbol holding the function that we're calling would then
> be available, so the trampoline could use it, making it possible to use
> a fixed set of trampolines (one per calling convention) rather than one
> per function).

So IIUC what you a resuggesting would be to pass the symbol holding the
function that we're calling to a generic (per calling convetion)
trampoline?

  Andrea

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> So IIUC what you a resuggesting would be to pass the symbol holding the
> function that we're calling to a generic (per calling convetion)
> trampoline?

I'm not suggesting anything in particular, really.
I was just pointing out that in order to fix the absence of the function
from the backtrace, we'll need to push the function's symbol onto the
backtrace, which means some piece of the code will now have access to
this symbol.  Depending on how it's done it might allow the trampoline
to find this symbol without having to hardcode it into its code, thus
making it possible to use a small set of precompiled trampolines.


        Stefan

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

[Eli Zaretskii]

> Cc: Jens Schmidt <jschmidt4gnu@vodafonemail.de>, 67005@debbugs.gnu.org
> From: Andrea Corallo <acorallo@gnu.org>
> Date: Fri, 17 Nov 2023 15:37:17 -0500
> 
> We should add those good ideas into some TODO list to keep track of
> them.

Please use etc/TODO for this purpose, adding a section there about
improvements in native-compilation stuff.  (There is already one such
item there, it should be moved to this new section.)

Thanks.

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

[Andrea Corallo]

Eli Zaretskii <eliz@gnu.org> writes:

>> Cc: Jens Schmidt <jschmidt4gnu@vodafonemail.de>, 67005@debbugs.gnu.org
>> From: Andrea Corallo <acorallo@gnu.org>
>> Date: Fri, 17 Nov 2023 15:37:17 -0500
>> 
>> We should add those good ideas into some TODO list to keep track of
>> them.
>
> Please use etc/TODO for this purpose, adding a section there about
> improvements in native-compilation stuff.  (There is already one such
> item there, it should be moved to this new section.)

Done thanks, I've in my todo to add more stuff to TODO now

Best Regards

  Andrea

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

[Andrea Corallo]

Jens Schmidt <jschmidt4gnu@vodafonemail.de> writes:

> X-Debbugs-Cc: Stefan Monnier <monnier@iro.umontreal.ca>, Andrea Corallo <acorallo@gnu.org>
> Severity: minor
>
>
> While digging around in nadvice.el, mainly still because of
> bug#66032, I came across the special-cased handling of
> `rename-buffer' and `macroexpand' in `advice--add-function'.
>>From there I came to `native-comp-never-optimize-functions' in
> comp.el.  So I started fiddeling away and came to the conclusion
> that these are probably no longer needed, and then I found more
> issues ... please check whether my reasoning below is correct.
>
> The attached patch is probably a nice TL;DR.
>
>
> Some notes:
>
> - Nothing described below is really a bug (except probably item
>   4b), but fixing these issues should improve long-term
>   maintainability, I think.
>
> - All patches have been built on (somewhat randomly chosen)
>   commit b819b8d6e90337b4cb36b35c2c6d0112c90a8e24, just to have a
>   reference that builds and tests fine on my system.
>
> - When I write "bootstrap & check: OK" below I mean the obvious: I did a
>   "make bootstrap && make check" and that came through without errors.
>   This is for sure no proof of correctness, but hopefully at least an
>   indication that I'm not completely wrong.
>
> - All diffs in 1, ..., 6 are to be successively cumulated, while the
>   attached patch is again to be applied directly on commit
>   b819b8d6e90337b4cb36b35c2c6d0112c90a8e24.
>
> - When whatever changes have been decided on, I will be glad to
>   write unit tests for whatever makes sense to test.
>
> - It's a lengthy essay, sorry.  I hope it's worth reading, though.
>
>
> 1. Since Stefan has removed the advices on uniquify functions in
>    1a724cc2d2e7, that special handling of `rename-buffer' in
>    `advice--add-function' and `native-comp-never-optimize-functions'
>    seems to be obsolete.  Away with it:
>
> ------------------------- snip(1) -------------------------
> diff --git a/lisp/emacs-lisp/comp.el b/lisp/emacs-lisp/comp.el
> index 7fd9543d2ba..d94c19c20fa 100644
> --- a/lisp/emacs-lisp/comp.el
> +++ b/lisp/emacs-lisp/comp.el
> @@ -119,10 +119,9 @@ 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)
> +  '(;; The following is mandatory for Emacs to be working correctly
> +    ;; (see comment in `advice--add-function'). DO NOT REMOVE.
> +    macroexpand)
>    "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 ce5467f3c5c..946ca43f1cf 100644
> --- a/lisp/emacs-lisp/nadvice.el
> +++ b/lisp/emacs-lisp/nadvice.el
> @@ -397,14 +397,12 @@ advice--add-function
>               (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))
> +      ;; circular dependency in eager macro expansion.  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))
>          ;; Must require explicitly as during bootstrap we have no
>          ;; autoloads.
>          (require 'comp)
> ------------------------- snip(1) -------------------------
>
>    => bootstrap & check: OK

+1

>
> 2. The comment in `advice--add-function' says that `macroexpand' causes
>    a circular dependency, and there *are* still advices done on
>    `macroexpand' (at least in `cl-symbol-macrolet').  But probably these
>    are not executed during the bootstrap?  Let's try.
>
> ------------------------- snip(2) -------------------------
> diff --git a/lisp/emacs-lisp/comp.el b/lisp/emacs-lisp/comp.el
> index d94c19c20fa..5bbbabe548f 100644
> --- a/lisp/emacs-lisp/comp.el
> +++ b/lisp/emacs-lisp/comp.el
> @@ -119,9 +119,7 @@ native-comp-bootstrap-deny-list
>    :version "28.1")
>
>  (defcustom native-comp-never-optimize-functions
> -  '(;; The following is mandatory for Emacs to be working correctly
> -    ;; (see comment in `advice--add-function'). DO NOT REMOVE.
> -    macroexpand)
> +  '()
>    "Primitive functions to exclude from trampoline optimization.
>
>  Primitive functions included in this list will not be called

+1

> diff --git a/lisp/emacs-lisp/nadvice.el b/lisp/emacs-lisp/nadvice.el
> index 946ca43f1cf..e1945cc2b1b 100644
> --- a/lisp/emacs-lisp/nadvice.el
> +++ b/lisp/emacs-lisp/nadvice.el
> @@ -396,17 +396,7 @@ advice--add-function
>    (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.  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))
> -        ;; Must require explicitly as during bootstrap we have no
> -        ;; autoloads.
> -        (require 'comp)
> -        (comp-subr-trampoline-install subr-name))))
> +      (comp-subr-trampoline-install subr-name)))
>    (let* ((name (cdr (assq 'name props)))
>           (a (advice--member-p (or name function) (if name t) (gv-deref ref))))
>      (when a
> ------------------------- snip(2) -------------------------
>
>    => bootstrap & check: OK

+1

> 3. But then on the other hand, function Ffset also calls
>    `comp-subr-trampoline-install', and if we have condition
>
>      `(subr-primitive-p (gv-deref ref))'
>
>    fulfilled in `advice--add-function', then the following `(setf
>    (gv-deref ref) ...)' should ultimately call `fset'.  So probably we
>    can completely remove that `comp-subr-trampoline-install' call from
>    `advice--add-function', like this:
>
> ------------------------- snip(3) -------------------------
> diff --git a/lisp/emacs-lisp/nadvice.el b/lisp/emacs-lisp/nadvice.el
> index e1945cc2b1b..e7027bb36a7 100644
> --- a/lisp/emacs-lisp/nadvice.el
> +++ b/lisp/emacs-lisp/nadvice.el
> @@ -393,10 +393,6 @@ 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)))))
> -      (comp-subr-trampoline-install subr-name)))
>    (let* ((name (cdr (assq 'name props)))
>           (a (advice--member-p (or name function) (if name t) (gv-deref ref))))
>      (when a
> ------------------------- snip(3) -------------------------
>
>    => bootstrap & check: OK

Unfortanatelly I can't remember why we have this specific handling, I
guess there was a reason but anyway... If there is still a good reason
we'll discover it.

> 4. At this stage, what happens if we reactivate (a different) advice
> on `rename-buffer' in uniquify.el?
>
> ------------------------- snip(4) -------------------------
> diff --git a/lisp/uniquify.el b/lisp/uniquify.el
> index 7119ae7eac3..24d6ccaefad 100644
> --- a/lisp/uniquify.el
> +++ b/lisp/uniquify.el
> @@ -489,6 +489,14 @@ uniquify-kill-buffer-function
>  ;; rename-buffer and create-file-buffer.  (Setting find-file-hook isn't
>  ;; sufficient.)
>
> +(defconst uniquify--rename-buffer-history '())
> +
> +(advice-add 'rename-buffer :around #'uniquify--the-real-rename-buffer-advice)
> +(defun uniquify--the-real-rename-buffer-advice (origfun newname &optional unique)
> +  (setq uniquify--rename-buffer-history
> +        (cons newname uniquify--rename-buffer-history))
> +  (funcall origfun newname unique))
> +
>  ;; (advice-add 'rename-buffer :around #'uniquify--rename-buffer-advice)
>  (defun uniquify--rename-buffer-advice (newname &optional unique)
>    ;; BEWARE: This is called directly from `buffer.c'!
> ------------------------- snip(4) -------------------------
>
>   => bootstrap & check: OK
>
>   Plus the advice gets properly called, even from natively compiled
>   code!  Huh?
>
>   I think this works without problems since there is already a second
>   line of defense as follows:
>
>   a) loadup.el sets `native-comp-enable-subr-trampolines' to t only
>      after all files have been loaded, so Ffset, which specifically
>      tests for `native-comp-enable-subr-trampolines', never will call
>      `comp-subr-trampoline-install' during bootstrap.

Ok

>   b) As soon as `rename-buffer' got advised (which *is* soon in the
>      bootstrap), the test on `subrp' in function
>      `comp-call-optim-form-call' evals to nil, since the `subrp' only
>      "sees" the surrounding advice, and not the inner primitive.  Which
>      means that call optimizations won't take place for that.
>
>
> 5. And actually I think that b) above probably is a bug: It means that
>    for advised subrs, the optimzation to indirect calls in function
>    `comp-call-optim-form-call' never takes place.
>
>    So I tried to recognize subrs in function `comp-call-optim-form-call'
>    even when they are advised:
>
> ------------------------- snip(5) -------------------------
> diff --git a/lisp/emacs-lisp/comp.el b/lisp/emacs-lisp/comp.el
> index 5bbbabe548f..e60e5a0fa61 100644
> --- a/lisp/emacs-lisp/comp.el
> +++ b/lisp/emacs-lisp/comp.el
> @@ -3409,7 +3409,7 @@ 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)
> +                    (advice--cd*r (symbol-function callee))

Maybe we should make advice--cd*r public if we use it here?

>                    (cl-assert (byte-code-function-p callee))
>                    callee))
>               (subrp (subrp f))
> @@ -3419,9 +3419,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)
> ------------------------- snip(5) -------------------------
>
>    (The second diff in that function is a minor optimization - subrp
>    should be always t in that branch of the `cond'.)
>
>    => bootstrap & check: OK
>
>    But: Natively compiled code now does NOT execute the advice on
>    `rename-buffer' added in step 4, since its call gets optimized but no
>    trampoline has been created for it.

It's good we optimize the call but we can't apply this patch if the this
issue is not solved.

> 6. So there is the question whether we should actively disallow advices
>    during bootstrap, now that we are free of them.  Like this:

Others can have different opinions but if it's not a big limitation for
the future it sounds like a good idea to me.

Thanks

  Andrea