* bug#41754: [feature/native-comp] Fix crash when loading lambdas from dumps with --enable-checking.
2020-06-08 21:30 ` Andrea Corallo
@ 2020-06-09 11:54 ` Nicolas Bértolo
2020-06-09 14:07 ` Andrea Corallo
2020-06-09 15:02 ` Eli Zaretskii
0 siblings, 2 replies; 11+ messages in thread
From: Nicolas Bértolo @ 2020-06-09 11:54 UTC (permalink / raw)
To: Andrea Corallo; +Cc: 41754
[-- 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
^ permalink raw reply related [flat|nested] 11+ messages in thread