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

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

[Alan Mackenzie]

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.

-- 
Alan Mackenzie (Nuremberg, Germany).

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

[Alan Mackenzie]

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?

Thanks!

Here's my proposed patch:


Amend the meaning of "MANY"; this solves bug #58739

Make "MANY" mean there are &rest parameters.  The previous definition left
ambiguous whether a subr with over 8 parameters also had &rest parameters.

* lisp/emacs-lisp/comp.el (comp-prepare-args-for-top-level): Return the number
of parameters rather than `many' when this is a fixed number over 8.

* src/comp.c (declare_imported_func): Throw an error when an imported function
has a fixed number over 8 of parameters, since we cannot compile a call to
this which will work for both byte code and native code.

* src/eval.c (eval_sub, funcall_subr): Where we test for MANY, also test the
number of arguments against 8.
(funcall_subr): Resolve the previous inability to call subr's with a fixed
number over 8 of arguments.


diff --git a/lisp/emacs-lisp/comp.el b/lisp/emacs-lisp/comp.el
index 5a05fe4854..f7e118616d 100644
--- a/lisp/emacs-lisp/comp.el
+++ b/lisp/emacs-lisp/comp.el
@@ -2057,9 +2057,10 @@ comp-prepare-args-for-top-level
   "Lexically-scoped FUNCTION."
   (let ((args (comp-func-l-args function)))
     (cons (make-comp-mvar :constant (comp-args-base-min args))
-          (make-comp-mvar :constant (if (comp-args-p args)
-                                        (comp-args-max args)
-                                      'many)))))
+          (make-comp-mvar :constant (cond
+                                     ((comp-args-p args) (comp-args-max args))
+                                     ((comp-nargs-rest args) 'many)
+                                     (t (comp-nargs-nonrest args)))))))
 
 (cl-defmethod comp-prepare-args-for-top-level ((function comp-func-d))
   "Dynamically scoped FUNCTION."
diff --git a/src/comp.c b/src/comp.c
index 14012634cc..8c9db20e72 100644
--- a/src/comp.c
+++ b/src/comp.c
@@ -999,6 +999,13 @@ declare_imported_func (Lisp_Object subr_sym, gcc_jit_type *ret_type,
       types = SAFE_ALLOCA (nargs * sizeof (* types));
       types[0] = comp.lisp_obj_type;
     }
+  else if (nargs > 8)
+      /* The calling convention for a function with a fixed number of
+	 arguments greater than eight is different between a native
+	 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",
+		  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)
   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)
 	      return subr->function.a8 (a[0], a[1], a[2], a[3], a[4], a[5],
 					a[6], a[7]);
 	    default:
-	      /* If a subr takes more than 8 arguments without using MANY
-		 or UNEVALLED, we need to extend this function to support it.
-		 Until this is done, there is no way to call the function.  */
-	      emacs_abort ();
+	      emacs_abort (); 	/* Can't happen. */
 	    }
 	}
 
       /* Call to n-adic subr.  */
-      if (subr->max_args == MANY)
+      if (subr->max_args == MANY
+	  || subr->max_args > 8)
 	return subr->function.aMANY (numargs, args);
     }
 
diff --git a/src/lisp.h b/src/lisp.h
index 5f6721595c..881548ead4 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -3187,10 +3187,11 @@ CHECK_SUBR (Lisp_Object x)
  `minargs' should be a number, the minimum number of arguments allowed.
  `maxargs' should be a number, the maximum number of arguments allowed,
     or else MANY or UNEVALLED.
-    MANY means pass a vector of evaluated arguments,
-	 in the form of an integer number-of-arguments
-	 followed by the address of a vector of Lisp_Objects
-	 which contains the argument values.
+    MANY means there are &rest arguments.  Here we pass a vector
+        of evaluated arguments in the form of an integer
+        number-of-arguments followed by the address of a vector of
+        Lisp_Objects which contains the argument values.  (We also use
+        this convention when calling a subr with more than 8 parameters.)
     UNEVALLED means pass the list of unevaluated arguments
  `intspec' says how interactive arguments are to be fetched.
     If the string starts with a `(', `intspec' is evaluated and the resulting


-- 
Alan Mackenzie (Nuremberg, Germany).

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

[Andrea Corallo]

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

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

[Alan Mackenzie]

Hello, Andrea.

On Tue, Oct 25, 2022 at 20:15:38 +0000, Andrea Corallo wrote:
> Alan Mackenzie <acm@muc.de> writes:

> > On Sun, Oct 23, 2022 at 12:12:49 +0000, Alan Mackenzie wrote:

[ .... ]

> >> 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).

We'll see what Eli says.

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

Apologies at this point.  I should have produced an error from this
before bothering you.  I was unable to produce such an error, and I've
spent the last few days understanding what happens here, with this
result:

A call from a native compiled function is always in the form nargs +
*args, regardless of whether there are more than 8 arguments or not.
More accurately, the call to an unknown type of function (.eln/.elc/.el)
puts the function (in some form, I don't know exactly what) at element 0
of *args, and the arguments themselves starting at element 1 of *args.
It then calls Ffuncall (or something like it).

When the called function is a byte-code function, further down the call
stack exec_byte_code gets called with this (nargs + 1) + *args.
exec_byte_code then pushes the arguments onto the stack before
interpreting the byte code.

So this "problem" with the native compiler not knowing what call sequence
to generate isn't a problem at all.  It's all dealt with at run time.  No
doubt this slows down Emacs quite a bit, but it's safe.

So I'll take that bit out of my patch, and commit the rest of it to
master, and see what happens from there.

> Just two small nits (forgive me please :) :

Thanks for these, I've incorporated them into my amended source code.

[ .... ]

> Thanks for the patch!

>   Andrea

-- 
Alan Mackenzie (Nuremberg, Germany).

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

[Eli Zaretskii]

> Cc: 58739@debbugs.gnu.org
> Date: Sat, 29 Oct 2022 12:04:36 +0000
> From: Alan Mackenzie <acm@muc.de>
> 
> > to me this fix looks like a good idea (assuming changing the definition
> > of MANY is acceptable).
> 
> We'll see what Eli says.

About what, may I ask?

bug#58739: Fixed

[Alan Mackenzie]

Bug fixed.

-- 
Alan Mackenzie (Nuremberg, Germany).

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

[Alan Mackenzie]

On Sat, Oct 29, 2022 at 15:48:35 +0300, Eli Zaretskii wrote:
> > Cc: 58739@debbugs.gnu.org
> > Date: Sat, 29 Oct 2022 12:04:36 +0000
> > From: Alan Mackenzie <acm@muc.de>

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

> > We'll see what Eli says.

> About what, may I ask?

My patch for this bug.

As a quick summary, the cause of spurious warning messages in the byte
compiler was subr-arity returning (12 . many) rather than the correct
(12 . 12) for a native compiled function with exactly 12 parameters.

I've corrected this, by making subr-arity return (12 . 12), and I
committed the patch earlier on this afternoon.

In the process, MANY came to be defined to mean "has &rest parameters".

I think you'll be interested in the patch.

-- 
Alan Mackenzie (Nuremberg, Germany).

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

[Andrea Corallo]

Alan Mackenzie <acm@muc.de> writes:

> Hello, Andrea.
>
> On Tue, Oct 25, 2022 at 20:15:38 +0000, Andrea Corallo wrote:
>> Alan Mackenzie <acm@muc.de> writes:
>
>> > On Sun, Oct 23, 2022 at 12:12:49 +0000, Alan Mackenzie wrote:
>
> [ .... ]
>
>> >> 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).
>
> We'll see what Eli says.
>
>> I think also that throwing the error in 'declare_imported_func' is okay
>> at this point.
>
> Apologies at this point.  I should have produced an error from this
> before bothering you.  I was unable to produce such an error, and I've
> spent the last few days understanding what happens here, with this
> result:
>
> A call from a native compiled function is always in the form nargs +
> *args, regardless of whether there are more than 8 arguments or not.
> More accurately, the call to an unknown type of function (.eln/.elc/.el)
> puts the function (in some form, I don't know exactly what) at element 0
> of *args, and the arguments themselves starting at element 1 of *args.
> It then calls Ffuncall (or something like it).
>
> When the called function is a byte-code function, further down the call
> stack exec_byte_code gets called with this (nargs + 1) + *args.
> exec_byte_code then pushes the arguments onto the stack before
> interpreting the byte code.
>
> So this "problem" with the native compiler not knowing what call sequence
> to generate isn't a problem at all.  It's all dealt with at run time.  No
> doubt this slows down Emacs quite a bit, but it's safe.
>
> So I'll take that bit out of my patch, and commit the rest of it to
> master, and see what happens from there.

Hi Alan,

That's correct.  As you prefer, we could also keep it as assertion.

Thanks

  Andrea