Subject: Re: bug#29040: Emacs 25 hangs on windows arbitrarily during search of a unicode file

From: Paul Eggert <[email-masked]>
Date: Thu, 2 Nov 2017 22:03:17 -0700

Eli Zaretskii wrote:
> Thanks, but I think the resulting code is confusing.  E.g.:
> 
>    static struct thread_state GCALIGNED main_thread;
> 
> This looks like we ask main_thread to be 8-byte aligned, which would
> bring back the problems we had before.

I attempted to clarify this by installing the first attached patch into the 
emacs-26 branch. This patch adds a comment to the definition of GCALIGNED. If 
the name "GCALIGNED" itself is confusing, we can systematically change the name 
to something else.  (The name doesn't confuse me, but perhaps I'm too close to 
the code.)

> I don't know anything about that code, and I don't think anyone does.
> Who will tell us that the comment is still accurate?

Since MSVC is no longer relevant, it doesn't matter whether the comment about 
MSVC is accurate, and we can remove it.

> Anything that is conditioned by _MSC_VER can go.

I did that by installing the second attached patch into 'master'. This affects 
only the mainline code; I didn't fiddle with the MS-Windows-only files since I 
can't easily test them.
From a87ce5c4b41ca8146c764b1186a6162c069b1933 Mon Sep 17 00:00:00 2001
From: Paul Eggert <[email-masked]>
Date: Thu, 2 Nov 2017 20:04:25 -0700
Subject: [PATCH] * src/lisp.h (GCALIGNED): Clarify comment (Bug#29040).

---
 src/lisp.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/lisp.h b/src/lisp.h
index bf9db59..a71ba22 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -272,8 +272,9 @@ error !;
 #endif
 
 /* Declare an object to have an address that is a multiple of
-   GCALIGNMENT.  alignas is not suitable here, as it fails if the
-   object's natural alignment exceeds GCALIGNMENT.  */
+   GCALIGNMENT.  This is a no-op if the object's natural alignment is
+   already a multiple of GCALIGNMENT.  alignas is not suitable here,
+   as it fails if the object's natural alignment exceeds GCALIGNMENT.  */
 #ifdef HAVE_STRUCT_ATTRIBUTE_ALIGNED
 # define GCALIGNED __attribute__ ((aligned (GCALIGNMENT)))
 #else
-- 
2.7.4

From 5b4ff53ab4579532b1b9eba233c135fee274d674 Mon Sep 17 00:00:00 2001
From: Paul Eggert <[email-masked]>
Date: Thu, 2 Nov 2017 21:54:23 -0700
Subject: [PATCH] Simplify by assuming !_MSC_VER.

Emacs has not been portable to MSVC for some time (Bug#29040#57).
* admin/CPP-DEFINES (_MSC_VER): Remove.
* src/lisp.h (ENUM_BF, DEFUN):
* src/regex.c (re_char):
Simplify by assuming _MSC_VER is not defined.
* src/regex.c (const_re_char): Remove.
All uses replaced by re_char.
---
 admin/CPP-DEFINES |  1 -
 src/lastfile.c    |  3 ---
 src/lisp.h        | 16 ++--------------
 src/regex.c       | 32 +++++++++++++-------------------
 4 files changed, 15 insertions(+), 37 deletions(-)

diff --git a/admin/CPP-DEFINES b/admin/CPP-DEFINES
index 10b558d..eb3eadf 100644
--- a/admin/CPP-DEFINES
+++ b/admin/CPP-DEFINES
@@ -19,7 +19,6 @@ __DJGPP_MINOR__ Minor version number of the DJGPP library; used only in msdos.c
 DOS_NT		Compiling for either the MS-DOS or native MS-Windows port.
 WINDOWSNT	Compiling the native MS-Windows (W32) port.
 __MINGW32__	Compiling the W32 port with the MinGW or MinGW-w64 ports of GCC.
-_MSC_VER	Compiling the W32 port with the Microsoft C compiler.
 MINGW_W64	Compiling the W32 port with the MinGW-w64 port of GCC.
 DARWIN_OS	Compiling on macOS or pure Darwin (and using s/darwin.h).
 SOLARIS2
diff --git a/src/lastfile.c b/src/lastfile.c
index 2901f14..1302279 100644
--- a/src/lastfile.c
+++ b/src/lastfile.c
@@ -49,9 +49,6 @@ char my_edata[] = "End of Emacs initialized data";
    isn't always a separate section in NT executables).  */
 char my_endbss[1];
 
-/* The Alpha MSVC linker globally segregates all static and public bss
-   data, so we must take both into account to determine the true extent
-   of the bss area used by Emacs.  */
 static char _my_endbss[1];
 char * my_endbss_static = _my_endbss;
 
diff --git a/src/lisp.h b/src/lisp.h
index a71ba22..1a8c3a0 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -418,9 +418,8 @@ error !;
 #define case_Lisp_Int case Lisp_Int0: case Lisp_Int1
 
 /* Idea stolen from GDB.  Pedantic GCC complains about enum bitfields,
-   MSVC doesn't support them, and xlc and Oracle Studio c99 complain
-   vociferously about them.  */
-#if (defined __STRICT_ANSI__ || defined _MSC_VER || defined __IBMC__ \
+   and xlc and Oracle Studio c99 complain vociferously about them.  */
+#if (defined __STRICT_ANSI__ || defined __IBMC__ \
      || (defined __SUNPRO_C && __STDC__))
 #define ENUM_BF(TYPE) unsigned int
 #else
@@ -2935,23 +2934,12 @@ CHECK_NUMBER_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.  */
-#ifdef _MSC_VER
-#define DEFUN(lname, fnname, sname, minargs, maxargs, intspec, doc)	\
-   Lisp_Object fnname DEFUN_ARGS_ ## maxargs ;				\
-   static struct Lisp_Subr GCALIGNED sname =				\
-   { { (PVEC_SUBR << PSEUDOVECTOR_AREA_BITS)				\
-       | (sizeof (struct Lisp_Subr) / sizeof (EMACS_INT)) },		\
-      { (Lisp_Object (__cdecl *)(void))fnname },                        \
-       minargs, maxargs, lname, intspec, 0};				\
-   Lisp_Object fnname
-#else  /* not _MSC_VER */
 #define DEFUN(lname, fnname, sname, minargs, maxargs, intspec, doc)	\
    static struct Lisp_Subr GCALIGNED sname =				\
      { { PVEC_SUBR << PSEUDOVECTOR_AREA_BITS },				\
        { .a ## maxargs = fnname },					\
        minargs, maxargs, lname, intspec, 0};				\
    Lisp_Object fnname
-#endif
 
 /* defsubr (Sname);
    is how we define the symbol for function `name' at start-up time.  */
diff --git a/src/regex.c b/src/regex.c
index 330f2f7..d3d910d 100644
--- a/src/regex.c
+++ b/src/regex.c
@@ -519,13 +519,7 @@ ptrdiff_t emacs_re_safe_alloca = MAX_ALLOCA;
 #endif
 
 /* Type of source-pattern and string chars.  */
-#ifdef _MSC_VER
-typedef unsigned char re_char;
-typedef const re_char const_re_char;
-#else
 typedef const unsigned char re_char;
-typedef re_char const_re_char;
-#endif
 
 typedef char boolean;
 
@@ -2403,7 +2397,7 @@ do {									\
   } while (0)
 
 static reg_errcode_t
-regex_compile (const_re_char *pattern, size_t size,
+regex_compile (re_char *pattern, size_t size,
 #ifdef emacs
 # define syntax RE_SYNTAX_EMACS
 	       bool posix_backtracking,
@@ -3728,7 +3722,7 @@ insert_op2 (re_opcode_t op, unsigned char *loc, int arg1, int arg2, unsigned cha
    least one character before the ^.  */
 
 static boolean
-at_begline_loc_p (const_re_char *pattern, const_re_char *p, reg_syntax_t syntax)
+at_begline_loc_p (re_char *pattern, re_char *p, reg_syntax_t syntax)
 {
   re_char *prev = p - 2;
   boolean odd_backslashes;
@@ -3769,7 +3763,7 @@ at_begline_loc_p (const_re_char *pattern, const_re_char *p, reg_syntax_t syntax)
    at least one character after the $, i.e., `P < PEND'.  */
 
 static boolean
-at_endline_loc_p (const_re_char *p, const_re_char *pend, reg_syntax_t syntax)
+at_endline_loc_p (re_char *p, re_char *pend, reg_syntax_t syntax)
 {
   re_char *next = p;
   boolean next_backslash = *next == '\\';
@@ -3813,7 +3807,7 @@ group_in_compile_stack (compile_stack_type compile_stack, regnum_t regnum)
    Return -1 if fastmap was not updated accurately.  */
 
 static int
-analyze_first (const_re_char *p, const_re_char *pend, char *fastmap,
+analyze_first (re_char *p, re_char *pend, char *fastmap,
 	       const int multibyte)
 {
   int j, k;
@@ -4555,7 +4549,7 @@ static int bcmp_translate (re_char *s1, re_char *s2,
 /* If the operation is a match against one or more chars,
    return a pointer to the next operation, else return NULL.  */
 static re_char *
-skip_one_char (const_re_char *p)
+skip_one_char (re_char *p)
 {
   switch (*p++)
     {
@@ -4597,7 +4591,7 @@ skip_one_char (const_re_char *p)
 
 /* Jump over non-matching operations.  */
 static re_char *
-skip_noops (const_re_char *p, const_re_char *pend)
+skip_noops (re_char *p, re_char *pend)
 {
   int mcnt;
   while (p < pend)
@@ -4628,7 +4622,7 @@ skip_noops (const_re_char *p, const_re_char *pend)
    character (i.e. without any translations).  UNIBYTE denotes whether c is
    unibyte or multibyte character. */
 static bool
-execute_charset (const_re_char **pp, unsigned c, unsigned corig, bool unibyte)
+execute_charset (re_char **pp, unsigned c, unsigned corig, bool unibyte)
 {
   re_char *p = *pp, *rtp = NULL;
   bool not = (re_opcode_t) *p == charset_not;
@@ -4692,8 +4686,8 @@ execute_charset (const_re_char **pp, unsigned c, unsigned corig, bool unibyte)
 
 /* Non-zero if "p1 matches something" implies "p2 fails".  */
 static int
-mutually_exclusive_p (struct re_pattern_buffer *bufp, const_re_char *p1,
-		      const_re_char *p2)
+mutually_exclusive_p (struct re_pattern_buffer *bufp, re_char *p1,
+		      re_char *p2)
 {
   re_opcode_t op2;
   const boolean multibyte = RE_MULTIBYTE_P (bufp);
@@ -4931,8 +4925,8 @@ WEAK_ALIAS (__re_match_2, re_match_2)
 /* This is a separate function so that we can force an alloca cleanup
    afterwards.  */
 static regoff_t
-re_match_2_internal (struct re_pattern_buffer *bufp, const_re_char *string1,
-		     size_t size1, const_re_char *string2, size_t size2,
+re_match_2_internal (struct re_pattern_buffer *bufp, re_char *string1,
+		     size_t size1, re_char *string2, size_t size2,
 		     ssize_t pos, struct re_registers *regs, ssize_t stop)
 {
   /* General temporaries.  */
@@ -6222,10 +6216,10 @@ re_match_2_internal (struct re_pattern_buffer *bufp, const_re_char *string1,
    bytes; nonzero otherwise.  */
 
 static int
-bcmp_translate (const_re_char *s1, const_re_char *s2, register ssize_t len,
+bcmp_translate (re_char *s1, re_char *s2, ssize_t len,
 		RE_TRANSLATE_TYPE translate, const int target_multibyte)
 {
-  register re_char *p1 = s1, *p2 = s2;
+  re_char *p1 = s1, *p2 = s2;
   re_char *p1_end = s1 + len;
   re_char *p2_end = s2 + len;
 
-- 
2.7.4