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

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

[Nicolas Bértolo]

Hi Andrea,

I have seen that a lambda may be loaded many times when loading a dump. Is this
expected? I don't know enough to tell. If it is, the attached patch prevents an
assertion failure when building with --enable-checking.

Nico.

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

[Andrea Corallo]

Nicolas Bértolo <nicolasbertolo@gmail.com> writes:

> Hi Andrea,
>
> I have seen that a lambda may be loaded many times when loading a dump. Is this
> expected? I don't know enough to tell. If it is, the attached patch prevents an
> assertion failure when building with --enable-checking.
>
> Nico.

Hi Nico,

the patch is missing.  Are you referring to a check in
check_comp_unit_relocs?

  Andrea

-- 
akrl@sdf.org

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

[Nicolas Bértolo]

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

Oops,

Here it is.

Nico.

[-- Attachment #2: 0001-Don-t-crash-when-a-lambda-has-been-already-loaded.patch --]
[-- Type: application/octet-stream, Size: 1003 bytes --]

From 343511d91eac3e85f57803fca821632858bb9ba9 Mon Sep 17 00:00:00 2001
From: Nicolas Bertolo <nicolasbertolo@gmail.com>
Date: Sun, 7 Jun 2020 14:40:14 -0300
Subject: [PATCH] Don't crash when a lambda has been already loaded.

* src/pdumper.c (dump_do_dump_relocation): Skip lambdas that have been
loaded already.
---
 src/pdumper.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/src/pdumper.c b/src/pdumper.c
index 92ac96a8faa..594b60b143b 100644
--- a/src/pdumper.c
+++ b/src/pdumper.c
@@ -5365,6 +5365,10 @@ dump_do_dump_relocation (const uintptr_t dump_base,
 	    XSETSUBR (tem, subr);
 	    Lisp_Object *fixup =
 	      &(comp_u->data_imp_relocs[XFIXNUM (lambda_data_idx)]);
+            bool lambda_already_loaded = SUBRP (*fixup)
+              && XSUBR (*fixup)->function.a0 == func;
+            if (lambda_already_loaded)
+              return;
 	    eassert (EQ (*fixup, Qlambda_fixup));
 	    *fixup = tem;
 	    Fputhash (tem, Qnil, comp_u->lambda_gc_guard);
-- 
2.25.1.windows.1

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

[Andrea Corallo]

Nicolas Bértolo <nicolasbertolo@gmail.com> writes:

> Hi Andrea,
>
> I have seen that a lambda may be loaded many times when loading a dump. Is this
> expected? I don't know enough to tell. If it is, the attached patch prevents an
> assertion failure when building with --enable-checking.
>
> Nico.

Hi,

had a look, this is a real bug.  Luckily except for failing the test
should not introduce instability in normal builds, still the test is
correct.

I'll come up with the fix once is tested.

Thanks

  Andrea

-- 
akrl@sdf.org

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

[Andrea Corallo]

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

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

[Nicolas Bértolo]

[-- 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

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

[Andrea Corallo]

Nicolas Bértolo <nicolasbertolo@gmail.com> writes:

> Hi Andrea,
>
> I can confirm that the issue with lambdas is fixed.

Good closing this.

I pushed the `package--delete-directory' fix.  Please post the patches
you are submitting in the thread you have discussed the related issue so
they can be effectively reviewed by people you have discussed with.

  Andrea
 
-- 
akrl@sdf.org

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

[Nicolas Bértolo]

> I pushed the `package--delete-directory' fix.  Please post the patches
> you are submitting in the thread you have discussed the related issue so
> they can be effectively reviewed by people you have discussed with.

Sorry. I apologize. I thought the discussion in those threads was over, after
having identified the bug, that's why I sent them directly to you. I didn't mean
to sidestep anyone.

Nico.

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

[Eli Zaretskii]

> From: Nicolas Bértolo <nicolasbertolo@gmail.com>
> Date: Tue, 9 Jun 2020 08:54:45 -0300
> Cc: 41754@debbugs.gnu.org
> 
> - 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.

This conses a string for each extension each time through the loop,
doesn't it?  Is that really necessary?

Maybe we should take a step back and consider restructuring the code a
bit.  AFAIU, you cons the extended_suf list to be able to use the
FOR_* loops that manipulate lists, is that correct?  If so, could it
be that removing that constraint will lead to a more elegant and less
expensive code?  After all, all this function does is to append STR to
each directory in PATH, then try finding the resulting file with or
without one of the extensions in SUFFIXES.  Could we produce the file
name to probe without walking a single list?

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

[Nicolas Bértolo]

> This conses a string for each extension each time through the loop,
> doesn't it?  Is that really necessary?

It does. I thought that keeping the code simple (aka using the FOR_* macros)
more important than saving a few memory allocations, especially when this
function accesses the filesystem, which should be much more expensive.

> AFAIU, you cons the extended_suf list to be able to use the
> FOR_* loops that manipulate lists, is that correct?

Exactly.

> If so, could it be that removing that constraint will lead to a more elegant
> and less expensive code? After all, all this function does is to append STR to
> each directory in PATH, then try finding the resulting file with or without one
> of the extensions in SUFFIXES. Could we produce the file name to probe without
> walking a single list?

I'll come up with a new version taking your suggestions into account.

Thanks, Nico.

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

[Eli Zaretskii]

> From: Nicolas Bértolo <nicolasbertolo@gmail.com>
> Date: Tue, 9 Jun 2020 12:11:15 -0300
> Cc: Andrea Corallo <akrl@sdf.org>, 41754@debbugs.gnu.org
> 
> > If so, could it be that removing that constraint will lead to a more elegant
> > and less expensive code? After all, all this function does is to append STR to
> > each directory in PATH, then try finding the resulting file with or without one
> > of the extensions in SUFFIXES. Could we produce the file name to probe without
> > walking a single list?
> 
> I'll come up with a new version taking your suggestions into account.

Please don't see what I wrote as a requirement.  Just take it into
consideration and see if it leads to something reasonable.  If you
feel that the result will not be significantly better, there's no need
to come up with yet another solution; I'd hate to ask you to do
redundant work unless my idea really happens to lead to an elegant
solution.

And thanks for your continued work on these matters, highly
appreciated.