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

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?