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" <bug-gnu-emacs@gnu.org>]

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