bug#35711: emacs crashes on m68k after d2f1971dd5

bug#35711: emacs crashes on m68k after d2f1971dd5

[John Paul Adrian Glaubitz]

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

Hello!

The commit [1] "Port flexible array members to GCC + valgrind" (d2f1971dd5)
broke emacs on m68k. This is because the new code aligns blocks of 1-byte
strings to 16 bit on m68k which is the default alignment on m68k.

The crash on m68k looks like this [2]:

/bin/mkdir -p ../etc
/usr/bin/make -C ../lisp update-subdirs
make[4]: Entering directory '/<<BUILDDIR>>/emacs-26.1+1/debian/build-gtk/lisp'
make[4]: Leaving directory '/<<BUILDDIR>>/emacs-26.1+1/debian/build-gtk/lisp'
./temacs --batch  --load loadup bootstrap
Loading loadup.el (source)...
Using load-path (/<<BUILDDIR>>/emacs-26.1+1/debian/build-src/lisp /<<BUILDDIR>>/emacs-26.1+1/debian/build-src/lisp/emacs-lisp /<<BUILDDIR>>/emacs-26.1+1/debian/build-src/lisp/progmodes /<<BUILDDIR>>/emacs-26.1+1/debian/build-src/lisp/language /<<BUILDDIR>>/emacs-26.1+1/debian/build-src/lisp/international /<<BUILDDIR>>/emacs-26.1+1/debian/build-src/lisp/textmodes /<<BUILDDIR>>/emacs-26.1+1/debian/build-src/lisp/vc)
Loading emacs-lisp/byte-run (source)...
Loading emacs-lisp/backquote (source)...
Loading subr (source)...
qemu: uncaught target signal 11 (Segmentation fault) - core dumped
make[3]: *** [Makefile:738: bootstrap-emacs] Segmentation fault

The attached patch by Michael Karcher fixes the problem.

Thanks,
Adrian

> [1] http://git.savannah.gnu.org/cgit/emacs.git/commit/?id=d2f1971dd570439da4198fa76603b53b072060f8
> [2] https://buildd.debian.org/status/fetch.php?pkg=emacs&arch=m68k&ver=1%3A26.1%2B1-3.2&stamp=1549253883&raw=0

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaubitz@debian.org
`. `'   Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

[-- Attachment #2: fix-m68k-crash.patch --]
[-- Type: text/x-patch, Size: 1065 bytes --]

--- emacs-26.1+1.orig/src/alloc.c
+++ emacs-26.1+1/src/alloc.c
@@ -1766,14 +1766,14 @@ static char const string_overrun_cookie[
 
 /* The 'max' reserves space for the nbytes union member even when NBYTES + 1 is
    less than the size of that member.  The 'max' is not needed when
-   SDATA_DATA_OFFSET is a multiple of FLEXALIGNOF (struct sdata),
-   because then the alignment code reserves enough space.  */
+   the tail padding of "struct sdata" with a single payload byte is big enough
+   to accomodate the full union "sdata". */
 
 #define SDATA_SIZE(NBYTES)				      \
      ((SDATA_DATA_OFFSET				      \
-       + (SDATA_DATA_OFFSET % FLEXALIGNOF (struct sdata) == 0 \
-	  ? NBYTES					      \
-	  : max (NBYTES, FLEXALIGNOF (struct sdata) - 1))     \
+       + (FLEXSIZEOF (struct sdata, data, 1) < sizeof (sdata) \
+	  ? max (NBYTES, sizeof (sdata) - SDATA_DATA_OFFSET - 1) \
+	  : NBYTES)                                           \
        + 1						      \
        + FLEXALIGNOF (struct sdata) - 1)		      \
       & ~(FLEXALIGNOF (struct sdata) - 1))

bug#35711: emacs crashes on m68k after d2f1971dd5

[Paul Eggert]

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

Thanks for the bug report. Please try the attached, more-general patch,
which I've installed on master. I suppose I should backport it to the
emacs-26 branch if it works for you.

[-- Attachment #2: 0001-Fix-broken-build-on-m68k.patch --]
[-- Type: text/x-patch, Size: 5634 bytes --]

From 5190acc14f58d5f96887b1d9b6981aa1c48ced6f Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Mon, 13 May 2019 12:43:13 -0700
Subject: [PATCH] Fix broken build on m68k

The GCC + valgrind fix caused the m68k build to fail (Bug#35711).
Simplify string allocation a bit to make similar problems less
likely in the future.
* src/alloc.c (sdata, SDATA_NBYTES, SDATA_DATA) [GC_CHECK_STRING_BYTES]:
Use the same implementation as with !GC_CHECK_STRING_BYTES,
as the special case is no longer needed.
(SDATA_ALIGN): New constant.
(SDATA_SIZE): Remove this macro, replacing with ...
(sdata_size): ... this new function.  All uses changed.
Properly account for sizes and alignments even in the m68k case,
and even if GC_CHECK_STRING_BYTES is not defined.
---
 src/alloc.c | 77 +++++++++++++++++------------------------------------
 1 file changed, 25 insertions(+), 52 deletions(-)

diff --git a/src/alloc.c b/src/alloc.c
index 948a0e8a2d..af4adb3856 100644
--- a/src/alloc.c
+++ b/src/alloc.c
@@ -1447,9 +1447,7 @@ mark_interval_tree (INTERVAL i)
 
 #define LARGE_STRING_BYTES 1024
 
-/* The SDATA typedef is a struct or union describing string memory
-   sub-allocated from an sblock.  This is where the contents of Lisp
-   strings are stored.  */
+/* The layout of a nonnull string.  */
 
 struct sdata
 {
@@ -1468,13 +1466,8 @@ struct sdata
   unsigned char data[FLEXIBLE_ARRAY_MEMBER];
 };
 
-#ifdef GC_CHECK_STRING_BYTES
-
-typedef struct sdata sdata;
-#define SDATA_NBYTES(S)	(S)->nbytes
-#define SDATA_DATA(S)	(S)->data
-
-#else
+/* A union describing string memory sub-allocated from an sblock.
+   This is where the contents of Lisp strings are stored.  */
 
 typedef union
 {
@@ -1502,8 +1495,6 @@ typedef union
 #define SDATA_NBYTES(S)	(S)->n.nbytes
 #define SDATA_DATA(S)	((struct sdata *) (S))->data
 
-#endif /* not GC_CHECK_STRING_BYTES */
-
 enum { SDATA_DATA_OFFSET = offsetof (struct sdata, data) };
 
 /* Structure describing a block of memory which is sub-allocated to
@@ -1586,31 +1577,20 @@ static char const string_overrun_cookie[GC_STRING_OVERRUN_COOKIE_SIZE] =
 # define GC_STRING_OVERRUN_COOKIE_SIZE 0
 #endif
 
-/* Value is the size of an sdata structure large enough to hold NBYTES
-   bytes of string data.  The value returned includes a terminating
-   NUL byte, the size of the sdata structure, and padding.  */
-
-#ifdef GC_CHECK_STRING_BYTES
-
-#define SDATA_SIZE(NBYTES) FLEXSIZEOF (struct sdata, data, (NBYTES) + 1)
+/* Return the size of an sdata structure large enough to hold N bytes
+   of string data.  This counts the sdata structure, the N bytes, a
+   terminating NUL byte, and alignment padding.  */
 
-#else /* not GC_CHECK_STRING_BYTES */
-
-/* The 'max' reserves space for the nbytes union member even when NBYTES + 1 is
-   less than the size of that member.  The 'max' is not needed when
-   SDATA_DATA_OFFSET is a multiple of FLEXALIGNOF (struct sdata),
-   because then the alignment code reserves enough space.  */
-
-#define SDATA_SIZE(NBYTES)				      \
-     ((SDATA_DATA_OFFSET				      \
-       + (SDATA_DATA_OFFSET % FLEXALIGNOF (struct sdata) == 0 \
-	  ? NBYTES					      \
-	  : max (NBYTES, FLEXALIGNOF (struct sdata) - 1))     \
-       + 1						      \
-       + FLEXALIGNOF (struct sdata) - 1)		      \
-      & ~(FLEXALIGNOF (struct sdata) - 1))
-
-#endif /* not GC_CHECK_STRING_BYTES */
+static ptrdiff_t
+sdata_size (ptrdiff_t n)
+{
+  /* Reserve space for the nbytes union member even when N + 1 is less
+     than the size of that member.  */
+  ptrdiff_t unaligned_size = max (SDATA_DATA_OFFSET + n + 1,
+				  sizeof (sdata));
+  int sdata_align = max (FLEXALIGNOF (struct sdata), alignof (sdata));
+  return (unaligned_size + sdata_align - 1) & ~(sdata_align - 1);
+}
 
 /* Extra bytes to allocate for each string.  */
 #define GC_STRING_EXTRA GC_STRING_OVERRUN_COOKIE_SIZE
@@ -1664,21 +1644,14 @@ string_bytes (struct Lisp_String *s)
 static void
 check_sblock (struct sblock *b)
 {
-  sdata *from, *end, *from_end;
-
-  end = b->next_free;
+  sdata *end = b->next_free;
 
-  for (from = b->data; from < end; from = from_end)
+  for (sdata *from = b->data; from < end; )
     {
-      /* Compute the next FROM here because copying below may
-	 overwrite data we need to compute it.  */
-      ptrdiff_t nbytes;
-
-      /* Check that the string size recorded in the string is the
-	 same as the one recorded in the sdata structure.  */
-      nbytes = SDATA_SIZE (from->string ? string_bytes (from->string)
-			   : SDATA_NBYTES (from));
-      from_end = (sdata *) ((char *) from + nbytes + GC_STRING_EXTRA);
+      ptrdiff_t nbytes = sdata_size (from->string
+				     ? string_bytes (from->string)
+				     : SDATA_NBYTES (from));
+      from = (sdata *) ((char *) from + nbytes + GC_STRING_EXTRA);
     }
 }
 
@@ -1810,14 +1783,14 @@ allocate_string_data (struct Lisp_String *s,
 {
   sdata *data, *old_data;
   struct sblock *b;
-  ptrdiff_t needed, old_nbytes;
+  ptrdiff_t old_nbytes;
 
   if (STRING_BYTES_MAX < nbytes)
     string_overflow ();
 
   /* Determine the number of bytes needed to store NBYTES bytes
      of string data.  */
-  needed = SDATA_SIZE (nbytes);
+  ptrdiff_t needed = sdata_size (nbytes);
   if (s->u.s.data)
     {
       old_data = SDATA_OF_STRING (s);
@@ -2068,7 +2041,7 @@ compact_small_strings (void)
 	      nbytes = s ? STRING_BYTES (s) : SDATA_NBYTES (from);
 	      eassert (nbytes <= LARGE_STRING_BYTES);
 
-	      ptrdiff_t size = SDATA_SIZE (nbytes);
+	      ptrdiff_t size = sdata_size (nbytes);
 	      sdata *from_end = (sdata *) ((char *) from
 					   + size + GC_STRING_EXTRA);
 
-- 
2.21.0

bug#35711: emacs crashes on m68k after d2f1971dd5

[John Paul Adrian Glaubitz]

Hi Paul!

On 5/13/19 9:47 PM, Paul Eggert wrote:
> Thanks for the bug report. Please try the attached, more-general patch,
> which I've installed on master. I suppose I should backport it to the
> emacs-26 branch if it works for you.

I just checked out emacs from git and started a test build on m68k. The
original crahs no longer shows, but I'm running into this problem now
which I don't know whether it's related:

Finding pointers to doc strings...
Finding pointers to doc strings...done
Dumping under the name bootstrap-emacs.pdmp
dumping fingerprint: 3cb05906acd421bf706fa119445377d793ee7ffbbde4c79b70ca65fdb4bb625e
dump relocation out of range
make[1]: *** [Makefile:808: bootstrap-emacs.pdmp] Error 255
make[1]: Leaving directory '/usr/src/emacs/src'
make: *** [Makefile:424: src] Error 2

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaubitz@debian.org
`. `'   Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

bug#35711: emacs crashes on m68k after d2f1971dd5

[Paul Eggert]

John Paul Adrian Glaubitz wrote:
> Dumping under the name bootstrap-emacs.pdmp
> dumping fingerprint: 3cb05906acd421bf706fa119445377d793ee7ffbbde4c79b70ca65fdb4bb625e
> dump relocation out of range

I'd wildly guess it's a different problem. Try configuring Emacs for debugging 
(see etc/DEBUG) and then see if you can get a C backtrace just before it outputs 
"dump relocation out of range".

bug#35711: emacs crashes on m68k after d2f1971dd5

[John Paul Adrian Glaubitz]

Hi Paul!

On 5/14/19 8:04 AM, Paul Eggert wrote:
> John Paul Adrian Glaubitz wrote:
>> Dumping under the name bootstrap-emacs.pdmp
>> dumping fingerprint: 3cb05906acd421bf706fa119445377d793ee7ffbbde4c79b70ca65fdb4bb625e
>> dump relocation out of range
> 
> I'd wildly guess it's a different problem. Try configuring Emacs for debugging
> (see etc/DEBUG) and then see if you can get a C backtrace just before it
> outputs "dump relocation out of range".

You seem to be correct. I just tried Michael's patch instead of yours and
emacs-git still crashes for me. On the other hand, backporting your current
patch to the 26 branch fixes the bug for me for emacs26.

Thus, could you backport your patch to the 26 branch?

Thanks,
Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaubitz@debian.org
`. `'   Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

bug#35711: emacs crashes on m68k after d2f1971dd5

[Paul Eggert]

On 5/14/19 2:10 AM, John Paul Adrian Glaubitz wrote:
> backporting your current
> patch to the 26 branch fixes the bug for me for emacs26.
>
> Thus, could you backport your patch to the 26 branch?

Thanks for checking. I backported it and am closing the bug report.