[Emacs-diffs] master d2a07b9: Fix struct thread alignment on FreeBSD x86

[Paul Eggert]

branch: master
commit d2a07b9a82a632e8baa179c667a98d275e5f6973
Author: Paul Eggert <address@hidden>
Commit: Paul Eggert <address@hidden>

    Fix struct thread alignment on FreeBSD x86
    
    Problem reported by Joseph Mingrone in:
    https://lists.gnu.org/r/emacs-devel/2018-10/msg00238.html
    While we’re at it, apply a similar fix to struct Lisp_Subr; this
    removes the need for GCALIGNED_STRUCT_MEMBER and thus can shrink
    struct Lisp_Subr a bit.
    * configure.ac (HAVE_STRUCT_ATTRIBUTE_ALIGNED): Bring back this macro.
    Although used only for performance (not to actually align
    structures), we might as well take advantage of it.
    * src/lisp.h (GCALIGNED_STRUCT_MEMBER): Remove; all uses removed.
    (union Aligned_Lisp_Subr): New type, like struct Lisp_Subr but aligned.
    * src/lisp.h (XSUBR, DEFUN):
    * src/lread.c (defsubr): Use it.  All callers changed.
    * src/thread.c (union aligned_thread_state): New type.
    (main_thread): Now of this type, so it’s aligned.
    All uses changed.
    * src/xmenu.c (syms_of_xmenu) [USE_GTK || USE_X_TOOLKIT]:
    Adjust to union Aligned_Lisp_Subr change.
---
 configure.ac | 16 ++++++++++++++++
 src/lisp.h   | 37 +++++++++++++++++++------------------
 src/lread.c  |  3 ++-
 src/thread.c | 47 +++++++++++++++++++++++++++--------------------
 src/xmenu.c  |  2 +-
 5 files changed, 65 insertions(+), 40 deletions(-)

diff --git a/configure.ac b/configure.ac
index 3a61090..50e3333 100644
--- a/configure.ac
+++ b/configure.ac
@@ -5207,6 +5207,22 @@ else
 fi
 AC_SUBST(LIBXMENU)
 
+AC_CACHE_CHECK([for struct alignment],
+  [emacs_cv_struct_alignment],
+  [AC_COMPILE_IFELSE(
+     [AC_LANG_PROGRAM([[#include <stddef.h>
+                       struct s { char c; } __attribute__ ((aligned (8)));
+                       struct t { char c; struct s s; };
+                       char verify[offsetof (struct t, s) == 8 ? 1 : -1];
+                     ]])],
+     [emacs_cv_struct_alignment=yes],
+     [emacs_cv_struct_alignment=no])])
+if test "$emacs_cv_struct_alignment" = yes; then
+  AC_DEFINE([HAVE_STRUCT_ATTRIBUTE_ALIGNED], 1,
+    [Define to 1 if 'struct __attribute__ ((aligned (N)))' aligns the
+     structure to an N-byte boundary.])
+fi
+
 if test "${GNU_MALLOC}" = "yes" ; then
   AC_DEFINE(GNU_MALLOC, 1,
            [Define to 1 if you want to use the GNU memory allocator.])
diff --git a/src/lisp.h b/src/lisp.h
index 145901d..eb67626 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -229,7 +229,7 @@ extern bool suppress_checking EXTERNALLY_VISIBLE;
    USE_LSB_TAG not only requires the least 3 bits of pointers returned by
    malloc to be 0 but also needs to be able to impose a mult-of-8 alignment
    on some non-GC Lisp_Objects, all of which are aligned via
-   GCALIGNED_UNION_MEMBER, GCALIGNED_STRUCT_MEMBER, and GCALIGNED_STRUCT.  */
+   GCALIGNED_UNION_MEMBER.  */
 
 enum Lisp_Bits
   {
@@ -284,15 +284,14 @@ error !;
 # define GCALIGNMENT 1
 #endif
 
-/* If a struct is always allocated by the GC and is therefore always
-   GC-aligned, put GCALIGNED_STRUCT after its closing '}'; this can
-   help the compiler generate better code.
+/* To cause a union to have alignment of at least GCALIGNMENT, put
+   GCALIGNED_UNION_MEMBER in its member list.
 
-   To cause a union to have alignment of at least GCALIGNMENT, put
-   GCALIGNED_UNION_MEMBER in its member list.  Similarly for a struct
-   and GCALIGNED_STRUCT_MEMBER, although this may make the struct a
-   bit bigger on non-GCC platforms.  Any struct using
-   GCALIGNED_STRUCT_MEMBER should also use GCALIGNED_STRUCT.
+   If a struct is always GC-aligned (either by the GC, or via
+   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.
 
    Although these macros are reasonably portable, they are not
    guaranteed on non-GCC platforms, as C11 does not require support
@@ -306,10 +305,8 @@ error !;
 
 #define GCALIGNED_UNION_MEMBER char alignas (GCALIGNMENT) gcaligned;
 #if HAVE_STRUCT_ATTRIBUTE_ALIGNED
-# define GCALIGNED_STRUCT_MEMBER
 # define GCALIGNED_STRUCT __attribute__ ((aligned (GCALIGNMENT)))
 #else
-# define GCALIGNED_STRUCT_MEMBER GCALIGNED_UNION_MEMBER
 # define GCALIGNED_STRUCT
 #endif
 #define GCALIGNED(type) (alignof (type) % GCALIGNMENT == 0)
@@ -1970,9 +1967,13 @@ struct Lisp_Subr
     const char *symbol_name;
     const char *intspec;
     EMACS_INT doc;
-    GCALIGNED_STRUCT_MEMBER
   } GCALIGNED_STRUCT;
-verify (GCALIGNED (struct Lisp_Subr));
+union Aligned_Lisp_Subr
+  {
+    struct Lisp_Subr s;
+    GCALIGNED_UNION_MEMBER
+  };
+verify (GCALIGNED (union Aligned_Lisp_Subr));
 
 INLINE bool
 SUBRP (Lisp_Object a)
@@ -1984,7 +1985,7 @@ INLINE struct Lisp_Subr *
 XSUBR (Lisp_Object a)
 {
   eassert (SUBRP (a));
-  return XUNTAG (a, Lisp_Vectorlike, struct Lisp_Subr);
+  return &XUNTAG (a, Lisp_Vectorlike, union Aligned_Lisp_Subr)->s;
 }
 
 enum char_table_specials
@@ -2952,15 +2953,15 @@ CHECK_FIXNUM_CDR (Lisp_Object x)
 /* This version of DEFUN declares a function prototype with the right
    arguments, so we can catch errors with maxargs at compile-time.  */
 #define DEFUN(lname, fnname, sname, minargs, maxargs, intspec, doc)    \
-   static struct Lisp_Subr sname =                             \
-     { { PVEC_SUBR << PSEUDOVECTOR_AREA_BITS },                                
\
+   static union Aligned_Lisp_Subr sname =                              \
+     {{{ PVEC_SUBR << PSEUDOVECTOR_AREA_BITS },                                
\
        { .a ## maxargs = fnname },                                     \
-       minargs, maxargs, lname, intspec, 0};                           \
+       minargs, maxargs, lname, intspec, 0}};                          \
    Lisp_Object fnname
 
 /* defsubr (Sname);
    is how we define the symbol for function `name' at start-up time.  */
-extern void defsubr (struct Lisp_Subr *);
+extern void defsubr (union Aligned_Lisp_Subr *);
 
 enum maxargs
   {
diff --git a/src/lread.c b/src/lread.c
index 62616cb..5f38714 100644
--- a/src/lread.c
+++ b/src/lread.c
@@ -4409,8 +4409,9 @@ init_obarray (void)
 }
 
 void
-defsubr (struct Lisp_Subr *sname)
+defsubr (union Aligned_Lisp_Subr *aname)
 {
+  struct Lisp_Subr *sname = &aname->s;
   Lisp_Object sym, tem;
   sym = intern_c_string (sname->symbol_name);
   XSETPVECTYPE (sname, PVEC_SUBR);
diff --git a/src/thread.c b/src/thread.c
index 3674af0..6612697 100644
--- a/src/thread.c
+++ b/src/thread.c
@@ -27,11 +27,18 @@ along with GNU Emacs.  If not, see 
<https://www.gnu.org/licenses/>.  */
 #include "syssignal.h"
 #include "keyboard.h"
 
-static struct thread_state main_thread;
+union aligned_thread_state
+{
+  struct thread_state s;
+  GCALIGNED_UNION_MEMBER
+};
+verify (GCALIGNED (union aligned_thread_state));
+
+static union aligned_thread_state main_thread;
 
-struct thread_state *current_thread = &main_thread;
+struct thread_state *current_thread = &main_thread.s;
 
-static struct thread_state *all_threads = &main_thread;
+static struct thread_state *all_threads = &main_thread.s;
 
 static sys_mutex_t global_lock;
 
@@ -113,7 +120,7 @@ maybe_reacquire_global_lock (void)
   /* SIGINT handler is always run on the main thread, see
      deliver_process_signal, so reflect that in our thread-tracking
      variables.  */
-  current_thread = &main_thread;
+  current_thread = &main_thread.s;
 
   if (current_thread->not_holding_lock)
     {
@@ -659,7 +666,7 @@ mark_threads (void)
 void
 unmark_main_thread (void)
 {
-  main_thread.header.size &= ~ARRAY_MARK_FLAG;
+  main_thread.s.header.size &= ~ARRAY_MARK_FLAG;
 }
 
 
@@ -1043,23 +1050,23 @@ thread_check_current_buffer (struct buffer *buffer)
 static void
 init_main_thread (void)
 {
-  main_thread.header.size
+  main_thread.s.header.size
     = PSEUDOVECSIZE (struct thread_state, m_stack_bottom);
-  XSETPVECTYPE (&main_thread, PVEC_THREAD);
-  main_thread.m_last_thing_searched = Qnil;
-  main_thread.m_saved_last_thing_searched = Qnil;
-  main_thread.name = Qnil;
-  main_thread.function = Qnil;
-  main_thread.result = Qnil;
-  main_thread.error_symbol = Qnil;
-  main_thread.error_data = Qnil;
-  main_thread.event_object = Qnil;
+  XSETPVECTYPE (&main_thread.s, PVEC_THREAD);
+  main_thread.s.m_last_thing_searched = Qnil;
+  main_thread.s.m_saved_last_thing_searched = Qnil;
+  main_thread.s.name = Qnil;
+  main_thread.s.function = Qnil;
+  main_thread.s.result = Qnil;
+  main_thread.s.error_symbol = Qnil;
+  main_thread.s.error_data = Qnil;
+  main_thread.s.event_object = Qnil;
 }
 
 bool
 main_thread_p (void *ptr)
 {
-  return ptr == &main_thread;
+  return ptr == &main_thread.s;
 }
 
 bool
@@ -1080,11 +1087,11 @@ void
 init_threads (void)
 {
   init_main_thread ();
-  sys_cond_init (&main_thread.thread_condvar);
+  sys_cond_init (&main_thread.s.thread_condvar);
   sys_mutex_init (&global_lock);
   sys_mutex_lock (&global_lock);
-  current_thread = &main_thread;
-  main_thread.thread_id = sys_thread_self ();
+  current_thread = &main_thread.s;
+  main_thread.s.thread_id = sys_thread_self ();
 }
 
 void
@@ -1130,7 +1137,7 @@ syms_of_threads (void)
   DEFVAR_LISP ("main-thread", Vmain_thread,
     doc: /* The main thread of Emacs.  */);
 #ifdef THREADS_ENABLED
-  XSETTHREAD (Vmain_thread, &main_thread);
+  XSETTHREAD (Vmain_thread, &main_thread.s);
 #else
   Vmain_thread = Qnil;
 #endif
diff --git a/src/xmenu.c b/src/xmenu.c
index 10e882a..31034f7 100644
--- a/src/xmenu.c
+++ b/src/xmenu.c
@@ -2420,6 +2420,6 @@ syms_of_xmenu (void)
 #if defined (USE_GTK) || defined (USE_X_TOOLKIT)
   defsubr (&Sx_menu_bar_open_internal);
   Ffset (intern_c_string ("accelerate-menu"),
-        intern_c_string (Sx_menu_bar_open_internal.symbol_name));
+        intern_c_string (Sx_menu_bar_open_internal.s.symbol_name));
 #endif
 }