bug#41754: [feature/native-comp] Fix crash when loading lambdas from dumps with --enable-checking.

["Nicolas Bértolo" <nicolasbertolo@gmail.com>]

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

Hi Andrea,

I can confirm that the issue with lambdas is fixed.

I have attached three bug fixes:

- Copy the suffixes before building the heap-based list in openp. I know this is
  not the solution I proposed in the bug report, but I couldn't adapt the code
  without increasing its complexity way too much for my liking. If you think
  this is not an appropriate solution I will come up with another one.

- Fix a simple bug caused by using cl-destructuring-bind incorrectly.

- Use a C array to keep the list of native compilation units. This one fixes the
  crashes when closing Emacs. Ideally I would figure out why iterating over
  a weak hash-table does not skip elements that were already GC'd, but I could
  not do it. I feel fixing this bug is very important, so I used a C array to
  keep the list of native compilation units.

Thanks, Nico.

El lun., 8 jun. 2020 a las 18:30, Andrea Corallo (<akrl@sdf.org>) escribió:
>
> Hi,
>
> with "4784bcc * Fix load logic for the reloading CU case (bug#41754)"
> this issue should be fixed.
>
> Reverting on my local branch "e38678b268 * Reduce the number of..." I
> did boot-strapped it with and without --enable-checking.
>
> Please have a run to confirm when you can.
>
> Thanks
>
>   Andrea
>
> --
> akrl@sdf.org

[-- Attachment #2: 0001-Fix-usage-of-cl-destructuring-bind-in-package-delete.patch --]
[-- Type: application/octet-stream, Size: 1151 bytes --]

From 1f2e5366e13ece4b1b62623222248f6967b805a3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nicol=C3=A1s=20B=C3=A9rtolo?= <nicolasbertolo@gmail.com>
Date: Mon, 8 Jun 2020 20:47:06 -0300
Subject: [PATCH] Fix usage of cl-destructuring-bind in
 package--delete-directory.

* lisp/emacs-lisp/package.el (package--delete-directory): Fix usage of
cl-destructuring-bind.
---
 lisp/emacs-lisp/package.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
index 904fc9e1094..0171fd56ffd 100644
--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -2215,7 +2215,7 @@ package--delete-directory
              (condition-case err
                  (delete-directory dir t)
                (file-error
-                (cl-destructuring-bind (reason1 reason2 filename) err
+                (cl-destructuring-bind (_ reason1 reason2 filename) err
                   (if (and (string= "Removing old name" reason1)
                            (string= "Permission denied" reason2)
                            (string-prefix-p (expand-file-name package-user-dir)
-- 
2.25.1.windows.1


[-- Attachment #3: 0001-Copy-suffixes-passed-to-openp-to-avoid-GC-crashes.-F.patch --]
[-- Type: application/octet-stream, Size: 2209 bytes --]

From 4b4fd526abe124c8a74bfa11209dd53c3a564cc7 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nicol=C3=A1s=20B=C3=A9rtolo?= <nicolasbertolo@gmail.com>
Date: Mon, 8 Jun 2020 22:01:25 -0300
Subject: [PATCH] Copy suffixes passed to 'openp' to avoid GC crashes. Fixes
 bug#41755

In openp_add_middle_dir_to_suffixes we build a heap-based list from
the passed suffixes.  It is crucial that we don't create a heap-based
cons that points to a stack-based list.

* src/lread.c (openp_add_middle_dir_to_suffixes): Copy suffixes when
building a list of middle-dirs and suffixes.
---
 src/lread.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/src/lread.c b/src/lread.c
index c127d32eb17..65d84462c4c 100644
--- a/src/lread.c
+++ b/src/lread.c
@@ -1635,21 +1635,27 @@ openp_add_middle_dir_to_suffixes (Lisp_Object suffixes)
   Lisp_Object extended_suf = Qnil;
   FOR_EACH_TAIL_SAFE (tail)
     {
-#ifdef HAVE_NATIVE_COMP
+      /*  suffixes may be a stack-based cons pointing to stack-based
+          strings.  We must copy the suffix if we are putting it into
+          a heap-based cons to avoid a dangling reference.  This would
+          lead to crashes during the GC.  */
       CHECK_STRING_CAR (tail);
       char * suf = SSDATA (XCAR (tail));
+      Lisp_Object copied_suffix = build_string (suf);
+#ifdef HAVE_NATIVE_COMP
       if (strcmp (NATIVE_ELISP_SUFFIX, suf) == 0)
         {
           CHECK_STRING (Vcomp_native_path_postfix);
           /* Here we add them in the opposite order so that nreverse
              corrects it.  */
-          extended_suf = Fcons (Fcons (Qnil, XCAR (tail)), extended_suf);
-          extended_suf = Fcons (Fcons (Vcomp_native_path_postfix, XCAR (tail)),
+          extended_suf = Fcons (Fcons (Qnil, copied_suffix), extended_suf);
+          extended_suf = Fcons (Fcons (Vcomp_native_path_postfix,
+                                       copied_suffix),
                                 extended_suf);
         }
       else
 #endif
-	extended_suf = Fcons (Fcons (Qnil, XCAR (tail)), extended_suf);
+	extended_suf = Fcons (Fcons (Qnil, copied_suffix), extended_suf);
     }
 
   suffixes = Fnreverse (extended_suf);
-- 
2.25.1.windows.1


[-- Attachment #4: 0001-Use-a-C-array-to-keep-the-list-of-live-native-compil.patch --]
[-- Type: application/octet-stream, Size: 7537 bytes --]

From 717437852b7e91a5fb9cf87dab21b4661f3bb3b4 Mon Sep 17 00:00:00 2001
From: Nicolas Bertolo <nicolasbertolo@gmail.com>
Date: Sun, 7 Jun 2020 15:54:50 -0300
Subject: [PATCH] Use a C array to keep the list of live native compilation
 units.

I was getting crashes related to the access to the hashtable when
Emacs was about to close. I couldn't debug them, so I replaced it with
a simple C array.

* src/lisp.h (allocate_native_comp_unit): register every native
compilation unit allocated.
* src/comp.h: Expose register_native_comp_unit () to lisp.h.
* src/comp.c: Remove all_loaded_comp_units_h. Replace it with a
dynamically sized array.
---
 src/comp.c | 94 +++++++++++++++++++++++++++++++-----------------------
 src/comp.h |  5 +++
 src/lisp.h |  9 ++++--
 3 files changed, 66 insertions(+), 42 deletions(-)

diff --git a/src/comp.c b/src/comp.c
index 521cadcb10c..1a7eafaaadf 100644
--- a/src/comp.c
+++ b/src/comp.c
@@ -4111,17 +4111,17 @@ helper_PSEUDOVECTOR_TYPEP_XUNTAG (Lisp_Object a, enum pvec_type code)
 
   There are two data structures used:
 
-  - The `all_loaded_comp_units_h` hashtable.
+  - The `all_loaded_comp_units` list.
 
-  This hashtable is used like an array of weak references to native
-  compilation units.  This hash table is filled by load_comp_unit ()
-  and dispose_all_remaining_comp_units () iterates over all values
-  that were not disposed by the GC and performs all disposal steps
-  when Emacs is closing.
+  This list is filled by allocate_native_comp_unit () and
+  dispose_all_remaining_comp_units () iterates over all values that
+  remain and performs all disposal steps when Emacs is closing.  The
+  dispose_comp_unit () function removes entries that were disposed by
+  the GC.
 
   - The `delayed_comp_unit_disposal_list` list.
 
-  This is were the dispose_comp_unit () function, when called by the
+  This is where the dispose_comp_unit () function, when called by the
   GC sweep stage, stores the original filenames of the disposed native
   compilation units.  This is an ad-hoc C structure instead of a Lisp
   cons because we need to allocate instances of this structure during
@@ -4136,7 +4136,35 @@ helper_PSEUDOVECTOR_TYPEP_XUNTAG (Lisp_Object a, enum pvec_type code)
 #ifdef WINDOWSNT
 #define OLD_ELN_SUFFIX_REGEXP build_string ("\\.eln\\.old\\'")
 
-static Lisp_Object all_loaded_comp_units_h;
+struct all_loaded_comp_units_s {
+  struct Lisp_Native_Comp_Unit **mem;
+  size_t size;
+  size_t capacity;
+};
+
+static struct all_loaded_comp_units_s all_loaded_comp_units;
+
+static void
+loaded_comp_units_remove (struct Lisp_Native_Comp_Unit * comp_u)
+{
+  size_t i;
+  bool found = false;
+  for (i = 0 ; i < all_loaded_comp_units.size; ++i)
+    if (all_loaded_comp_units.mem[i] == comp_u)
+      {
+        found = true;
+        break;
+      }
+  if (!found)
+    emacs_abort ();
+
+  size_t elements_on_right = all_loaded_comp_units.size - i - 1;
+  memmove (&all_loaded_comp_units.mem[i],
+           &all_loaded_comp_units.mem[i + 1],
+           elements_on_right * sizeof (struct Lisp_Native_Comp_Unit *));
+
+  all_loaded_comp_units.mem[--all_loaded_comp_units.size] = NULL;
+}
 
 /* We need to allocate instances of this struct during a GC sweep.
    This is why it can't be transformed into a simple cons.  */
@@ -4193,17 +4221,9 @@ clean_package_user_dir_of_old_comp_units (void)
 void
 dispose_all_remaining_comp_units (void)
 {
-  struct Lisp_Hash_Table *h = XHASH_TABLE (all_loaded_comp_units_h);
-
-  for (ptrdiff_t i = 0; i < HASH_TABLE_SIZE (h); ++i)
+  for (ptrdiff_t i = all_loaded_comp_units.size - 1; i >= 0; --i)
     {
-      Lisp_Object k = HASH_KEY (h, i);
-      if (!EQ (k, Qunbound))
-        {
-          Lisp_Object val = HASH_VALUE (h, i);
-          struct Lisp_Native_Comp_Unit *cu = XNATIVE_COMP_UNIT (val);
-          dispose_comp_unit (cu, false);
-        }
+      dispose_comp_unit (all_loaded_comp_units.mem[i], false);
     }
 }
 
@@ -4227,22 +4247,26 @@ finish_delayed_disposal_of_comp_units (void)
       xfree (item);
     }
 }
-#endif
 
 /* This function puts the compilation unit in the
-  `all_loaded_comp_units_h` hashmap.  */
-static void
-register_native_comp_unit (Lisp_Object comp_u)
+  `all_loaded_comp_units` list.  */
+void
+register_native_comp_unit (struct Lisp_Native_Comp_Unit * comp_u)
 {
-#ifdef WINDOWSNT
-  /* We have to do this since we can't use `gensym'. This function is
-     called early when loading a dump file and subr.el may not have
-     been loaded yet.  */
-  static intmax_t count;
+  /*  Grow the array if necessary.  */
+  if (all_loaded_comp_units.size + 1 > all_loaded_comp_units.capacity)
+    {
+      all_loaded_comp_units.capacity = max (1,
+                                            2 * all_loaded_comp_units.capacity);
+      all_loaded_comp_units.mem
+        = xrealloc (all_loaded_comp_units.mem,
+                    all_loaded_comp_units.capacity
+                    * sizeof (struct Lisp_Native_Comp_Unit *));
+    }
 
-  Fputhash (make_int (count++), comp_u, all_loaded_comp_units_h);
-#endif
+  all_loaded_comp_units.mem[all_loaded_comp_units.size++] = comp_u;
 }
+#endif
 
 /* This function disposes compilation units.  It is called during the GC sweep
    stage and when Emacs is closing.
@@ -4257,6 +4281,7 @@ dispose_comp_unit (struct Lisp_Native_Comp_Unit *comp_handle, bool delay)
   eassert (comp_handle->handle);
   dynlib_close (comp_handle->handle);
 #ifdef WINDOWSNT
+  loaded_comp_units_remove (comp_handle);
   if (!delay)
     {
       Lisp_Object dirname = internal_condition_case_1 (
@@ -4501,12 +4526,6 @@ load_comp_unit (struct Lisp_Native_Comp_Unit *comp_u, bool loading_dump,
       d_vec_len = XFIXNUM (Flength (comp_u->data_impure_vec));
       for (EMACS_INT i = 0; i < d_vec_len; i++)
 	data_imp_relocs[i] = AREF (comp_u->data_impure_vec, i);
-
-      /* If we register them while dumping we will get some entries in
-	 the hash table that will be duplicated when pdumper calls
-	 load_comp_unit.  */
-      if (!will_dump_p ())
-	register_native_comp_unit (comp_u_lisp_obj);
     }
 
   if (!loading_dump)
@@ -4818,11 +4837,6 @@ syms_of_comp (void)
   staticpro (&delayed_sources);
   delayed_sources = Qnil;
 
-#ifdef WINDOWSNT
-  staticpro (&all_loaded_comp_units_h);
-  all_loaded_comp_units_h = CALLN (Fmake_hash_table, QCweakness, Qvalue);
-#endif
-
   DEFVAR_LISP ("comp-ctxt", Vcomp_ctxt,
 	       doc: /* The compiler context.  */);
   Vcomp_ctxt = Qnil;
diff --git a/src/comp.h b/src/comp.h
index 507379bf5e6..e4196d4a5f9 100644
--- a/src/comp.h
+++ b/src/comp.h
@@ -101,6 +101,11 @@ XNATIVE_COMP_UNIT (Lisp_Object a)
 
 extern void clean_package_user_dir_of_old_comp_units (void);
 
+#ifdef WINDOWSNT
+extern void
+register_native_comp_unit (struct Lisp_Native_Comp_Unit * comp_unit);
+#endif
+
 #else /* #ifdef HAVE_NATIVE_COMP */
 
 static inline void
diff --git a/src/lisp.h b/src/lisp.h
index 22cd166c954..f64463afeec 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -4764,8 +4764,13 @@ SUBR_NATIVE_COMPILEDP (Lisp_Object a)
 INLINE struct Lisp_Native_Comp_Unit *
 allocate_native_comp_unit (void)
 {
-  return ALLOCATE_ZEROED_PSEUDOVECTOR (struct Lisp_Native_Comp_Unit,
-				       data_impure_vec, PVEC_NATIVE_COMP_UNIT);
+  struct Lisp_Native_Comp_Unit * comp_u
+    = ALLOCATE_ZEROED_PSEUDOVECTOR (struct Lisp_Native_Comp_Unit,
+                                    data_impure_vec, PVEC_NATIVE_COMP_UNIT);
+#ifdef WINDOWSNT
+  register_native_comp_unit (comp_u);
+#endif
+  return comp_u;
 }
 #else
 INLINE bool
-- 
2.25.1.windows.1