From 0dc529175dc027c1567fb9b7cd529d29236aad44 Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Mon, 25 May 2020 20:26:14 -0700 Subject: [PATCH] Fix aborts due to GC losing pseudovectors MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit Problem reported by Eli Zaretskii (Bug#41321). * src/alloc.c (MALLOC_ALIGNMENT_BOUND): New constant. (LISP_ALIGNMENT): Lower it to avoid crashes on MinGW and similarly buggy platforms where malloc returns pointers not aligned to alignof (max_align_t). But keep it higher on platforms where this is known to work, as it helps GC performance. (MALLOC_IS_LISP_ALIGNED): Define in terms of the other two. * src/alloc.c (stacktop_sentry): * src/thread.c (run_thread): Don’t overalign or oversize stack sentries; they need to be aligned only for pointers and Lisp_Object, not for arbitrary pseudovector contents. * src/lisp.h (union emacs_align_type): New type, used for LISP_ALIGNMENT. --- src/alloc.c | 55 +++++++++++++++++++++++++++------------------------- src/lisp.h | 43 +++++++++++++++++++++++++++++++++++++++- src/thread.c | 9 +++++++-- 3 files changed, 78 insertions(+), 29 deletions(-) diff --git a/src/alloc.c b/src/alloc.c index d5a6d9167ea..602282e5704 100644 --- a/src/alloc.c +++ b/src/alloc.c @@ -112,9 +112,9 @@ along with GNU Emacs. If not, see . */ 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,28 +655,30 @@ 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); +/* A lower bound on the alignment of malloc. For better performance + this bound should be tighter. For glibc 2.26 and later a tighter + bound is known. */ +#if 2 < __GLIBC__ + (26 <= __GLIBC_MINOR__) +enum { MALLOC_ALIGNMENT_BOUND = MALLOC_ALIGNMENT }; +#else +/* A bound known to work for all Emacs porting targets. Tightening + this looser bound by using max_align_t instead of long long int + would break buggy malloc implementations like MinGW circa 2020. */ +enum { MALLOC_ALIGNMENT_BOUND = alignof (long long int) }; +#endif + +/* A lower bound on the alignment of Lisp objects. All Lisp objects + must have an address that is a multiple of LISP_ALIGNMENT; + otherwise maybe_lisp_pointer can issue false negatives, causing crashes. + It's good to make this bound tight: if Lisp objects are always + aligned more strictly than LISP_ALIGNMENT, maybe_lisp_pointer will + issue more false positives, hurting performance. */ +enum { LISP_ALIGNMENT = max (max (GCALIGNMENT, MALLOC_ALIGNMENT_BOUND), + alignof (union emacs_align_type)) }; /* 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. */ -enum { MALLOC_IS_LISP_ALIGNED = alignof (max_align_t) % LISP_ALIGNMENT == 0 }; + Lisp objects whenever N is a multiple of LISP_ALIGNMENT. */ +enum { MALLOC_IS_LISP_ALIGNED = MALLOC_ALIGNMENT_BOUND % LISP_ALIGNMENT == 0 }; /* If compiled with XMALLOC_BLOCK_INPUT_CHECK, define a symbol BLOCK_INPUT_IN_MEMORY_ALLOCATORS that is visible to the debugger. @@ -4885,9 +4887,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/lisp.h b/src/lisp.h index 85bdc172b20..937052f6df8 100644 --- a/src/lisp.h +++ b/src/lisp.h @@ -277,7 +277,8 @@ error !; allocation in a containing union that has GCALIGNED_UNION_MEMBER) and does not contain a GC-aligned struct or union, putting GCALIGNED_STRUCT after its closing '}' can help the compiler - generate better code. + generate better code. Also, such structs should be added to the + emacs_align_type union. Although these macros are reasonably portable, they are not guaranteed on non-GCC platforms, as C11 does not require support @@ -5059,6 +5060,46 @@ maybe_gc (void) maybe_garbage_collect (); } +/* 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 + components visible here. */ +union emacs_align_type +{ + struct Lisp_Bool_Vector Lisp_Bool_Vector; + struct Lisp_Char_Table Lisp_Char_Table; + struct Lisp_CondVar Lisp_CondVar; + struct Lisp_Finalizer Lisp_Finalizer; + struct Lisp_Float Lisp_Float; + struct Lisp_Hash_Table Lisp_Hash_Table; + struct Lisp_Marker Lisp_Marker; + struct Lisp_Misc_Ptr Lisp_Misc_Ptr; + struct Lisp_Mutex Lisp_Mutex; + struct Lisp_Overlay Lisp_Overlay; + struct Lisp_Sub_Char_Table Lisp_Sub_Char_Table; + struct Lisp_Subr Lisp_Subr; + struct Lisp_User_Ptr Lisp_User_Ptr; + struct Lisp_Vector Lisp_Vector; + struct thread_state thread_state; + + /* Omit the following since they would require including bignum.h, + frame.h etc., and in practice their alignments never exceed that + of the structs already listed. */ +#if 0 + struct frame frame; + struct Lisp_Bignum Lisp_Bignum; + struct Lisp_Module_Function Lisp_Module_Function; + struct Lisp_Process Lisp_Process; + struct save_window_data save_window_data; + struct scroll_bar scroll_bar; + struct terminal terminal; + struct window window; + struct xwidget xwidget; + struct xwidget_view xwidget_view; +#endif +}; + INLINE_HEADER_END #endif /* EMACS_LISP_H */ diff --git a/src/thread.c b/src/thread.c index df1a7053826..b638dd77f8b 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) -- 2.39.5