bug#61880: Native compilation fails to generate trampolines on certain scenarios

bug#61880: Native compilation fails to generate trampolines on certain scenarios

[Sergio Durigan Junior]

[-- Attachment #1: Type: text/plain, Size: 4661 bytes --]

Hello,

While investigating a few bugs affecting Debian's and Ubuntu's Emacs
packages (for example,
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1028725), I stumbled
upon a problem that's affecting native compilation on Emacs 28.1+,
currently reproducible with git master as well.

I haven't been able to fully understand why the problem is happening,
but when there are two primitive functions (that would become
trampolines) being used sequentially, Emacs doesn't generate the
corresponding .eln file for the second function.

I spent some time investigating the problem and came up with a "minimal"
reproducer:

--8<---------------cut here---------------start------------->8---
(require 'cl-lib)

(defmacro foo--flet (funcs &rest body)
  "Like `cl-flet' but with dynamic function scope."
  (declare (indent 1))                                                                                                                                                                    
  (let* ((names (mapcar #'car funcs))
         (lambdas (mapcar #'cdr funcs))
         (gensyms (cl-loop for name in names
                           collect (make-symbol (symbol-name name)))))
    `(let ,(cl-loop for name in names
                    for gensym in gensyms
                    collect `(,gensym (symbol-function ',name)))
       (unwind-protect
           (progn
             ,@(cl-loop for name in names
                        for lambda in lambdas
                        for body = `(lambda ,@lambda)
                        collect `(setf (symbol-function ',name) ,body))
             ,@body)
         ,@(cl-loop for name in names
                    for gensym in gensyms
                    collect `(setf (symbol-function ',name) ,gensym))))))

(defun bar (file)
  (and (file-exists-p file) (file-readable-p file)))

(defun test ()
  (foo--flet ((file-exists-p (file) t)
              (file-readable-p (file) nil))
    (message "%s" (bar "/home/sergio/.lesshst"))))
--8<---------------cut here---------------end--------------->8---

When I run it using the following Emacs:

--8<---------------cut here---------------start------------->8---
GNU Emacs 30.0.50
Development version 68cc286c0495 on master branch; build date 2023-02-28.
--8<---------------cut here---------------end--------------->8---

here is the output I see:

--8<---------------cut here---------------start------------->8---
$ emacs -batch -Q -l t.el -f test -L .
Error: native-lisp-load-failed ("file does not exists" "/home/sergio/.emacs.d/eln-cache/30.0.50-23de7b18/subr--trampoline-66696c652d7265616461626c652d70_file_readable_p_0.eln")
  debug-early-backtrace()
  debug-early(error (native-lisp-load-failed "file does not exists" "/home/sergio/.emacs.d/eln-cache/30.0.50-23de7b18/subr--trampoline-66696c652d7265616461626c652d70_file_readable_p_0.eln"))
  native-elisp-load("/home/sergio/.emacs.d/eln-cache/30.0.50-23de7b18/subr--trampoline-66696c652d7265616461626c652d70_file_readable_p_0.eln")
  comp-trampoline-search(file-readable-p)
  comp-subr-trampoline-install(file-readable-p)
  fset(file-readable-p (lambda (file) nil))
  (progn (fset 'file-exists-p #'(lambda (file) t)) (fset 'file-readable-p #'(lambda (file) nil)) (message "%s" (bar "/home/sergio/.lesshst")))
  (unwind-protect (progn (fset 'file-exists-p #'(lambda (file) t)) (fset 'file-readable-p #'(lambda (file) nil)) (message "%s" (bar "/home/sergio/.lesshst"))) (fset 'file-exists-p file-exist
s-p) (fset 'file-readable-p file-readable-p))
  (let ((file-exists-p (symbol-function 'file-exists-p)) (file-readable-p (symbol-function 'file-readable-p))) (unwind-protect (progn (fset 'file-exists-p #'(lambda (file) t)) (fset 'file-re
adable-p #'(lambda (file) nil)) (message "%s" (bar "/home/sergio/.lesshst"))) (fset 'file-exists-p file-exists-p) (fset 'file-readable-p file-readable-p)))
  test()
  command-line-1(("-l" "t.el" "-f" "test" "-L" "."))
  command-line()
  normal-top-level()
Native elisp load failed: "file does not exists", "/home/sergio/.emacs.d/eln-cache/30.0.50-23de7b18/subr--trampoline-66696c652d7265616461626c652d70_file_readable_p_0.eln"
--8<---------------cut here---------------end--------------->8---

Do note that this is already affecting a few packages, like buttercup
(see https://github.com/jorgenschaefer/emacs-buttercup/issues/230) and
emacs-web-server, for example.

Please let me know if you need more information regarding the problem.

Thank you,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
https://sergiodj.net/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

bug#61880: Native compilation fails to generate trampolines on certain scenarios

[Eli Zaretskii]

> From: Sergio Durigan Junior <sergiodj@sergiodj.net>
> Date: Tue, 28 Feb 2023 19:13:58 -0500
> 
> While investigating a few bugs affecting Debian's and Ubuntu's Emacs
> packages (for example,
> https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1028725), I stumbled
> upon a problem that's affecting native compilation on Emacs 28.1+,
> currently reproducible with git master as well.
> 
> I haven't been able to fully understand why the problem is happening,
> but when there are two primitive functions (that would become
> trampolines) being used sequentially, Emacs doesn't generate the
> corresponding .eln file for the second function.
> 
> I spent some time investigating the problem and came up with a "minimal"
> reproducer:
> 
> --8<---------------cut here---------------start------------->8---
> (require 'cl-lib)
> 
> (defmacro foo--flet (funcs &rest body)
>   "Like `cl-flet' but with dynamic function scope."
>   (declare (indent 1))                                                                                                                                                                    
>   (let* ((names (mapcar #'car funcs))
>          (lambdas (mapcar #'cdr funcs))
>          (gensyms (cl-loop for name in names
>                            collect (make-symbol (symbol-name name)))))
>     `(let ,(cl-loop for name in names
>                     for gensym in gensyms
>                     collect `(,gensym (symbol-function ',name)))
>        (unwind-protect
>            (progn
>              ,@(cl-loop for name in names
>                         for lambda in lambdas
>                         for body = `(lambda ,@lambda)
>                         collect `(setf (symbol-function ',name) ,body))
>              ,@body)
>          ,@(cl-loop for name in names
>                     for gensym in gensyms
>                     collect `(setf (symbol-function ',name) ,gensym))))))
> 
> (defun bar (file)
>   (and (file-exists-p file) (file-readable-p file)))
> 
> (defun test ()
>   (foo--flet ((file-exists-p (file) t)
>               (file-readable-p (file) nil))
>     (message "%s" (bar "/home/sergio/.lesshst"))))
> --8<---------------cut here---------------end--------------->8---
> 
> When I run it using the following Emacs:
> 
> --8<---------------cut here---------------start------------->8---
> GNU Emacs 30.0.50
> Development version 68cc286c0495 on master branch; build date 2023-02-28.
> --8<---------------cut here---------------end--------------->8---
> 
> here is the output I see:
> 
> --8<---------------cut here---------------start------------->8---
> $ emacs -batch -Q -l t.el -f test -L .
> Error: native-lisp-load-failed ("file does not exists" "/home/sergio/.emacs.d/eln-cache/30.0.50-23de7b18/subr--trampoline-66696c652d7265616461626c652d70_file_readable_p_0.eln")
>   debug-early-backtrace()
>   debug-early(error (native-lisp-load-failed "file does not exists" "/home/sergio/.emacs.d/eln-cache/30.0.50-23de7b18/subr--trampoline-66696c652d7265616461626c652d70_file_readable_p_0.eln"))
>   native-elisp-load("/home/sergio/.emacs.d/eln-cache/30.0.50-23de7b18/subr--trampoline-66696c652d7265616461626c652d70_file_readable_p_0.eln")
>   comp-trampoline-search(file-readable-p)
>   comp-subr-trampoline-install(file-readable-p)
>   fset(file-readable-p (lambda (file) nil))
>   (progn (fset 'file-exists-p #'(lambda (file) t)) (fset 'file-readable-p #'(lambda (file) nil)) (message "%s" (bar "/home/sergio/.lesshst")))
>   (unwind-protect (progn (fset 'file-exists-p #'(lambda (file) t)) (fset 'file-readable-p #'(lambda (file) nil)) (message "%s" (bar "/home/sergio/.lesshst"))) (fset 'file-exists-p file-exist
> s-p) (fset 'file-readable-p file-readable-p))
>   (let ((file-exists-p (symbol-function 'file-exists-p)) (file-readable-p (symbol-function 'file-readable-p))) (unwind-protect (progn (fset 'file-exists-p #'(lambda (file) t)) (fset 'file-re
> adable-p #'(lambda (file) nil)) (message "%s" (bar "/home/sergio/.lesshst"))) (fset 'file-exists-p file-exists-p) (fset 'file-readable-p file-readable-p)))
>   test()
>   command-line-1(("-l" "t.el" "-f" "test" "-L" "."))
>   command-line()
>   normal-top-level()
> Native elisp load failed: "file does not exists", "/home/sergio/.emacs.d/eln-cache/30.0.50-23de7b18/subr--trampoline-66696c652d7265616461626c652d70_file_readable_p_0.eln"
> --8<---------------cut here---------------end--------------->8---
> 
> Do note that this is already affecting a few packages, like buttercup
> (see https://github.com/jorgenschaefer/emacs-buttercup/issues/230) and
> emacs-web-server, for example.
> 
> Please let me know if you need more information regarding the problem.

Thanks.

Andrea, can you please look into this?  I guess if this happens in
Emacs 28 and on master, it also affects Emacs 29, so I hope this can
be fixed quickly and safely.  TIA.

bug#61880: Native compilation fails to generate trampolines on certain scenarios

[Sergio Durigan Junior]

On Wednesday, March 01 2023, Andrea Corallo wrote:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> From: Sergio Durigan Junior <sergiodj@sergiodj.net>
>>> Date: Tue, 28 Feb 2023 19:13:58 -0500
>>> 
>>> While investigating a few bugs affecting Debian's and Ubuntu's Emacs
>>> packages (for example,
>>> https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1028725), I stumbled
>>> upon a problem that's affecting native compilation on Emacs 28.1+,
>>> currently reproducible with git master as well.
>>> 
>>> I haven't been able to fully understand why the problem is happening,
>>> but when there are two primitive functions (that would become
>>> trampolines) being used sequentially, Emacs doesn't generate the
>>> corresponding .eln file for the second function.
>>> 
>>> I spent some time investigating the problem and came up with a "minimal"
>>> reproducer:
>>> 
>>> --8<---------------cut here---------------start------------->8---
>>> (require 'cl-lib)
>>> 
>>> (defmacro foo--flet (funcs &rest body)
>>>   "Like `cl-flet' but with dynamic function scope."
>>>   (declare (indent 1))                                                                                                                                                                    
>>>   (let* ((names (mapcar #'car funcs))
>>>          (lambdas (mapcar #'cdr funcs))
>>>          (gensyms (cl-loop for name in names
>>>                            collect (make-symbol (symbol-name name)))))
>>>     `(let ,(cl-loop for name in names
>>>                     for gensym in gensyms
>>>                     collect `(,gensym (symbol-function ',name)))
>>>        (unwind-protect
>>>            (progn
>>>              ,@(cl-loop for name in names
>>>                         for lambda in lambdas
>>>                         for body = `(lambda ,@lambda)
>>>                         collect `(setf (symbol-function ',name) ,body))
>>>              ,@body)
>>>          ,@(cl-loop for name in names
>>>                     for gensym in gensyms
>>>                     collect `(setf (symbol-function ',name) ,gensym))))))
>>> 
>>> (defun bar (file)
>>>   (and (file-exists-p file) (file-readable-p file)))
>>> 
>>> (defun test ()
>>>   (foo--flet ((file-exists-p (file) t)
>>>               (file-readable-p (file) nil))
>>>     (message "%s" (bar "/home/sergio/.lesshst"))))
>>> --8<---------------cut here---------------end--------------->8---
>>> 
>>> When I run it using the following Emacs:
>>> 
>>> --8<---------------cut here---------------start------------->8---
>>> GNU Emacs 30.0.50
>>> Development version 68cc286c0495 on master branch; build date 2023-02-28.
>>> --8<---------------cut here---------------end--------------->8---
>>> 
>>> here is the output I see:
>>> 
>>> --8<---------------cut here---------------start------------->8---
>>> $ emacs -batch -Q -l t.el -f test -L .
>>> Error: native-lisp-load-failed ("file does not exists" "/home/sergio/.emacs.d/eln-cache/30.0.50-23de7b18/subr--trampoline-66696c652d7265616461626c652d70_file_readable_p_0.eln")
>>>   debug-early-backtrace()
>>>   debug-early(error (native-lisp-load-failed "file does not exists" "/home/sergio/.emacs.d/eln-cache/30.0.50-23de7b18/subr--trampoline-66696c652d7265616461626c652d70_file_readable_p_0.eln"))
>>>   native-elisp-load("/home/sergio/.emacs.d/eln-cache/30.0.50-23de7b18/subr--trampoline-66696c652d7265616461626c652d70_file_readable_p_0.eln")
>>>   comp-trampoline-search(file-readable-p)
>>>   comp-subr-trampoline-install(file-readable-p)
>>>   fset(file-readable-p (lambda (file) nil))
>>>   (progn (fset 'file-exists-p #'(lambda (file) t)) (fset 'file-readable-p #'(lambda (file) nil)) (message "%s" (bar "/home/sergio/.lesshst")))
>>>   (unwind-protect (progn (fset 'file-exists-p #'(lambda (file) t)) (fset 'file-readable-p #'(lambda (file) nil)) (message "%s" (bar "/home/sergio/.lesshst"))) (fset 'file-exists-p file-exist
>>> s-p) (fset 'file-readable-p file-readable-p))
>>>   (let ((file-exists-p (symbol-function 'file-exists-p)) (file-readable-p (symbol-function 'file-readable-p))) (unwind-protect (progn (fset 'file-exists-p #'(lambda (file) t)) (fset 'file-re
>>> adable-p #'(lambda (file) nil)) (message "%s" (bar "/home/sergio/.lesshst"))) (fset 'file-exists-p file-exists-p) (fset 'file-readable-p file-readable-p)))
>>>   test()
>>>   command-line-1(("-l" "t.el" "-f" "test" "-L" "."))
>>>   command-line()
>>>   normal-top-level()
>>> Native elisp load failed: "file does not exists", "/home/sergio/.emacs.d/eln-cache/30.0.50-23de7b18/subr--trampoline-66696c652d7265616461626c652d70_file_readable_p_0.eln"
>>> --8<---------------cut here---------------end--------------->8---
>>> 
>>> Do note that this is already affecting a few packages, like buttercup
>>> (see https://github.com/jorgenschaefer/emacs-buttercup/issues/230) and
>>> emacs-web-server, for example.
>>> 
>>> Please let me know if you need more information regarding the problem.
>>
>> Thanks.
>>
>> Andrea, can you please look into this?  I guess if this happens in
>> Emacs 28 and on master, it also affects Emacs 29, so I hope this can
>> be fixed quickly and safely.  TIA.
>>
>
> Yep, sorry the IP of my mail provider is (temporary?) banned and I don't
> receive nor emacs-bugs nor emacs-devel since a couple of days :( thanks
> for Ccing me.
>
> So what should be going on here is that `file-exists-p' gets redefined
> with a non working mock function while we are trying to compile a
> trampoline for `file-readable-p' (it's redefined just afterward).

Thank you for taking the time to reply and investigate this problem.

> I don't think there is a trivial fix for this, we should rewrite
> `comp-trampoline-search' in C in order to have it not sensitive to the
> redefinition of `file-exists-p', or define another primitive like
> `comp-file-exists-p' (that calls directly Ffile_exists_p from C) and use
> that in `comp-trampoline-search'.  This would cover this specific case
> but potentially not others.

Forgive my ignorance, but wouldn't we need to do that for every
primitive that can be compiled into a trampoline?  I say that because
the error I've encountered above refers to 'file-readable-p', which
doesn't seem to call 'file-exists-p'.  FWIW, I did encounter the same
problem with 'file-exists-p' and 'buffer-file-name' as well, which is
the reason why I think solely having a 'comp-file-exists-p' wouldn't be
enough.

> Fact is, Emacs can't be robust against the redefinition of all
> primitives (actually never was), the programmer that redefines
> primitives should be ready to understand the underlying Emacs machinery,
> and with native compilation this machinery changed a bit.

I understand where you're coming from, but it's also important to note
that this behaviour was accepted without problems until the native
compilation feature came about, so it is understandable that we are
getting a lot of confusing people wondering why their tests started
failing now.  I believe there should be more emphasis in the
documentation that this problem can creep in, especially for those who
are relying on redefinitions for testing purposes.

> So two options:
>
> * The redefinition of `file-exists-p' is tipically done for test
>   purposes only, we accept that and for this case we suggest to run
>   these specific tests setting `native-comp-enable-subr-trampolines' to
>   nil

This is what I'm currently doing in Debian/Ubuntu, and will start
suggesting upstream maintainers to do the same.

> * We work around this specific issue with the `comp-file-exists-p' trick
>   (or another one).

As said above, I don't believe this will be enough for this case, but I
may be completely wrong here.

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
https://sergiodj.net/

bug#61880: Native compilation fails to generate trampolines on certain scenarios

[Eli Zaretskii]

> From: Sergio Durigan Junior <sergiodj@sergiodj.net>
> Cc: Eli Zaretskii <eliz@gnu.org>,  61880@debbugs.gnu.org
> Date: Wed, 01 Mar 2023 18:14:01 -0500
> 
> > I don't think there is a trivial fix for this, we should rewrite
> > `comp-trampoline-search' in C in order to have it not sensitive to the
> > redefinition of `file-exists-p', or define another primitive like
> > `comp-file-exists-p' (that calls directly Ffile_exists_p from C) and use
> > that in `comp-trampoline-search'.  This would cover this specific case
> > but potentially not others.
> 
> Forgive my ignorance, but wouldn't we need to do that for every
> primitive that can be compiled into a trampoline?

That's basically what Andrea was saying by the "potentially not
others" part.  So you are in agreement here.

> > Fact is, Emacs can't be robust against the redefinition of all
> > primitives (actually never was), the programmer that redefines
> > primitives should be ready to understand the underlying Emacs machinery,
> > and with native compilation this machinery changed a bit.
> 
> I understand where you're coming from, but it's also important to note
> that this behaviour was accepted without problems until the native
> compilation feature came about, so it is understandable that we are
> getting a lot of confusing people wondering why their tests started
> failing now.  I believe there should be more emphasis in the
> documentation that this problem can creep in, especially for those who
> are relying on redefinitions for testing purposes.
> 
> > So two options:
> >
> > * The redefinition of `file-exists-p' is tipically done for test
> >   purposes only, we accept that and for this case we suggest to run
> >   these specific tests setting `native-comp-enable-subr-trampolines' to
> >   nil
> 
> This is what I'm currently doing in Debian/Ubuntu, and will start
> suggesting upstream maintainers to do the same.

I can come up with documentation of this subtlety, including a list of
primitives whose redefinition could trigger these issues, if this is
an acceptable solution.

> > * We work around this specific issue with the `comp-file-exists-p' trick
> >   (or another one).
> 
> As said above, I don't believe this will be enough for this case, but I
> may be completely wrong here.

You are not wrong.  I don't think the 2nd alternative is what we
should do.

bug#61880: Native compilation fails to generate trampolines on certain scenarios

[Sergio Durigan Junior]

On Thursday, March 02 2023, Eli Zaretskii wrote:

>> From: Sergio Durigan Junior <sergiodj@sergiodj.net>
>> Cc: Eli Zaretskii <eliz@gnu.org>,  61880@debbugs.gnu.org
>> Date: Wed, 01 Mar 2023 18:14:01 -0500
>> 
>> > Fact is, Emacs can't be robust against the redefinition of all
>> > primitives (actually never was), the programmer that redefines
>> > primitives should be ready to understand the underlying Emacs machinery,
>> > and with native compilation this machinery changed a bit.
>> 
>> I understand where you're coming from, but it's also important to note
>> that this behaviour was accepted without problems until the native
>> compilation feature came about, so it is understandable that we are
>> getting a lot of confusing people wondering why their tests started
>> failing now.  I believe there should be more emphasis in the
>> documentation that this problem can creep in, especially for those who
>> are relying on redefinitions for testing purposes.
>> 
>> > So two options:
>> >
>> > * The redefinition of `file-exists-p' is tipically done for test
>> >   purposes only, we accept that and for this case we suggest to run
>> >   these specific tests setting `native-comp-enable-subr-trampolines' to
>> >   nil
>> 
>> This is what I'm currently doing in Debian/Ubuntu, and will start
>> suggesting upstream maintainers to do the same.
>
> I can come up with documentation of this subtlety, including a list of
> primitives whose redefinition could trigger these issues, if this is
> an acceptable solution.

Yes, this would be a great first step.  I wonder if there's some warning
Emacs can print when it detects that a primitive is being redefined and
native compilation is enabled.  On the one hand, Emacs would be a bit
more verbose than perhaps desirable; on the other, I think this scenario
is particular enough that having a warning is OK-ish.

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
https://sergiodj.net/

bug#61880: Native compilation fails to generate trampolines on certain scenarios

[Eli Zaretskii]

> From: Sergio Durigan Junior <sergiodj@sergiodj.net>
> Cc: akrl@sdf.org,  61880@debbugs.gnu.org
> Date: Thu, 02 Mar 2023 18:54:33 -0500
> 
> On Thursday, March 02 2023, Eli Zaretskii wrote:
> 
> > I can come up with documentation of this subtlety, including a list of
> > primitives whose redefinition could trigger these issues, if this is
> > an acceptable solution.
> 
> Yes, this would be a great first step.  I wonder if there's some warning
> Emacs can print when it detects that a primitive is being redefined and
> native compilation is enabled.  On the one hand, Emacs would be a bit
> more verbose than perhaps desirable; on the other, I think this scenario
> is particular enough that having a warning is OK-ish.

Emitting such a warning for every primitive that is redefined or
advised could be annoying indeed, but maybe we should warn only about
the few primitives that might disrupt compilation of trampolines, and
only when a trampoline is compiled?  Andrea, can we do something like
that?

bug#61880: Native compilation fails to generate trampolines on certain scenarios

[Eli Zaretskii]

> From: Andrea Corallo <akrl@sdf.org>
> Cc: Sergio Durigan Junior <sergiodj@sergiodj.net>,  61880@debbugs.gnu.org
> Date: Fri, 03 Mar 2023 10:05:21 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Emitting such a warning for every primitive that is redefined or
> > advised could be annoying indeed, but maybe we should warn only about
> > the few primitives that might disrupt compilation of trampolines, and
> > only when a trampoline is compiled?  Andrea, can we do something like
> > that?
> >
> 
> I think technically should be easy to emit the warning, again the non
> trivial part is to form the list of primitives to warn at redefinition
> (and to keep this list updated over time!).

Well, currently we don't warn at all, so even warning about some of
the primitives would be an improvement, I think.

> To a quick look into the trampoline machinery in comp.el I see we rely
> on:
> 
> null, memq, gethash, and, subrp, not, subr-native-elisp-p,
> comp--install-trampoline, concat, if, symbolp, symbol-name, make-string,
> length, aset, aref, length>, mapcar, expand-file-name,
> file-name-as-directory, file-exists-p, native-elisp-load.
> 
> Note: I haven't followed all the possible execution paths outside
> comp.el.
> 
> Should we start with these?

Yes, I think we should start with those, and add more as we discover
them.

> PS I'll never understand why people think redefining something like `if'
> would indeed break everything but they expect `file-exists-p' to be
> redefinable at any point

I agree.  But since we are going to have a list of primitives to warn
about anyway, I see no reason not to have those basic ones there, just
in case someone tries to do the unimaginable, for whatever reasons.

Thanks.

bug#61880: Native compilation fails to generate trampolines on certain scenarios

[Andrea Corallo]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Andrea Corallo <akrl@sdf.org>
>> Cc: Sergio Durigan Junior <sergiodj@sergiodj.net>,  61880@debbugs.gnu.org
>> Date: Fri, 03 Mar 2023 10:05:21 +0000
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > Emitting such a warning for every primitive that is redefined or
>> > advised could be annoying indeed, but maybe we should warn only about
>> > the few primitives that might disrupt compilation of trampolines, and
>> > only when a trampoline is compiled?  Andrea, can we do something like
>> > that?
>> >
>> 
>> I think technically should be easy to emit the warning, again the non
>> trivial part is to form the list of primitives to warn at redefinition
>> (and to keep this list updated over time!).
>
> Well, currently we don't warn at all, so even warning about some of
> the primitives would be an improvement, I think.
>
>> To a quick look into the trampoline machinery in comp.el I see we rely
>> on:
>> 
>> null, memq, gethash, and, subrp, not, subr-native-elisp-p,
>> comp--install-trampoline, concat, if, symbolp, symbol-name, make-string,
>> length, aset, aref, length>, mapcar, expand-file-name,
>> file-name-as-directory, file-exists-p, native-elisp-load.
>> 
>> Note: I haven't followed all the possible execution paths outside
>> comp.el.
>> 
>> Should we start with these?
>
> Yes, I think we should start with those, and add more as we discover
> them.

BTW would you like to suggest a warning message?

Should we say that redefining this primitive breaks Emacs in general or
be more specific on the trampoline mechanism?

I ask as I'm a little puzzled on what to say as there's certanly a ton
of other primitives that when redefined breaks Emacs somewere else than
the trampoline machinery, so maybe we should be not too generic if we
want to have this warning also for nativecomp.  At the same time I feel
beeing too specific in the message would be not ideal.

Best Regards

  Andrea

bug#61880: Native compilation fails to generate trampolines on certain scenarios

[Eli Zaretskii]

> From: Andrea Corallo <akrl@sdf.org>
> Cc: sergiodj@sergiodj.net,  61880@debbugs.gnu.org
> Date: Sat, 04 Mar 2023 00:20:41 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> Should we start with these?
> >
> > Yes, I think we should start with those, and add more as we discover
> > them.
> 
> BTW would you like to suggest a warning message?

Something like

  Redefining `%s' while compiling trampolines might fail compilation.

where %s is the primitive name.

> Should we say that redefining this primitive breaks Emacs in general or
> be more specific on the trampoline mechanism?

The latter, I think.

> I ask as I'm a little puzzled on what to say as there's certanly a ton
> of other primitives that when redefined breaks Emacs somewere else than
> the trampoline machinery, so maybe we should be not too generic if we
> want to have this warning also for nativecomp.  At the same time I feel
> beeing too specific in the message would be not ideal.

It's true that redefining arbitrary primitive is inherently dangerous,
but as long as that danger just causes the programmer shoot themselves
in the foot, that is their problem.  Here we are talking about a
mechanism -- native compilation of primitives -- that gets activated
implicitly, not by any request of the program that runs, so it's a bit
different.

Btw, an alternative is to automatically disable trampoline compilation
if we detect one of the critical primitives redefined.  Then we could
say in the warning

  Native compilation of trampolines disabled because `%s' is redefined.

WDYT about this possibility?

bug#61880: Native compilation fails to generate trampolines on certain scenarios

[Eli Zaretskii]

> From: Andrea Corallo <akrl@sdf.org>
> Cc: sergiodj@sergiodj.net,  61880@debbugs.gnu.org
> Date: Sun, 05 Mar 2023 10:08:12 +0000
> 
> Okay I pushed 9c18af0cfaf with a slightly reworded warning to address
> this, feel free to improve it if you fee.

Thanks.  I've changed the wording slightly.

bug#61880: Native compilation fails to generate trampolines on certain scenarios

[Eli Zaretskii]

> Cc: sergiodj@sergiodj.net, 61880@debbugs.gnu.org
> Date: Sun, 05 Mar 2023 12:44:29 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> 
> > From: Andrea Corallo <akrl@sdf.org>
> > Cc: sergiodj@sergiodj.net,  61880@debbugs.gnu.org
> > Date: Sun, 05 Mar 2023 10:08:12 +0000
> > 
> > Okay I pushed 9c18af0cfaf with a slightly reworded warning to address
> > this, feel free to improve it if you fee.
> 
> Thanks.  I've changed the wording slightly.

Is there anything else to do for this issue, or should we now close
it?

bug#61880: Native compilation fails to generate trampolines on certain scenarios

[Andrea Corallo]

Eli Zaretskii <eliz@gnu.org> writes:

>> Cc: sergiodj@sergiodj.net, 61880@debbugs.gnu.org
>> Date: Sun, 05 Mar 2023 12:44:29 +0200
>> From: Eli Zaretskii <eliz@gnu.org>
>> 
>> > From: Andrea Corallo <akrl@sdf.org>
>> > Cc: sergiodj@sergiodj.net,  61880@debbugs.gnu.org
>> > Date: Sun, 05 Mar 2023 10:08:12 +0000
>> > 
>> > Okay I pushed 9c18af0cfaf with a slightly reworded warning to address
>> > this, feel free to improve it if you fee.
>> 
>> Thanks.  I've changed the wording slightly.
>
> Is there anything else to do for this issue, or should we now close
> it?

Not that I'm aware.

I'm closing it now, happy to reopen if something more pops-up.

Best Regards

  Andrea

bug#61880: Native compilation fails to generate trampolines on certain scenarios

[Sergio Durigan Junior]

On Thursday, March 09 2023, Andrea Corallo wrote:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> Cc: sergiodj@sergiodj.net, 61880@debbugs.gnu.org
>>> Date: Sun, 05 Mar 2023 12:44:29 +0200
>>> From: Eli Zaretskii <eliz@gnu.org>
>>> 
>>> > From: Andrea Corallo <akrl@sdf.org>
>>> > Cc: sergiodj@sergiodj.net,  61880@debbugs.gnu.org
>>> > Date: Sun, 05 Mar 2023 10:08:12 +0000
>>> > 
>>> > Okay I pushed 9c18af0cfaf with a slightly reworded warning to address
>>> > this, feel free to improve it if you fee.
>>> 
>>> Thanks.  I've changed the wording slightly.
>>
>> Is there anything else to do for this issue, or should we now close
>> it?
>
> Not that I'm aware.
>
> I'm closing it now, happy to reopen if something more pops-up.

Thank you both for working on a proper fix for this problem.

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
https://sergiodj.net/

bug#61880: Native compilation fails to generate trampolines on certain scenarios

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > > To a quick look into the trampoline machinery in comp.el I see we rely
  > > on:
  > > 
  > > null, memq, gethash, and, subrp, not, subr-native-elisp-p,
  > > comp--install-trampoline, concat, if, symbolp, symbol-name, make-string,
  > > length, aset, aref, length>, mapcar, expand-file-name,
  > > file-name-as-directory, file-exists-p, native-elisp-load.

There is nothing wrong with ignoring the return value of `aset'.  That
is normal.  We use it like `setq'.  Do not make warnings for that.

It is normal to ignore return values from `if' and `and'.  They are
control constructs.  Do not make warnings for that.

The others are really functions, so ignoring their return values is
slightly anomalous.  It is ok to implement warnings when their values
are ignored, but please do not enable these warnings by default.
Give us a chance to test the warnings and see what fraction of them
indicate a real reason to change the code.  If we are all happy with
the issuance of those warnings, thenwe can enable the warnings by
default.


-- 
Dr Richard Stallman (https://stallman.org)
Chief GNUisance of the GNU Project (https://gnu.org)
Founder, Free Software Foundation (https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

bug#61880: Native compilation fails to generate trampolines on certain scenarios

[Eli Zaretskii]

> From: Richard Stallman <rms@gnu.org>
> Cc: akrl@sdf.org, sergiodj@sergiodj.net, 61880@debbugs.gnu.org
> Date: Sat, 04 Mar 2023 23:03:57 -0500
> 
>   > > To a quick look into the trampoline machinery in comp.el I see we rely
>   > > on:
>   > > 
>   > > null, memq, gethash, and, subrp, not, subr-native-elisp-p,
>   > > comp--install-trampoline, concat, if, symbolp, symbol-name, make-string,
>   > > length, aset, aref, length>, mapcar, expand-file-name,
>   > > file-name-as-directory, file-exists-p, native-elisp-load.
> 
> There is nothing wrong with ignoring the return value of `aset'.  That
> is normal.  We use it like `setq'.  Do not make warnings for that.

This is a misunderstanding, I think: this particular discussion is not
about ignoring the return values, it is about redefining primitives
while natively-compiling trampolines.

I think you mixed this up with another discussion.

bug#61880: Native compilation fails to generate trampolines on certain scenarios

[Eli Zaretskii]

> From: Andrea Corallo <akrl@sdf.org>
> Cc: Eli Zaretskii <eliz@gnu.org>,  61880@debbugs.gnu.org
> Date: Thu, 02 Mar 2023 11:47:43 +0000
> 
> >> * The redefinition of `file-exists-p' is tipically done for test
> >>   purposes only, we accept that and for this case we suggest to run
> >>   these specific tests setting `native-comp-enable-subr-trampolines' to
> >>   nil
> >
> > This is what I'm currently doing in Debian/Ubuntu, and will start
> > suggesting upstream maintainers to do the same.
> 
> Note, I think this should be suggested only for tests redefining
> `file-exists-p'.

Are you saying that file-exists-p is the only primitive whose
redefinition could screw generation of trampolines that follows?  I
though redefinition of additional primitives could potentially cause
similar problems.  Basically, any primitives that are called by the
code which is involved in producing a trampoline.  Was I mistaken?

bug#61880: Native compilation fails to generate trampolines on certain scenarios

[Sergio Durigan Junior]

On Thursday, March 02 2023, Andrea Corallo wrote:

> Sergio Durigan Junior <sergiodj@sergiodj.net> writes:
>
>> On Wednesday, March 01 2023, Andrea Corallo wrote:
>>
>>> Eli Zaretskii <eliz@gnu.org> writes:
>>>
>>>>> From: Sergio Durigan Junior <sergiodj@sergiodj.net>
>>>>> Date: Tue, 28 Feb 2023 19:13:58 -0500
>>>>> 
>>>>> While investigating a few bugs affecting Debian's and Ubuntu's Emacs
>>>>> packages (for example,
>>>>> https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1028725), I stumbled
>>>>> upon a problem that's affecting native compilation on Emacs 28.1+,
>>>>> currently reproducible with git master as well.
>>>>> 
>>>>> I haven't been able to fully understand why the problem is happening,
>>>>> but when there are two primitive functions (that would become
>>>>> trampolines) being used sequentially, Emacs doesn't generate the
>>>>> corresponding .eln file for the second function.
>>>>> 
>>>>> I spent some time investigating the problem and came up with a "minimal"
>>>>> reproducer:
>>>>> 
>>>>> --8<---------------cut here---------------start------------->8---
>>>>> (require 'cl-lib)
>>>>> 
>>>>> (defmacro foo--flet (funcs &rest body)
>>>>>   "Like `cl-flet' but with dynamic function scope."
>>>>>   (declare (indent 1))                                                                                                                                                                    
>>>>>   (let* ((names (mapcar #'car funcs))
>>>>>          (lambdas (mapcar #'cdr funcs))
>>>>>          (gensyms (cl-loop for name in names
>>>>>                            collect (make-symbol (symbol-name name)))))
>>>>>     `(let ,(cl-loop for name in names
>>>>>                     for gensym in gensyms
>>>>>                     collect `(,gensym (symbol-function ',name)))
>>>>>        (unwind-protect
>>>>>            (progn
>>>>>              ,@(cl-loop for name in names
>>>>>                         for lambda in lambdas
>>>>>                         for body = `(lambda ,@lambda)
>>>>>                         collect `(setf (symbol-function ',name) ,body))
>>>>>              ,@body)
>>>>>          ,@(cl-loop for name in names
>>>>>                     for gensym in gensyms
>>>>>                     collect `(setf (symbol-function ',name) ,gensym))))))
>>>>> 
>>>>> (defun bar (file)
>>>>>   (and (file-exists-p file) (file-readable-p file)))
>>>>> 
>>>>> (defun test ()
>>>>>   (foo--flet ((file-exists-p (file) t)
>>>>>               (file-readable-p (file) nil))
>>>>>     (message "%s" (bar "/home/sergio/.lesshst"))))
>>>>> --8<---------------cut here---------------end--------------->8---
>>>>> 
>>>>> When I run it using the following Emacs:
>>>>> 
>>>>> --8<---------------cut here---------------start------------->8---
>>>>> GNU Emacs 30.0.50
>>>>> Development version 68cc286c0495 on master branch; build date 2023-02-28.
>>>>> --8<---------------cut here---------------end--------------->8---
>>>>> 
>>>>> here is the output I see:
>>>>> 
>>>>> --8<---------------cut here---------------start------------->8---
>>>>> $ emacs -batch -Q -l t.el -f test -L .
>>>>> Error: native-lisp-load-failed ("file does not exists" "/home/sergio/.emacs.d/eln-cache/30.0.50-23de7b18/subr--trampoline-66696c652d7265616461626c652d70_file_readable_p_0.eln")
>>>>>   debug-early-backtrace()
>>>>>   debug-early(error (native-lisp-load-failed "file does not exists" "/home/sergio/.emacs.d/eln-cache/30.0.50-23de7b18/subr--trampoline-66696c652d7265616461626c652d70_file_readable_p_0.eln"))
>>>>>   native-elisp-load("/home/sergio/.emacs.d/eln-cache/30.0.50-23de7b18/subr--trampoline-66696c652d7265616461626c652d70_file_readable_p_0.eln")
>>>>>   comp-trampoline-search(file-readable-p)
>>>>>   comp-subr-trampoline-install(file-readable-p)
>>>>>   fset(file-readable-p (lambda (file) nil))
>>>>>   (progn (fset 'file-exists-p #'(lambda (file) t)) (fset 'file-readable-p #'(lambda (file) nil)) (message "%s" (bar "/home/sergio/.lesshst")))
>>>>>   (unwind-protect (progn (fset 'file-exists-p #'(lambda (file) t)) (fset 'file-readable-p #'(lambda (file) nil)) (message "%s" (bar "/home/sergio/.lesshst"))) (fset 'file-exists-p file-exist
>>>>> s-p) (fset 'file-readable-p file-readable-p))
>>>>>   (let ((file-exists-p (symbol-function 'file-exists-p)) (file-readable-p (symbol-function 'file-readable-p))) (unwind-protect (progn (fset 'file-exists-p #'(lambda (file) t)) (fset 'file-re
>>>>> adable-p #'(lambda (file) nil)) (message "%s" (bar "/home/sergio/.lesshst"))) (fset 'file-exists-p file-exists-p) (fset 'file-readable-p file-readable-p)))
>>>>>   test()
>>>>>   command-line-1(("-l" "t.el" "-f" "test" "-L" "."))
>>>>>   command-line()
>>>>>   normal-top-level()
>>>>> Native elisp load failed: "file does not exists", "/home/sergio/.emacs.d/eln-cache/30.0.50-23de7b18/subr--trampoline-66696c652d7265616461626c652d70_file_readable_p_0.eln"
>>>>> --8<---------------cut here---------------end--------------->8---
>>>>> 
>>>>> Do note that this is already affecting a few packages, like buttercup
>>>>> (see https://github.com/jorgenschaefer/emacs-buttercup/issues/230) and
>>>>> emacs-web-server, for example.
>>>>> 
>>>>> Please let me know if you need more information regarding the problem.
>>>>
>>>> Thanks.
>>>>
>>>> Andrea, can you please look into this?  I guess if this happens in
>>>> Emacs 28 and on master, it also affects Emacs 29, so I hope this can
>>>> be fixed quickly and safely.  TIA.
>>>>
>>>
>>> Yep, sorry the IP of my mail provider is (temporary?) banned and I don't
>>> receive nor emacs-bugs nor emacs-devel since a couple of days :( thanks
>>> for Ccing me.
>>>
>>> So what should be going on here is that `file-exists-p' gets redefined
>>> with a non working mock function while we are trying to compile a
>>> trampoline for `file-readable-p' (it's redefined just afterward).
>>
>> Thank you for taking the time to reply and investigate this problem.
>>
>>> I don't think there is a trivial fix for this, we should rewrite
>>> `comp-trampoline-search' in C in order to have it not sensitive to the
>>> redefinition of `file-exists-p', or define another primitive like
>>> `comp-file-exists-p' (that calls directly Ffile_exists_p from C) and use
>>> that in `comp-trampoline-search'.  This would cover this specific case
>>> but potentially not others.
>>
>> Forgive my ignorance, but wouldn't we need to do that for every
>> primitive that can be compiled into a trampoline?
>
> No that's only for primitives used by the trampoline machinery (and
> specifically the part written in lisp).  Once `file-exists-p' is
> crippled the trampoline machinery is broken for all following primitives
> being redefined.

OK, understood.  What's strange to me is the fact that there are other
primitives that are affected by this problem, like 'buffer-file-name'
and 'file-readable-p', but they don't seem to call 'file-exists-p'.

>> I say that because
>> the error I've encountered above refers to 'file-readable-p', which
>> doesn't seem to call 'file-exists-p'.  FWIW, I did encounter the same
>> problem with 'file-exists-p' and 'buffer-file-name' as well, which is
>> the reason why I think solely having a 'comp-file-exists-p' wouldn't be
>> enough.
>>
>>> Fact is, Emacs can't be robust against the redefinition of all
>>> primitives (actually never was), the programmer that redefines
>>> primitives should be ready to understand the underlying Emacs machinery,
>>> and with native compilation this machinery changed a bit.
>>
>> I understand where you're coming from, but it's also important to note
>> that this behaviour was accepted without problems until the native
>> compilation feature came about, so it is understandable that we are
>> getting a lot of confusing people wondering why their tests started
>> failing now.  I believe there should be more emphasis in the
>> documentation that this problem can creep in, especially for those who
>> are relying on redefinitions for testing purposes.
>
> Agree
>
>>> So two options:
>>>
>>> * The redefinition of `file-exists-p' is tipically done for test
>>>   purposes only, we accept that and for this case we suggest to run
>>>   these specific tests setting `native-comp-enable-subr-trampolines' to
>>>   nil
>>
>> This is what I'm currently doing in Debian/Ubuntu, and will start
>> suggesting upstream maintainers to do the same.
>
> Note, I think this should be suggested only for tests redefining
> `file-exists-p'.

What about 'buffer-file-name' and 'file-readable-p'?

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
https://sergiodj.net/

bug#61880: Native compilation fails to generate trampolines on certain scenarios

[Al Haji-Ali]

Just a question and an FYI. It seems that even advising primitives issues the same warning now

(advice-add 'concat :before #'ignore)

Is this intentional?

-- Al

bug#61880: Native compilation fails to generate trampolines on certain scenarios

[Eli Zaretskii]

> From: Al Haji-Ali <abdo.haji.ali@gmail.com>
> Cc: 61880@debbugs.gnu.org
> Date: Sat, 11 Mar 2023 15:06:03 +0000
> 
> 
> Just a question and an FYI. It seems that even advising primitives issues the same warning now
> 
> (advice-add 'concat :before #'ignore)
> 
> Is this intentional?

Yes, because advising primitives which are involved in producing
primitives could produce unpredictable results.

bug#61880: Native compilation fails to generate trampolines on certain scenarios

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > > Just a question and an FYI. It seems that even advising primitives issues the same warning now
  > > 
  > > (advice-add 'concat :before #'ignore)
  > > 
  > > Is this intentional?

  > Yes, because advising primitives which are involved in producing
  > primitives could produce unpredictable results.

I'm very glad that we now warn users about this pitfall.
We have had bug reports over the years from users who fell
into it.  To "fix" it would be a mistake, but a warning
is fine.

Maybe the text of the warning should be specific in the case
of primitives like this.

-- 
Dr Richard Stallman (https://stallman.org)
Chief GNUisance of the GNU Project (https://gnu.org)
Founder, Free Software Foundation (https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

bug#61880: Native compilation fails to generate trampolines on certain scenarios

[Al Haji-Ali]

On 13/03/2023, Richard Stallman wrote:
> I'm very glad that we now warn users about this pitfall.
> We have had bug reports over the years from users who fell
> into it.  To "fix" it would be a mistake, but a warning
> is fine.

Connecting this to bug#61917, is the compilation error there perhaps caused by the advice to the primitive function `delete-region`?

Is it always wrong to advice primitives? Or only those that are "involved in producing primitives" as Eli put it? If so, how can one tell which is which?

-- Al

bug#61880: Native compilation fails to generate trampolines on certain scenarios

[Eli Zaretskii]

> From: Al Haji-Ali <abdo.haji.ali@gmail.com>
> Cc: 61880@debbugs.gnu.org, akrl@sdf.org
> Date: Tue, 14 Mar 2023 13:22:13 +0000
> 
> 
> On 13/03/2023, Richard Stallman wrote:
> > I'm very glad that we now warn users about this pitfall.
> > We have had bug reports over the years from users who fell
> > into it.  To "fix" it would be a mistake, but a warning
> > is fine.
> 
> Connecting this to bug#61917, is the compilation error there perhaps caused by the advice to the primitive function `delete-region`?
> 
> Is it always wrong to advice primitives?

Not wrong, but risky.  Especially if the advice basically subverts the
primitive, like makes it always fail or something -- this is
frequently done in various mocking frameworks for testing purposes.

> Or only those that are "involved in producing primitives" as Eli put it?

Only those, yes.  And only in a way that significantly changes what
the original primitives do.

> If so, how can one tell which is which?

The warning knows.  It only warns against those we know could be
harmful.