Native compilation fails to generate trampolines on certain scenarios

  • Done
  • quality assurance status badge
Details
5 participants
  • Al Haji-Ali
  • Andrea Corallo
  • Eli Zaretskii
  • Richard Stallman
  • Sergio Durigan Junior
Owner
unassigned
Submitted by
Sergio Durigan Junior
Severity
normal
S
S
Sergio Durigan Junior wrote on 1 Mar 2023 01:13
(address . bug-gnu-emacs@gnu.org)
877cw1l455.fsf@sergiodj.net
Hello,

While investigating a few bugs affecting Debian's and Ubuntu's Emacs
packages (for example,
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:

Toggle snippet (32 lines)
(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"))))

When I run it using the following Emacs:

Toggle snippet (4 lines)
GNU Emacs 30.0.50
Development version 68cc286c0495 on master branch; build date 2023-02-28.

here is the output I see:

Toggle snippet (20 lines)
$ 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"

Do note that this is already affecting a few packages, like buttercup
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
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCgAdFiEEI3pUsQKHKL8A7zH00Ot2KGX8XjYFAmP+mMYACgkQ0Ot2KGX8
XjYBlA//bA7gohJluKWD2juNgSPG1lFFSf+I4OlX5uAkd9HG9p1sWA3LRrTV4WGZ
Hv8oGKLaXHrwJqueWYAH/oRJZ2fgpEBLnRCbXh2JJu5zVTt0E8ICi+qrm0Zcu1MV
1lfLTAI4AfInIQ6u9sEP/lBrnaMxjqpWtMvcKaV30Do/4yWyd5iB52FZzbRs7cG+
s+7Ybmi1bnZfLm1nudtmwxIKJCOd/zTA4qE3iTdrsr1x7v50KDK8wwT0aGciLMHE
U0q+8gGnJkeLyp/MPkYKAL1ht/Na6TtFMWIHz2A3Gnds0F56OgddsbqiOE6hdQ70
xIRh2A81jn9WoYDXe794SktxEsrpAJ6qgjSwi2KnvuNLW7NaaPLPnlQvxqu6dGMS
fRkyB3THswGWMT0yiTFNJINxohc2P6X82l3Qqwsxaoi/5LU198BweWRH3cegPD9R
Dhwtj177ESGJ3bOE2/mMhLfL+WYLyCZUXy2xEnhszDMQ/NqbsdcyrYjDi61XW9h3
wsChJGSNzzPnEmxbMKeyEcGYbut3tzpTD2/Drml+9xqYj74KspHxoRKZJGr8BEmY
VVjjHVrGwUmfesfKNBUIWqhGVH6wHiUebDhsRdHkHsjHC+yEJFwcUBDavQVFrHU/
dynas2kFF2meE7L3qvzPPOOykBrGLhyXHqcQi9mMSk37WPp1CQk=
=Edby
-----END PGP SIGNATURE-----

E
E
Eli Zaretskii wrote on 1 Mar 2023 13:26
(address . 61880@debbugs.gnu.org)
83sfeofyi6.fsf@gnu.org
Toggle quote (86 lines)
> 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.
S
S
Sergio Durigan Junior wrote on 2 Mar 2023 00:14
(name . Andrea Corallo)(address . akrl@sdf.org)
87edq8hxom.fsf@sergiodj.net
On Wednesday, March 01 2023, Andrea Corallo wrote:

Toggle quote (103 lines)
> 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.

Toggle quote (7 lines)
> 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.

Toggle quote (5 lines)
> 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.

Toggle quote (7 lines)
> 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.

Toggle quote (3 lines)
> * 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
E
E
Eli Zaretskii wrote on 2 Mar 2023 08:05
(name . Sergio Durigan Junior)(address . sergiodj@sergiodj.net)
83v8jjeiq0.fsf@gnu.org
Toggle quote (14 lines)
> 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.

Toggle quote (23 lines)
> > 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.

Toggle quote (6 lines)
> > * 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.
E
E
Eli Zaretskii wrote on 2 Mar 2023 13:55
(name . Andrea Corallo)(address . akrl@sdf.org)
834jr3e2hw.fsf@gnu.org
Toggle quote (15 lines)
> 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?
S
S
Sergio Durigan Junior wrote on 3 Mar 2023 00:50
(name . Andrea Corallo)(address . akrl@sdf.org)
87a60uemqt.fsf@sergiodj.net
On Thursday, March 02 2023, Andrea Corallo wrote:

Toggle quote (124 lines)
> 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'.

Toggle quote (35 lines)
>> 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
S
S
Sergio Durigan Junior wrote on 3 Mar 2023 00:54
(name . Eli Zaretskii)(address . eliz@gnu.org)
875ybiemkm.fsf@sergiodj.net
On Thursday, March 02 2023, Eli Zaretskii wrote:

Toggle quote (31 lines)
>> 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
E
E
Eli Zaretskii wrote on 3 Mar 2023 08:10
(name . Sergio Durigan Junior)(address . sergiodj@sergiodj.net)
83mt4ucnsz.fsf@gnu.org
Toggle quote (16 lines)
> 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?
E
E
Eli Zaretskii wrote on 3 Mar 2023 12:32
(name . Andrea Corallo)(address . akrl@sdf.org)
831qm6cbpr.fsf@gnu.org
Toggle quote (17 lines)
> 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.

Toggle quote (13 lines)
> 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.

Toggle quote (4 lines)
> 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.
A
A
Andrea Corallo wrote on 4 Mar 2023 01:20
(name . Eli Zaretskii)(address . eliz@gnu.org)
xjfmt4txt7q.fsf@ma.sdf.org
Eli Zaretskii <eliz@gnu.org> writes:

Toggle quote (36 lines)
>> 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
E
E
Eli Zaretskii wrote on 4 Mar 2023 08:38
(name . Andrea Corallo)(address . akrl@sdf.org)
83bkl9arvo.fsf@gnu.org
Toggle quote (13 lines)
> 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.

Toggle quote (3 lines)
> Should we say that redefining this primitive breaks Emacs in general or
> be more specific on the trampoline mechanism?

The latter, I think.

Toggle quote (6 lines)
> 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?
R
R
Richard Stallman wrote on 5 Mar 2023 05:03
(name . Eli Zaretskii)(address . eliz@gnu.org)
E1pYfbJ-0006z2-7A@fencepost.gnu.org
[[[ 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)
E
E
Eli Zaretskii wrote on 5 Mar 2023 07:28
(address . rms@gnu.org)
83zg8r90ff.fsf@gnu.org
Toggle quote (15 lines)
> 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.
E
E
Eli Zaretskii wrote on 5 Mar 2023 11:44
(name . Andrea Corallo)(address . akrl@sdf.org)
83pm9n8oky.fsf@gnu.org
Toggle quote (7 lines)
> 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.
E
E
Eli Zaretskii wrote on 9 Mar 2023 10:49
(address . 61880@debbugs.gnu.org)
83cz5i2r0i.fsf@gnu.org
Toggle quote (13 lines)
> 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?
A
A
Andrea Corallo wrote on 9 Mar 2023 12:36
(name . Eli Zaretskii)(address . eliz@gnu.org)
xjfo7p2tav2.fsf@ma.sdf.org
Eli Zaretskii <eliz@gnu.org> writes:

Toggle quote (16 lines)
>> 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
Closed
S
S
Sergio Durigan Junior wrote on 11 Mar 2023 05:35
(name . Andrea Corallo)(address . akrl@sdf.org)
87r0tv6h1y.fsf@sergiodj.net
On Thursday, March 09 2023, Andrea Corallo wrote:

Toggle quote (22 lines)
> 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
A
A
Al Haji-Ali wrote on 11 Mar 2023 16:06
bug#61880: Native compilation fails to generate trampolines on certain scenarios
(address . 61880@debbugs.gnu.org)
87ilf79vl0.fsf@gmail.com
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
E
E
Eli Zaretskii wrote on 11 Mar 2023 16:34
(name . Al Haji-Ali)(address . abdo.haji.ali@gmail.com)
83356bwbd2.fsf@gnu.org
Toggle quote (11 lines)
> 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.
R
R
Richard Stallman wrote on 14 Mar 2023 04:58
(name . Eli Zaretskii)(address . eliz@gnu.org)
E1pbvoD-00056i-3b@fencepost.gnu.org
[[[ 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)
A
A
Al Haji-Ali wrote on 14 Mar 2023 14:22
875yb3wjqy.fsf@gmail.com
On 13/03/2023, Richard Stallman wrote:
Toggle quote (5 lines)
> 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
E
E
Eli Zaretskii wrote on 14 Mar 2023 17:13
(name . Al Haji-Ali)(address . abdo.haji.ali@gmail.com)
83h6uns43g.fsf@gnu.org
Toggle quote (15 lines)
> 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.

Toggle quote (2 lines)
> 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.

Toggle quote (2 lines)
> If so, how can one tell which is which?

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