bug#58739: Lack of error message about number of args (?native compilation?)

[Andrea Corallo <akrl@sdf.org>]

Alan Mackenzie <acm@muc.de> writes:

> Hello, Andrea.
>
> On Sun, Oct 23, 2022 at 12:12:49 +0000, Alan Mackenzie wrote:
>> Hello, Emacs.
>
>> Firstly, note that the function desktop-buffer has exactly 12
>> parameters.
>
>> With an up to date Emacs 29:
>
>> (i) emacs -Q
>> (ii) M-x load-library RET desktop RET.
>> (iii) M-x disassemble RET desktop-buffer.
>
>> Note that this is native code.
>
>> (iv) M-: (desktop-buffer 1 2 3 4 5 6 7 8 9 10 11 12 13) RET
>
>> This gives an error message about 4 not being a list.  What it ought to
>> do is instead throw an error about the number of arguments.  This is a
>> bug.
>
>> (v) M-x load-file RET /path/to/emacs/lisp/desktop.elc.
>> (vi) M-x disassemble RET desktop-buffer.
>
>> Note that we now have byte compiled code.
>
>> (vii) M-: (desktop-buffer 1 2 3 4 5 6 7 8 9 10 11 12 13) RET
>
>> We now get a correct error message about the numbere of arguments.
>
>> As a matter of interest, I noticed this bug while byte-compiling
>> desktop.el inside Emacs.  It gave a warning message about the number of
>> parameters to desktop-buffer having changed from 12+ to 12.
>
>> Here, I suspect there's a bug in the native compilation of
>> desktop-buffer.
>
> The problem here is that (func-arity 'desktop-buffer) returns (12 . 12) on a
> byte compiled desktop.elc, but (12 . many) on the corresponding .eln file.
> This (12 . many) must be regarded as a bug, since there are no &rest
> parameters in desktop-buffer.
>
> I propose a minor amendment to the definition of MANY, such that it will
> mean "there are &rest parameters", rather than "the calling convention
> is with nargs + *args".  I have implemented this, and my patch is below.
>
> What I want to ask you to check is that in the native compiler, when
> declare_imported_func encounters a function with, say, exactly 12
> arguments, it will throw an error.  I think this is actually correct,
> since the compiler cannot know whether this function uses the subr
> calling convention of nargs + *args, or the byte coded convention of
> pushing the 12 arguments individually onto the stack.  Is throwing this
> error a good idea?

Hi Alan,

to me this fix looks like a good idea (assuming changing the definition
of MANY is acceptable).

I think also that throwing the error in 'declare_imported_func' is okay
at this point.

Just two small nits (forgive me please :) :

> Thanks!
>
> Here's my proposed patch:

[...]

>      }
> +  else if (nargs > 8)
> +      /* The calling convention for a function with a fixed number of
> +	 arguments greater than eight is different between a native
         ^^^
         I think this should be aligned with the initial 'The'
         
> +	 compiled function and a byte compiled function.  Thus we
> +	 cannot safely compile it with the native compiler.  */
> +    signal_error ("Imported function has too many arguments safely to compile natively",

I think we should break this string to stay within 78 characters (see CONTRIBUTE).

> +		  subr_sym);
>    else if (!types)
>      {
>        types = SAFE_ALLOCA (nargs * sizeof (* types));
> diff --git a/src/eval.c b/src/eval.c
> index 8810136c04..495dbb53b5 100644
> --- a/src/eval.c
> +++ b/src/eval.c
> @@ -2433,7 +2433,9 @@ eval_sub (Lisp_Object form)
>  
>        else if (XSUBR (fun)->max_args == UNEVALLED)
>  	val = (XSUBR (fun)->function.aUNEVALLED) (args_left);
> -      else if (XSUBR (fun)->max_args == MANY)
> +      else if (XSUBR (fun)->max_args == MANY
> +	       || XSUBR (fun)->max_args > 8)
> +
>  	{
>  	  /* Pass a vector of evaluated arguments.  */
>  	  Lisp_Object *vals;
> @@ -2996,7 +2998,8 @@ funcall_subr (struct Lisp_Subr *subr, ptrdiff_t numargs, Lisp_Object *args)

Likewise

>    if (numargs >= subr->min_args)
>      {
>        /* Conforming call to finite-arity subr.  */
> -      if (numargs <= subr->max_args)
> +      if (numargs <= subr->max_args
> +	  && subr->max_args <= 8)
>  	{
>  	  Lisp_Object argbuf[8];
>  	  Lisp_Object *a;
> @@ -3032,15 +3035,13 @@ funcall_subr (struct Lisp_Subr *subr, ptrdiff_t numargs, Lisp_Object *args)

Likewise

>  	      return subr->function.a8 (a[0], a[1], a[2], a[3], a[4], a[5],
>  					a[6], a[7]);

[...]

Thanks for the patch!

  Andrea