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