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