Package: emacs;
Reported by: Sergio Durigan Junior <sergiodj <at> sergiodj.net>
Date: Wed, 1 Mar 2023 00:15:02 UTC
Severity: normal
Done: Andrea Corallo <akrl <at> sdf.org>
Bug is archived. No further changes may be made.
View this message in rfc822 format
From: Sergio Durigan Junior <sergiodj <at> sergiodj.net> To: Andrea Corallo <akrl <at> sdf.org> Cc: Eli Zaretskii <eliz <at> gnu.org>, 61880 <at> debbugs.gnu.org Subject: bug#61880: Native compilation fails to generate trampolines on certain scenarios Date: Thu, 02 Mar 2023 18:50:50 -0500
On Thursday, March 02 2023, Andrea Corallo wrote:
> Sergio Durigan Junior <sergiodj <at> sergiodj.net> writes:
>
>> On Wednesday, March 01 2023, Andrea Corallo wrote:
>>
>>> Eli Zaretskii <eliz <at> gnu.org> writes:
>>>
>>>>> From: Sergio Durigan Junior <sergiodj <at> 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/
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.