Re: master 9227864: Further fix for aborts due to GC losing pseudovectors

[Pip Cet <pipcet@gmail.com>]

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

On Tue, May 26, 2020 at 7:09 AM Paul Eggert <eggert@cs.ucla.edu> wrote:
> On 5/25/20 11:40 PM, Pip Cet wrote:
>
> > It makes the code much more
> > complicated, all for hypothetical systems that require alignment > 8
> > for a hypothetical 16-byte data type in a Lisp structure.
>
> It's true that we haven't run into these systems yet. However, I worry that it
> won't be all that hypothetical in the not-so-distant future, given that so many
> SIMD instructions require multiple-of-16 alignment and Emacs pseudovectors use
> system types that may require such alignment.

(I believe some AVX instructions require multiple-of-32 alignment.)

> > I think we should be specific here and say it's the mingw.org 32-bit
> > version (or whatever Eli's using) only that has problems.
>
> It can also happen with GCC 7 + glibc 2.25. Some platforms are fitfully moving
> to alignment-of-16 malloc and there are mismatches between system pieces. I'm
> not sure we can catalog all the affected systems.

Then we shouldn't mention any system at all.

> >> -   generate better code.
> >> +   generate better code.  Also, such structs should be added to the
> >> +   emacs_align_type union.
> >
> > That's going to be a maintenance nightmare, since failures to do so
> > won't actually show up on real machines, and a lot of wasted memory if
> > someone does add an AVX512 type.
>
> In current master I've fixed this so that there is zero wasted memory; the type
> is used only to calculate alignment, not to allocate memory.

roundup_size still uses LISP_ALIGNMENT here, so I don't see how that's
true. I'll look again.

> > I'd prefer a simple warning not to use long double or similarly
> > unusual types in pseudovectors, and an eassert (see below) to catch it
> > if people do that.
>
> That's not going to work if some platform uses an alignment-of-16 type in
> pthread_cond_t (or some other system type that Emacs uses).

Well, realistically, if pthread_cond_t is going to require greater
alignment, it's probably going to require alignment to a cache line:
64 bytes or more. We don't want that to silently work (wasting as much
memory as it would), we want it to generate an assertion error so we
know to move the pthread_cond_t to a malloc'd area.

> > I think a simple eassert (GCALIGNMENT % alignof (type) == 0) in an
> > (inlined, obviously) version of allocate_pseudovector should suffice
> > to catch this hypothetical problem.
>
> I assume you meant 'verify' rather than 'eassert'? That'd catch the bug at
> compile time.

I don't see how that would be possible using inline functions?

> But instead, how about an alignment argument to allocate_pseudovector, or a
> variant of allocate_pseudovector that takes such an argument? Then Emacs could
> support any alignment-greater-than-16 types that turn up.

Precisely. But until then, let's leave it as an eassert. Here's the
patch I was going to propose before you wrote.

Maybe there should be a variant of eassert that generates a
compile-time error if its argument is a compile-time constant that
evaluates to 0? Something like "eassert_reachable", maybe?

[-- Attachment #2: 0001-Fix-GCALIGNMENT.patch --]
[-- Type: text/x-patch, Size: 7782 bytes --]

diff --git a/src/alloc.c b/src/alloc.c
index d5a6d9167e..cb88db9878 100644
--- a/src/alloc.c
+++ b/src/alloc.c
@@ -112,9 +112,9 @@ Copyright (C) 1985-1986, 1988, 1993-1995, 1997-2020 Free Software
    adds sizeof (size_t) to SIZE for internal overhead, and then rounds
    up to a multiple of MALLOC_ALIGNMENT.  Emacs can improve
    performance a bit on GNU platforms by arranging for the resulting
-   size to be a power of two.  This heuristic is good for glibc 2.0
-   (1997) through at least glibc 2.31 (2020), and does not affect
-   correctness on other platforms.  */
+   size to be a power of two.  This heuristic is good for glibc 2.26
+   (2017) and later, and does not affect correctness on other
+   platforms.  */
 
 #define MALLOC_SIZE_NEAR(n) \
   (ROUNDUP (max (n, sizeof (size_t)), MALLOC_ALIGNMENT) - sizeof (size_t))
@@ -655,27 +655,13 @@ buffer_memory_full (ptrdiff_t nbytes)
 #define COMMON_MULTIPLE(a, b) \
   ((a) % (b) == 0 ? (a) : (b) % (a) == 0 ? (b) : (a) * (b))
 
-/* LISP_ALIGNMENT is the alignment of Lisp objects.  It must be at
-   least GCALIGNMENT so that pointers can be tagged.  It also must be
-   at least as strict as the alignment of all the C types used to
-   implement Lisp objects; since pseudovectors can contain any C type,
-   this is max_align_t.  On recent GNU/Linux x86 and x86-64 this can
-   often waste up to 8 bytes, since alignof (max_align_t) is 16 but
-   typical vectors need only an alignment of 8.  Although shrinking
-   the alignment to 8 would save memory, it cost a 20% hit to Emacs
-   CPU performance on Fedora 28 x86-64 when compiled with gcc -m32.  */
-enum { LISP_ALIGNMENT = alignof (union { max_align_t x;
-					 GCALIGNED_UNION_MEMBER }) };
-verify (LISP_ALIGNMENT % GCALIGNMENT == 0);
-
 /* True if malloc (N) is known to return storage suitably aligned for
    Lisp objects whenever N is a multiple of LISP_ALIGNMENT.  In
    practice this is true whenever alignof (max_align_t) is also a
-   multiple of LISP_ALIGNMENT.  This works even for x86, where some
-   platform combinations (e.g., GCC 7 and later, glibc 2.25 and
-   earlier) have bugs where alignof (max_align_t) is 16 even though
-   the malloc alignment is only 8, and where Emacs still works because
-   it never does anything that requires an alignment of 16.  */
+   multiple of LISP_ALIGNMENT.  On buggy platforms like the one
+   shipped by mingw.org circa 2020, alignof (max_align_t) is 16 but
+   malloc alignment is only 8.  Emacs still works because GCALIGNMENT
+   is only 8.  */
 enum { MALLOC_IS_LISP_ALIGNED = alignof (max_align_t) % LISP_ALIGNMENT == 0 };
 
 /* If compiled with XMALLOC_BLOCK_INPUT_CHECK, define a symbol
@@ -4657,12 +4643,12 @@ mark_maybe_objects (Lisp_Object const *array, ptrdiff_t nelts)
    collected, and false otherwise (i.e., false if it is easy to see
    that P cannot point to Lisp data that can be garbage collected).
    Symbols are implemented via offsets not pointers, but the offsets
-   are also multiples of LISP_ALIGNMENT.  */
+   are also multiples of GCALIGNMENT.  */
 
 static bool
 maybe_lisp_pointer (void *p)
 {
-  return (uintptr_t) p % LISP_ALIGNMENT == 0;
+  return (uintptr_t) p % GCALIGNMENT == 0;
 }
 
 /* If P points to Lisp data, mark that as live if it isn't already
@@ -4885,9 +4871,10 @@ test_setjmp (void)
    as a stack scan limit.  */
 typedef union
 {
-  /* Align the stack top properly.  Even if !HAVE___BUILTIN_UNWIND_INIT,
-     jmp_buf may not be aligned enough on darwin-ppc64.  */
-  max_align_t o;
+  /* Make sure stack_top and m_stack_bottom are properly aligned as GC
+     expects.  */
+  Lisp_Object o;
+  void *p;
 #ifndef HAVE___BUILTIN_UNWIND_INIT
   sys_jmp_buf j;
   char c;
diff --git a/src/font.c b/src/font.c
index ab00402b40..a2df8f9d6d 100644
--- a/src/font.c
+++ b/src/font.c
@@ -158,8 +158,10 @@ font_make_spec (void)
   Lisp_Object font_spec;
   struct font_spec *spec
     = ((struct font_spec *)
-       allocate_pseudovector (VECSIZE (struct font_spec),
-			      FONT_SPEC_MAX, FONT_SPEC_MAX, PVEC_FONT));
+       allocate_aligned_pseudovector (VECSIZE (struct font_spec),
+				      FONT_SPEC_MAX, FONT_SPEC_MAX,
+				      alignof (struct font_spec),
+				      PVEC_FONT));
   XSETFONT (font_spec, spec);
   return font_spec;
 }
@@ -170,8 +172,9 @@ font_make_entity (void)
   Lisp_Object font_entity;
   struct font_entity *entity
     = ((struct font_entity *)
-       allocate_pseudovector (VECSIZE (struct font_entity),
-			      FONT_ENTITY_MAX, FONT_ENTITY_MAX, PVEC_FONT));
+       allocate_aligned_pseudovector (VECSIZE (struct font_entity),
+				      FONT_ENTITY_MAX, FONT_ENTITY_MAX,
+				      alignof (struct font_entity),PVEC_FONT));
   XSETFONT (font_entity, entity);
   return font_entity;
 }
diff --git a/src/lisp.h b/src/lisp.h
index 85bdc172b2..6093f664ac 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -3976,26 +3976,44 @@ make_nil_vector (ptrdiff_t size)
 extern struct Lisp_Vector *allocate_pseudovector (int, int, int,
 						  enum pvec_type);
 
+/* Alignment needed for memory blocks that are allocated via malloc
+   and that contain Lisp objects.  On typical hosts malloc already
+   aligns sufficiently, but extra work is needed on oddball hosts
+   where Emacs would crash if malloc returned a non-GCALIGNED pointer.  */
+enum { LISP_ALIGNMENT = alignof (union { GCALIGNED_UNION_MEMBER }) };
+verify (LISP_ALIGNMENT % GCALIGNMENT == 0);
+
+INLINE
+struct Lisp_Vector *
+allocate_aligned_pseudovector (int memlen, int lisplen, int zerolen,
+			       int alignment, enum pvec_type tag)
+{
+  eassert (LISP_ALIGNMENT % alignment == 0);
+  return allocate_pseudovector (memlen, lisplen, zerolen, tag);
+}
+
 /* Allocate uninitialized pseudovector with no Lisp_Object slots.  */
 
 #define ALLOCATE_PLAIN_PSEUDOVECTOR(type, tag) \
-  ((type *) allocate_pseudovector (VECSIZE (type), 0, 0, tag))
+  ((type *) allocate_aligned_pseudovector (VECSIZE (type), 0, 0, \
+					   alignof (type), tag))
 
 /* Allocate partially initialized pseudovector where all Lisp_Object
    slots are set to Qnil but the rest (if any) is left uninitialized.  */
 
-#define ALLOCATE_PSEUDOVECTOR(type, field, tag)			       \
-  ((type *) allocate_pseudovector (VECSIZE (type),		       \
-				   PSEUDOVECSIZE (type, field),	       \
-				   PSEUDOVECSIZE (type, field), tag))
+#define ALLOCATE_PSEUDOVECTOR(type, field, tag)				\
+  ((type *) allocate_aligned_pseudovector				\
+   (VECSIZE (type), PSEUDOVECSIZE (type, field),			\
+    PSEUDOVECSIZE (type, field),	alignof (type), tag))
 
 /* Allocate fully initialized pseudovector where all Lisp_Object
    slots are set to Qnil and the rest (if any) is zeroed.  */
 
-#define ALLOCATE_ZEROED_PSEUDOVECTOR(type, field, tag)		       \
-  ((type *) allocate_pseudovector (VECSIZE (type),		       \
-				   PSEUDOVECSIZE (type, field),	       \
-				   VECSIZE (type), tag))
+#define ALLOCATE_ZEROED_PSEUDOVECTOR(type, field, tag)			\
+  ((type *) allocate_aligned_pseudovector (VECSIZE (type),		\
+					   PSEUDOVECSIZE (type, field),	\
+					   VECSIZE (type),		\
+					   alignof (type), tag))
 
 extern bool gc_in_progress;
 extern Lisp_Object make_float (double);
diff --git a/src/thread.c b/src/thread.c
index df1a705382..b638dd77f8 100644
--- a/src/thread.c
+++ b/src/thread.c
@@ -717,12 +717,17 @@ run_thread (void *state)
 {
   /* Make sure stack_top and m_stack_bottom are properly aligned as GC
      expects.  */
-  max_align_t stack_pos;
+  union
+  {
+    Lisp_Object o;
+    void *p;
+    char c;
+  } stack_pos;
 
   struct thread_state *self = state;
   struct thread_state **iter;
 
-  self->m_stack_bottom = self->stack_top = (char *) &stack_pos;
+  self->m_stack_bottom = self->stack_top = &stack_pos.c;
   self->thread_id = sys_thread_self ();
 
   if (self->thread_name)