bug#46824: 28.0.50; nativecomp crash with cl-block/cl-defun

[Pip Cet <pipcet@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2354 bytes --]

On Tue, Mar 2, 2021 at 1:28 PM Eli Zaretskii <eliz@gnu.org> wrote:
> > From: Pip Cet <pipcet@gmail.com>
> > Date: Tue, 2 Mar 2021 09:16:12 +0000
> > Cc: 46824@debbugs.gnu.org, Aaron Jensen <aaronjensen@gmail.com>
> >
> > +  gcc_jit_param *params[] =
> > +    {
> > +      gcc_jit_context_new_param (comp.ctxt, NULL, comp.void_ptr_type, "buf"),
> > +    };
>
> Nitpicking: the braces here should be in column zero.

How about we compromise and put them in column two rather than four or zero? :-)

(I hope that's what you meant).

I was going to apologize for apparently messing up my cc-mode setup
here, but emacs -Q also insists on the incorrect brace placement. We
might want to fix that somehow for those Emacs developers who use this
"Emacs" thing.

> > +  /* Don't call setjmp through a function pointer (Bug#46824) */
> > +  return gcc_jit_context_new_call (comp.ctxt,
> > +                                NULL,
> > +                                gcc_jit_context_new_function
> > +                                (comp.ctxt, NULL, GCC_JIT_FUNCTION_IMPORTED,
> > +                                 comp.int_type, STR (SETJMP_NAME),
> > +                                 ARRAYELTS (params), params,
> > +                                 false),
> > +                                1,
> > +                                args);
>
> Another nit: in cases like this one I find this style more readable:
>
>   return gcc_jit_context_new_call (comp.ctxt, NULL,
>                                    gcc_jit_context_new_function (comp.ctxt,
>                                                                  NULL,
>                                                                  GCC_JIT_FUNCTION_IMPORTED,
>                                                                  ...
>
> IOW, avoid having the function's name and its arguments on separate
> lines, as I find that harder to read.
>
> Or maybe even this:
>
>   gcc_jit_rvalue *val =
>     gcc_jit_context_new_call (comp.ctxt, NULL,
>                               gcc_jit_context_new_function (comp.ctxt, NULL,
>                                                             GCC_JIT_FUNCTION_IMPORTED,
>                                                             ...),
>                               1, args);
>   return val;

I've decided on the attached, if it's okay.

> Thanks.

Thank you!

[-- Attachment #2: 0001-Don-t-call-_setjmp-through-a-function-pointer-Bug-46.patch --]
[-- Type: text/x-patch, Size: 4672 bytes --]

From 7085c6a0f83daba1dc3d7241cb598c28734d24b3 Mon Sep 17 00:00:00 2001
From: Pip Cet <pipcet@gmail.com>
Date: Sun, 28 Feb 2021 06:31:00 +0000
Subject: [PATCH] Don't call _setjmp through a function pointer (Bug#46824)

* src/comp.c (helper_link_table): Don't include SETJMP except on Windows.
(emit_setjmp): Don't use function pointers except on Windows.
(declare_runtime_imported_funcs): Don't import SETJMP at runtime.
(ABI_VERSION): Bump.
* test/src/comp-tests.el (46824-1): New test.
* test/src/comp-test-funcs.el (comp-test-46824-1-f): New function.
---
 src/comp.c                  | 24 +++++++++++++++++-------
 test/src/comp-test-funcs.el | 18 +++++++++++++++++-
 test/src/comp-tests.el      |  6 +++++-
 3 files changed, 39 insertions(+), 9 deletions(-)

diff --git a/src/comp.c b/src/comp.c
index 21d1c1a23cf0e..b24fcb5bf5a99 100644
--- a/src/comp.c
+++ b/src/comp.c
@@ -416,7 +416,7 @@ load_gccjit_if_necessary (bool mandatory)
 
 \f
 /* Increase this number to force a new Vcomp_abi_hash to be generated.  */
-#define ABI_VERSION "1"
+#define ABI_VERSION "2"
 
 /* Length of the hashes used for eln file naming.  */
 #define HASH_LENGTH 8
@@ -640,7 +640,9 @@ #define NUM_CAST_TYPES 15
     helper_PSEUDOVECTOR_TYPEP_XUNTAG,
     pure_write_error,
     push_handler,
+#ifdef WINDOWSNT
     SETJMP_NAME,
+#endif
     record_unwind_protect_excursion,
     helper_unbind_n,
     helper_save_restriction,
@@ -1929,8 +1931,19 @@ emit_setjmp (gcc_jit_rvalue *buf)
 {
 #ifndef WINDOWSNT
   gcc_jit_rvalue *args[] = {buf};
-  return emit_call (intern_c_string (STR (SETJMP_NAME)), comp.int_type, 1, args,
-                   false);
+  gcc_jit_param *params[] =
+  {
+    gcc_jit_context_new_param (comp.ctxt, NULL, comp.void_ptr_type, "buf"),
+  };
+  /* Don't call setjmp through a function pointer (Bug#46824) */
+  gcc_jit_function *f =
+    gcc_jit_context_new_function (comp.ctxt, NULL,
+				  GCC_JIT_FUNCTION_IMPORTED,
+				  comp.int_type, STR (SETJMP_NAME),
+				  ARRAYELTS (params), params,
+				  false);
+
+  return gcc_jit_context_new_call (comp.ctxt, NULL, f, 1, args);
 #else
   /* _setjmp (buf, __builtin_frame_address (0)) */
   gcc_jit_rvalue *args[2];
@@ -2662,10 +2675,7 @@ #define ADD_IMPORTED(f_name, ret_type, nargs, args)			       \
   args[1] = comp.int_type;
   ADD_IMPORTED (push_handler, comp.handler_ptr_type, 2, args);
 
-#ifndef WINDOWSNT
-  args[0] = gcc_jit_type_get_pointer (gcc_jit_struct_as_type (comp.jmp_buf_s));
-  ADD_IMPORTED (SETJMP_NAME, comp.int_type, 1, args);
-#else
+#ifdef WINDOWSNT
   args[0] = gcc_jit_type_get_pointer (gcc_jit_struct_as_type (comp.jmp_buf_s));
   args[1] = comp.void_ptr_type;
   ADD_IMPORTED (SETJMP_NAME, comp.int_type, 2, args);
diff --git a/test/src/comp-test-funcs.el b/test/src/comp-test-funcs.el
index 5bae743d1534c..edb6fa182c90b 100644
--- a/test/src/comp-test-funcs.el
+++ b/test/src/comp-test-funcs.el
@@ -1,6 +1,6 @@
 ;;; comp-test-funcs.el --- compilation unit tested by comp-tests.el -*- lexical-binding: t; -*-
 
-;; Copyright (C) 2019-2020 Free Software Foundation, Inc.
+;; Copyright (C) 2019-2021 Free Software Foundation, Inc.
 
 ;; Author: Andrea Corallo <akrl@sdf.org>
 
@@ -485,6 +485,22 @@ comp-test-46670-2-f
   (and (equal (comp-test-46670-1-f (length s)) s)
        s))
 
+(cl-defun comp-test-46824-1-f ()
+  (let ((next-repos '(1)))
+    (while t
+      (let ((recipe (car next-repos)))
+        (cl-block loop
+          (while t
+            (let ((err
+                   (condition-case e
+                       (progn
+                         (setq next-repos
+                               (cdr next-repos))
+                         (cl-return-from loop))
+                     (error e))))
+              (format "%S"
+                      (error-message-string err))))))
+      (cl-return-from comp-test-46824-1-f))))
 \f
 ;;;;;;;;;;;;;;;;;;;;
 ;; Tromey's tests ;;
diff --git a/test/src/comp-tests.el b/test/src/comp-tests.el
index 651df3329665f..81e60a62428d4 100644
--- a/test/src/comp-tests.el
+++ b/test/src/comp-tests.el
@@ -1,6 +1,6 @@
 ;;; comp-tests.el --- unit tests for src/comp.c      -*- lexical-binding: t; -*-
 
-;; Copyright (C) 2019-2020 Free Software Foundation, Inc.
+;; Copyright (C) 2019-2021 Free Software Foundation, Inc.
 
 ;; Author: Andrea Corallo <akrl@sdf.org>
 
@@ -503,6 +503,10 @@ 46670-1
   (should (equal (subr-type (symbol-function #'comp-test-46670-2-f))
                  '(function (t) (or null sequence)))))
 
+(comp-deftest 46824-1 ()
+  "<https://lists.gnu.org/archive/html/bug-gnu-emacs/2021-02/msg01949.html>"
+  (should (equal (comp-test-46824-1-f) nil)))
+
 \f
 ;;;;;;;;;;;;;;;;;;;;;
 ;; Tromey's tests. ;;
-- 
2.30.1