From mboxrd@z Thu Jan  1 00:00:00 1970
Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail
From: Andrea Corallo <acorallo@gnu.org>
Newsgroups: gmane.emacs.bugs
Subject: bug#67005: 30.0.50; improve nadivce/comp/trampoline handling
Date: Mon, 13 Nov 2023 04:54:23 -0500
Message-ID: <yp1msvi6ks0.fsf@fencepost.gnu.org>
References: <874jhv9921.fsf@sappc2.fritz.box>
Mime-Version: 1.0
Content-Type: text/plain
Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214";
	logging-data="18486"; mail-complaints-to="usenet@ciao.gmane.io"
User-Agent: Gnus/5.13 (Gnus v5.13)
Cc: 67005@debbugs.gnu.org, stefan monnier <monnier@iro.umontreal.ca>
To: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Mon Nov 13 10:55:41 2023
Return-path: <bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org>
Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org
Original-Received: from lists.gnu.org ([209.51.188.17])
	by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256)
	(Exim 4.92)
	(envelope-from <bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org>)
	id 1r2TfQ-0004cn-Qu
	for geb-bug-gnu-emacs@m.gmane-mx.org; Mon, 13 Nov 2023 10:55:40 +0100
Original-Received: from localhost ([::1] helo=lists1p.gnu.org)
	by lists.gnu.org with esmtp (Exim 4.90_1)
	(envelope-from <bug-gnu-emacs-bounces@gnu.org>)
	id 1r2Tf9-0004pl-9X; Mon, 13 Nov 2023 04:55:23 -0500
Original-Received: from eggs.gnu.org ([2001:470:142:3::10])
 by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256)
 (Exim 4.90_1) (envelope-from <Debian-debbugs@debbugs.gnu.org>)
 id 1r2Tf7-0004pO-0y
 for bug-gnu-emacs@gnu.org; Mon, 13 Nov 2023 04:55:21 -0500
Original-Received: from debbugs.gnu.org ([2001:470:142:5::43])
 by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128)
 (Exim 4.90_1) (envelope-from <Debian-debbugs@debbugs.gnu.org>)
 id 1r2Tf6-0005LQ-PS
 for bug-gnu-emacs@gnu.org; Mon, 13 Nov 2023 04:55:20 -0500
Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2)
 (envelope-from <Debian-debbugs@debbugs.gnu.org>) id 1r2Tfl-0004q9-NE
 for bug-gnu-emacs@gnu.org; Mon, 13 Nov 2023 04:56:01 -0500
X-Loop: help-debbugs@gnu.org
Resent-From: Andrea Corallo <acorallo@gnu.org>
Original-Sender: "Debbugs-submit" <debbugs-submit-bounces@debbugs.gnu.org>
Resent-CC: bug-gnu-emacs@gnu.org
Resent-Date: Mon, 13 Nov 2023 09:56:01 +0000
Resent-Message-ID: <handler.67005.B67005.169986931718544@debbugs.gnu.org>
Resent-Sender: help-debbugs@gnu.org
X-GNU-PR-Message: followup 67005
X-GNU-PR-Package: emacs
Original-Received: via spool by 67005-submit@debbugs.gnu.org id=B67005.169986931718544
 (code B ref 67005); Mon, 13 Nov 2023 09:56:01 +0000
Original-Received: (at 67005) by debbugs.gnu.org; 13 Nov 2023 09:55:17 +0000
Original-Received: from localhost ([127.0.0.1]:57543 helo=debbugs.gnu.org)
 by debbugs.gnu.org with esmtp (Exim 4.84_2)
 (envelope-from <debbugs-submit-bounces@debbugs.gnu.org>)
 id 1r2Tf2-0004p1-RT
 for submit@debbugs.gnu.org; Mon, 13 Nov 2023 04:55:17 -0500
Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:35316)
 by debbugs.gnu.org with esmtp (Exim 4.84_2)
 (envelope-from <acorallo@gnu.org>) id 1r2Tex-0004ob-UT
 for 67005@debbugs.gnu.org; Mon, 13 Nov 2023 04:55:15 -0500
Original-Received: from fencepost.gnu.org ([2001:470:142:3::e])
 by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256)
 (Exim 4.90_1) (envelope-from <acorallo@gnu.org>)
 id 1r2TeB-00056Z-My; Mon, 13 Nov 2023 04:54:23 -0500
DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=gnu.org;
 s=fencepost-gnu-org; h=MIME-Version:Date:References:In-Reply-To:Subject:To:
 From; bh=6fbENi61zHxZVl8+UBgae8H8uoOUvfhhweW+3v+Z1P8=; b=rFZZnz4JA1Mv4osUyjX6
 wvQKgeDym26IEhHJOyRh2kTEWIzWC0wDi2mrtWIK06SMMPKeihUmw0I5UzoBgUDrefz7aWL6eh5ei
 QT1e7ALb2z8BWYp4SeH2psJN8kONPqI6KewtOZoFLWg+iEdWbuqaBz8QM4Um7tDBZW/ftPY1HcrtT
 ogTOVKIXlT/qvRfaPRj8iEdVA6bmW/JNwdASDg6ocnTEhZTYKK/JQ7KReRWTmgmGpxjCmIz1Z/pov
 V1G38wjIE6KEHhV4a0yYCrqb5q6vNrWAsnNR2Aw82wNC5a87cA1Yf4SeuEChj6OdiCvTtrz+oIFcT
 ZwySUY3D0IMptA==;
Original-Received: from acorallo by fencepost.gnu.org with local (Exim 4.90_1)
 (envelope-from <acorallo@gnu.org>)
 id 1r2TeB-0005Ww-CE; Mon, 13 Nov 2023 04:54:23 -0500
In-Reply-To: <874jhv9921.fsf@sappc2.fritz.box> (Jens Schmidt's message of
 "Wed, 08 Nov 2023 23:25:42 +0100")
X-BeenThere: debbugs-submit@debbugs.gnu.org
X-Mailman-Version: 2.1.18
Precedence: list
X-BeenThere: bug-gnu-emacs@gnu.org
List-Id: "Bug reports for GNU Emacs,
 the Swiss army knife of text editors" <bug-gnu-emacs.gnu.org>
List-Unsubscribe: <https://lists.gnu.org/mailman/options/bug-gnu-emacs>,
 <mailto:bug-gnu-emacs-request@gnu.org?subject=unsubscribe>
List-Archive: <https://lists.gnu.org/archive/html/bug-gnu-emacs>
List-Post: <mailto:bug-gnu-emacs@gnu.org>
List-Help: <mailto:bug-gnu-emacs-request@gnu.org?subject=help>
List-Subscribe: <https://lists.gnu.org/mailman/listinfo/bug-gnu-emacs>,
 <mailto:bug-gnu-emacs-request@gnu.org?subject=subscribe>
Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org
Original-Sender: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org
Xref: news.gmane.io gmane.emacs.bugs:274249
Archived-At: <http://permalink.gmane.org/gmane.emacs.bugs/274249>

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