bug#50975: 28.0.60; mh-utils-tests fail with native compilation

bug#50975: 28.0.60; mh-utils-tests fail with native compilation

[Ken Brown]

Most of the tests in mh-utils-tests fail if I build the emacs-28 branch on 
Cygwin with native compilation:

$ make -C test mh-utils-tests
make: Entering directory '/home/kbrown/src/emacs/x86_64-emacs-28-native-lisp/test'
make[1]: Entering directory 
'/home/kbrown/src/emacs/x86_64-emacs-28-native-lisp/test'
   GEN      lisp/mh-e/mh-utils-tests.log
Running 16 tests (2021-10-02 14:13:13-0400, selector ‘(not (tag :unstable))’)
No MH variant found on the system
call-process mock unexpected arglist (emacs --batch -l 
/tmp/emacs-int-comp-subr--trampoline-66696c652d6469726563746f72792d70_file_directory_p_0-rg5Ucy.el)
call-process mock unexpected arglist (emacs --batch -l 
/tmp/emacs-int-comp-subr--trampoline-66696c652d6469726563746f72792d70_file_directory_p_0-f6P2ZU.el)
Test mh-folder-completion-function-02-empty backtrace:
   signal(native-compiler-error ((lambda (arg538 &optional) (let ((f #'
   comp--native-compile((lambda (arg538 &optional) (let ((f #'file-dire
   comp-trampoline-compile(file-directory-p)
   comp-subr-trampoline-install(file-directory-p)
   fset(file-directory-p #<subr file-directory-p>)
   (unwind-protect (progn (if mh-test-variant-logged-already nil (mh-va
   (let* ((vnew (make-hash-table :test #'equal)) (vnew (getenv "MH")) (
   (let ((lexical-binding t)) (let* ((vnew (make-hash-table :test #'equ
   (closure (t) nil (let ((lexical-binding t)) (let* ((vnew (make-hash-
   ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
   ert-run-test(#s(ert-test :name mh-folder-completion-function-02-empt
   ert-run-or-rerun-test(#s(ert--stats :selector (not (tag :unstable))
   ert-run-tests((not (tag :unstable)) #f(compiled-function (event-type
   ert-run-tests-batch((not (tag :unstable)))
   ert-run-tests-batch-and-exit((not (tag :unstable)))
   command-line-1(("-L" ":../../emacs-28-x86_64/test" "-l" "ert" "-l" "
   command-line()
   normal-top-level()
Test mh-folder-completion-function-02-empty condition:
     (native-compiler-error
      (lambda
        (arg538 &optional)
        (let
            ((f ...))
          (funcall f arg538)))
      "")
    FAILED   1/16  mh-folder-completion-function-02-empty (1.860486 sec)
[...]
Ran 16 tests, 4 results as expected, 12 unexpected (2021-10-02 14:13:21-0400, 
7.807508 sec)
2 expected failures

12 unexpected results:
    FAILED  mh-folder-completion-function-02-empty
    FAILED  mh-folder-completion-function-03-plus
    FAILED  mh-folder-completion-function-04-rel-folder
    FAILED  mh-folder-completion-function-05-rel-folder-slash
    FAILED  mh-folder-completion-function-06-rel-folder-slash-foo
    FAILED  mh-folder-completion-function-07-rel-folder-slash-foo-slash
    FAILED  mh-folder-completion-function-10-plus-slash-abs-folder
    FAILED  mh-folder-completion-function-11-plus-slash-abs-folder-slash-foo
    FAILED  mh-folder-completion-function-12-plus-nosuchfolder
    FAILED  mh-folder-completion-function-13-plus-slash-nosuchfolder
    FAILED  mh-sub-folders
    FAILED  mh-sub-folders-actual

This doesn't happen in a build without native compilation:

$ make -C test mh-utils-tests
make: Entering directory '/home/kbrown/src/emacs/x86_64-emacs-28/test'
make[1]: Entering directory '/home/kbrown/src/emacs/x86_64-emacs-28/test'
   GEN      lisp/mh-e/mh-utils-tests.log
Running 16 tests (2021-10-02 14:18:12-0400, selector ‘(not (or (tag :unstable) 
(tag :nativecomp)))’)
No MH variant found on the system
[...]
Ran 16 tests, 16 results as expected, 0 unexpected (2021-10-02 14:18:12-0400, 
0.081313 sec)
2 expected failures

bug#50975: 28.0.60; mh-utils-tests fail with native compilation

[Eli Zaretskii]

> From: Ken Brown <kbrown@cornell.edu>
> Date: Sat, 2 Oct 2021 14:49:04 -0400
> 
> Most of the tests in mh-utils-tests fail if I build the emacs-28 branch on 
> Cygwin with native compilation:
> 
> $ make -C test mh-utils-tests
> make: Entering directory '/home/kbrown/src/emacs/x86_64-emacs-28-native-lisp/test'
> make[1]: Entering directory 
> '/home/kbrown/src/emacs/x86_64-emacs-28-native-lisp/test'
>    GEN      lisp/mh-e/mh-utils-tests.log
> Running 16 tests (2021-10-02 14:13:13-0400, selector ‘(not (tag :unstable))’)
> No MH variant found on the system
> call-process mock unexpected arglist (emacs --batch -l 
> /tmp/emacs-int-comp-subr--trampoline-66696c652d6469726563746f72792d70_file_directory_p_0-rg5Ucy.el)
> call-process mock unexpected arglist (emacs --batch -l 
> /tmp/emacs-int-comp-subr--trampoline-66696c652d6469726563746f72792d70_file_directory_p_0-f6P2ZU.el)
> Test mh-folder-completion-function-02-empty backtrace:

Andrea, please look into this when you have time.

Thanks.

bug#50975: 28.0.60; mh-utils-tests fail with native compilation

[Robert Pluim]

>>>>> On Sat, 02 Oct 2021 22:11:55 +0300, Eli Zaretskii <eliz@gnu.org> said:

    >> From: Ken Brown <kbrown@cornell.edu>
    >> Date: Sat, 2 Oct 2021 14:49:04 -0400
    >> 
    >> Most of the tests in mh-utils-tests fail if I build the emacs-28 branch on 
    >> Cygwin with native compilation:
    >> 
    >> $ make -C test mh-utils-tests
    >> make: Entering directory '/home/kbrown/src/emacs/x86_64-emacs-28-native-lisp/test'
    >> make[1]: Entering directory 
    >> '/home/kbrown/src/emacs/x86_64-emacs-28-native-lisp/test'
    >> GEN      lisp/mh-e/mh-utils-tests.log
    >> Running 16 tests (2021-10-02 14:13:13-0400, selector ‘(not (tag :unstable))’)
    >> No MH variant found on the system
    >> call-process mock unexpected arglist (emacs --batch -l 
    >> /tmp/emacs-int-comp-subr--trampoline-66696c652d6469726563746f72792d70_file_directory_p_0-rg5Ucy.el)
    >> call-process mock unexpected arglist (emacs --batch -l 
    >> /tmp/emacs-int-comp-subr--trampoline-66696c652d6469726563746f72792d70_file_directory_p_0-f6P2ZU.el)
    >> Test mh-folder-completion-function-02-empty backtrace:

    Eli> Andrea, please look into this when you have time.

FWIW this bug is not specific to cywgin, it happens on GNU/Linux as
well:

Test mh-folder-completion-function-02-empty backtrace:
  signal(native-compiler-error ((lambda (arg3 &optional) (let ((f #'fi
  comp--native-compile((lambda (arg3 &optional) (let ((f #'file-direct
  comp-trampoline-compile(file-directory-p)
  comp-subr-trampoline-install(file-directory-p)
  #f(compiled-function () #<bytecode 0x1da46465110900b6>)()
  #f(compiled-function () #<bytecode -0x3f8e279c3d4286b>)()
  ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
  ert-run-test(#s(ert-test :name mh-folder-completion-function-02-empt
  ert-run-or-rerun-test(#s(ert--stats :selector (not ...) :tests [...
  ert-run-tests((not (or (tag :expensive-test) (tag :unstable))) #f(co
  ert-run-tests-batch((not (or (tag :expensive-test) (tag :unstable)))
  ert-run-tests-batch-and-exit((not (or (tag :expensive-test) (tag :un
  command-line-1(("-L" ":." "-l" "ert" "-l" "lisp/mh-e/mh-utils-tests"
  command-line()
  normal-top-level()
Test mh-folder-completion-function-02-empty condition:
    (native-compiler-error
     (lambda
       (arg3 &optional)
       (let
           ((f ...))
         (funcall f arg3)))
     "")
   FAILED   1/16  mh-folder-completion-function-02-empty (0.320526 sec)


(Iʼm assuming the native compiler doesnʼt like the (arg3 &optional)
bit. Didnʼt we make the byte-compiler signal an error for that
recently?)

Robert
-- 

bug#50975: 28.0.60; mh-utils-tests fail with native compilation

[Lars Ingebrigtsen]

Robert Pluim <rpluim@gmail.com> writes:

> (Iʼm assuming the native compiler doesnʼt like the (arg3 &optional)
> bit. Didnʼt we make the byte-compiler signal an error for that
> recently?)

No, that was (&rest).

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#50975: 28.0.60; mh-utils-tests fail with native compilation

[Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Ken Brown <kbrown@cornell.edu>
>> Date: Sat, 2 Oct 2021 14:49:04 -0400
>> 
>> Most of the tests in mh-utils-tests fail if I build the emacs-28 branch on 
>> Cygwin with native compilation:
>> 
>> $ make -C test mh-utils-tests
>> make: Entering directory '/home/kbrown/src/emacs/x86_64-emacs-28-native-lisp/test'
>> make[1]: Entering directory 
>> '/home/kbrown/src/emacs/x86_64-emacs-28-native-lisp/test'
>>    GEN      lisp/mh-e/mh-utils-tests.log
>> Running 16 tests (2021-10-02 14:13:13-0400, selector ‘(not (tag :unstable))’)
>> No MH variant found on the system
>> call-process mock unexpected arglist (emacs --batch -l 
>> /tmp/emacs-int-comp-subr--trampoline-66696c652d6469726563746f72792d70_file_directory_p_0-rg5Ucy.el)
>> call-process mock unexpected arglist (emacs --batch -l 
>> /tmp/emacs-int-comp-subr--trampoline-66696c652d6469726563746f72792d70_file_directory_p_0-f6P2ZU.el)
>> Test mh-folder-completion-function-02-empty backtrace:
>
> Andrea, please look into this when you have time.
>
> Thanks.

Yep will have a look, I'd bet is `file-directory-p' being redefined with
a incompatible lambda list.

  Andrea

bug#50975: 28.0.60; mh-utils-tests fail with native compilation

[Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Andrea Corallo via "Bug reports for GNU Emacs, the Swiss army knife of
text editors" <bug-gnu-emacs@gnu.org> writes:

[...]

> Yep will have a look, I'd bet is `file-directory-p' being redefined with
> a incompatible lambda list.
>
>   Andrea

Okay I see what's the issue.

`with-mh-test-env' through `mh-test-utils-setup-with-mocks' is mocking
`call-process'.

The substitute for this (`mh-test-utils-mock-call-process') is verifying
something each time is called in the assumption that `call-process' is
only triggered by the tests.  Unfortunatelly to compile a trampoline
Emacs is invoking `call-process' and the test fails.

This is not 100% trivial to solve cause redefining a primitive needs to
have `call-process' functional and is not only a matter of skipping the
test in the fake `call-process' when this is called by the native
compiler.

I've pushed 63cb65dcce to fix that, it builds the two trampolines AOT so
we have no interference with the tests.

Seems to work here, please have a try.

Thanks!

  Andrea

bug#50975: 28.0.60; mh-utils-tests fail with native compilation

[Ken Brown]

On 10/4/2021 4:57 PM, Andrea Corallo wrote:
> I've pushed 63cb65dcce to fix that, it builds the two trampolines AOT so
> we have no interference with the tests.
> 
> Seems to work here, please have a try.

Confirmed.  Thanks.

Ken

bug#50975: 28.0.60; mh-utils-tests fail with native compilation

[Stephen Gildea]

Thank you, Andrea, for the native-trampolines patch to mh-utils-tests.el.
In this patch, the test explicitly compiles trampolines before redefining
two functions that are defined in C.

Is it necessary to provide trampolines at all for these short-lived test
functions?  The following works for me:

    (mapc (lambda (x) (add-to-list 'native-comp-never-optimize-functions x))
          '(call-process file-directory-p))

Before redefining the functions, the test could create a dynamic local
binding for native-comp-never-optimize-functions and add to it as above.

If that is a reasonable approach, can we go further?  Can the
native-compile code detect that this is a test and automatically
suppress trying to compile a trampoline, without the test having
to be aware of native-compile?

 < Stephen

bug#50975: 28.0.60; mh-utils-tests fail with native compilation

[Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Stephen Gildea <stepheng+emacs@gildea.com> writes:

> Thank you, Andrea, for the native-trampolines patch to mh-utils-tests.el.
> In this patch, the test explicitly compiles trampolines before redefining
> two functions that are defined in C.
>
> Is it necessary to provide trampolines at all for these short-lived test
> functions?  The following works for me:
>
>     (mapc (lambda (x) (add-to-list 'native-comp-never-optimize-functions x))
>           '(call-process file-directory-p))

Yes disabling the trampoline generation is another option.

> Before redefining the functions, the test could create a dynamic local
> binding for native-comp-never-optimize-functions and add to it as above.
>
> If that is a reasonable approach, can we go further?  Can the
> native-compile code detect that this is a test and automatically
> suppress trying to compile a trampoline, without the test having
> to be aware of native-compile?

I don't think so.  The Emacs implementation has the right to use
`call-process' to function and in general I think we really want to test
the full implementation including trampolines as much as possible in all
running tests we can.

If we find it useful we could add a knob to disable all trampoline
generation for special cases like this.

Best Regards

  Andrea

bug#50975: 28.0.60; mh-utils-tests fail with native compilation

[Eli Zaretskii]

> Cc: 50975@debbugs.gnu.org
> Date: Tue, 05 Oct 2021 07:54:41 +0000
> From:  Andrea Corallo via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> Stephen Gildea <stepheng+emacs@gildea.com> writes:
> 
> > Thank you, Andrea, for the native-trampolines patch to mh-utils-tests.el.
> > In this patch, the test explicitly compiles trampolines before redefining
> > two functions that are defined in C.
> >
> > Is it necessary to provide trampolines at all for these short-lived test
> > functions?  The following works for me:
> >
> >     (mapc (lambda (x) (add-to-list 'native-comp-never-optimize-functions x))
> >           '(call-process file-directory-p))
> 
> Yes disabling the trampoline generation is another option.

We should document these caveats and the solutions for them in the ERT
manual.

> > Before redefining the functions, the test could create a dynamic local
> > binding for native-comp-never-optimize-functions and add to it as above.
> >
> > If that is a reasonable approach, can we go further?  Can the
> > native-compile code detect that this is a test and automatically
> > suppress trying to compile a trampoline, without the test having
> > to be aware of native-compile?
> 
> I don't think so.  The Emacs implementation has the right to use
> `call-process' to function and in general I think we really want to test
> the full implementation including trampolines as much as possible in all
> running tests we can.

I agree.  I don't think native-compilation should second-guess what
the programmer wants to do, because it's quite possible that some test
will want not to have a trampoline, for valid reasons.