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/