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

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

[Pip Cet]

On Tue, May 26, 2020 at 6:06 AM Paul Eggert <eggert@cs.ucla.edu> wrote:
>     Further fix for aborts due to GC losing pseudovectors

I don't understand this change. 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.

> +   multiple of LISP_ALIGNMENT.  This works even for buggy platforms
> +   like MinGW circa 2020, 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.  */

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. mingw64 is
fine, I believe, and the normal mingw32 version is buggy but our
header inclusion order happens to get things right, so there are no
crashes.

>  static bool
>  maybe_lisp_pointer (void *p)
>  {
> -  return (uintptr_t) p % LISP_ALIGNMENT == 0;
> +  return (uintptr_t) p % GCALIGNMENT == 0;
>  }

This is the only change that I understand is necessary.

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

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.

> +/* A type with alignment at least as large as any object that Emacs
> +   allocates.  This is not max_align_t because some platforms (e.g.,
> +   mingw) have buggy malloc implementations that do not align for
> +   max_align_t.  This union contains types of all GCALIGNED_STRUCT

I think a simple eassert (GCALIGNMENT % alignof (type) == 0) in an
(inlined, obviously) version of allocate_pseudovector should suffice
to catch this hypothetical problem.

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

[Paul Eggert]

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

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

It is a maintenance hassle, though not bad compared to the hassle of remembering
to add GCALIGNED_STRUCT and GCALIGNED_UNION_MEMBER all over the place, which we
have to do anyway.

> 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).

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

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.

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

[Pip Cet]

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

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

[Paul Eggert]

On 5/26/20 12:25 AM, Pip Cet wrote:

> Then we shouldn't mention any system at all.

No big deal either way. I thought it helpful to give an example.

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

Oh, you're right. No harm so far since LISP_ALIGNMENT is 8 on current platforms.
But this area could use some thinking if we want more efficiency on platforms
where it's 16 (so far, I've been worried only about avoiding crashes on such
machines).
>>> 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?

We should use macros, as they'll catch this at compile-time. (I don't know how
to do an eassert_reachable.) We're already using macros for
ALLOCATE_PSEUDOVECTOR and the like, so this should not be a big deal.

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

[Pip Cet]

On Tue, May 26, 2020 at 7:40 AM Paul Eggert <eggert@cs.ucla.edu> wrote:
> On 5/26/20 12:25 AM, Pip Cet wrote:
> > roundup_size still uses LISP_ALIGNMENT here, so I don't see how that's
> > true.
>
> Oh, you're right. No harm so far since LISP_ALIGNMENT is 8 on current platforms.
> But this area could use some thinking if we want more efficiency on platforms
> where it's 16 (so far, I've been worried only about avoiding crashes on such
> machines).

Absolutely. I like the idea of an allocate_aligned_pseudovector API,
though it should be stubbed out (using eassert_reachable, or
_Static_assert) for now.

> >>> 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?
>
> We should use macros, as they'll catch this at compile-time.

Okay.

> (I don't know how to do an eassert_reachable.)

Something like

extern int __unreachable_function (int);

#define eassert(cond)                    \
  ({                            \
    if (__builtin_constant_p ((cond) && __unreachable_function((cond) != 0))) \
      {                            \
    *(int *)0 = 0;                    \
    __unreachable_function (0);            \
      }                            \
    eassert_1 (cond);                    \
  })

This provides a compile-time warning (easily upgraded to an error with
the right -Werror switch), a link-time error, and a run-time
assertion. It's not pretty, but it gets the job done.

FWIW, with this (invalid) version of eassert, the following correct,
but weird, code generates a link-time error:

    case Lisp_Int:
      eassert ("should not be dumping int: is self-representing" && 0);
      abort ();

But, at -O2/-O3, on x86_64-pc-linux-gnu, no other places in the build
appear to be using eassert (0).

> We're already using macros for ALLOCATE_PSEUDOVECTOR and the like, so this should not be a big deal.

I'm finding it hard to do so, but I'm not used to working with
_Static_assert and will try again.

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

[Stefan Monnier]

> Oh, you're right. No harm so far since LISP_ALIGNMENT is 8 on current platforms.

For 64bit float, a LISP_ALIGNMENT greater than 8 would imply
a significant amount of waste (same for cons cells on 32bit systems).
So I think it'll stay at 8 for the foreseeable future (which doesn't
mean we can't decide to use large alignment for some other objects, of
course).


        Stefan

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

[Pip Cet]

On Tue, May 26, 2020 at 2:51 PM Stefan Monnier <monnier@iro.umontreal.ca> wrote:
> > Oh, you're right. No harm so far since LISP_ALIGNMENT is 8 on current platforms.
>
> For 64bit float, a LISP_ALIGNMENT greater than 8 would imply
> a significant amount of waste (same for cons cells on 32bit systems).
> So I think it'll stay at 8 for the foreseeable future (which doesn't
> mean we can't decide to use large alignment for some other objects, of
> course).

I'm hoping that if we have an allocate_aligned_pseudovector function
(or macro) we won't need LISP_ALIGNMENT at all. Which is good, since
it was misnamed (pure Lisp data and Lisp data on the stack never
obeyed LISP_ALIGNMENT. I'm suspicious about pdumper data, too, as
pdumper.c sets DUMP_ALIGNMENT to GCALIGNMENT on many practical
platforms, not LISP_ALIGNMENT). It's true that GCALIGNMENT is 1 on
some platforms, but we can change that, can't we?

I guess the recent bugs also make it impossible to do a 32-bit
--wide-int USE_LSB_TAG build. My suspicion is that that would actually
be faster, since we need only look at one 32-bit word to chase a
pointer, but it would make stack marking a lot slower...

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

[Stefan Monnier]

> I guess the recent bugs also make it impossible to do a 32-bit
> --wide-int USE_LSB_TAG build.

Indeed.

> My suspicion is that that would actually be faster,

We had USE_LSB_TAGS for wide-int, originally, but experiments showed
that for wide-int MSB_TAGs was slightly faster, presumably because the
tagging/untagging is simpler.  Speed was (so far) the only reason why we
didn't use LSB_TAGs for wide-int.


        Stefan