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/