[master b3cf281] Unbreak the MinGW build

[master b3cf281] Unbreak the MinGW build

[Eli Zaretskii]

Paul,

I needed this commit to prevent temacs from crashing during dumping.
Don't ask me how including errno.h (both the one from Gnulib and the
MinGW one) could cause this, especially as the preprocessed __fpending
doesn't seem to change a bit as result of that, and it doesn't seem to
even be called during dumping.  The facts are stubborn: if I leave
that inclusion in place, I get a crash, removing it fixes the crash.

Any ideas?

Re: [master b3cf281] Unbreak the MinGW build

[Paul Eggert]

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

Eli Zaretskii wrote:

> I needed this commit to prevent temacs from crashing during dumping.
> Don't ask me how including errno.h (both the one from Gnulib and the
> MinGW one) could cause this, especially as the preprocessed __fpending
> doesn't seem to change a bit as result of that, and it doesn't seem to
> even be called during dumping.  The facts are stubborn: if I leave
> that inclusion in place, I get a crash, removing it fixes the crash.

I propagated this fix back to gnulib by installing the attached patch to Gnulib. 
I'll CC: this to Bruno Haible, who made the recent change to fpending. I don't 
use MinGW myself.

[-- Attachment #2: 0001-fpending-fix-port-to-MinGW-on-Emacs.patch --]
[-- Type: text/x-diff, Size: 1458 bytes --]

From 60635cc80e74f7f48cf5f794cc1d3d43d4c4f1e9 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Fri, 16 Dec 2016 07:38:58 -0800
Subject: [PATCH] fpending: fix port to MinGW on Emacs

* lib/stdio-impl.h [__MINGW32__]: Do not include errno.h.
Problem reported by Eli Zaretskii in:
http://lists.gnu.org/archive/html/emacs-devel/2016-12/msg00642.html
Is Plan 9 still a valid porting target, anyway?
---
 ChangeLog        | 8 ++++++++
 lib/stdio-impl.h | 2 ++
 2 files changed, 10 insertions(+)

diff --git a/ChangeLog b/ChangeLog
index 5e9e801..dd67dba 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,11 @@
+2016-12-16  Paul Eggert  <eggert@cs.ucla.edu>
+
+	fpending: fix port to MinGW on Emacs
+	* lib/stdio-impl.h [__MINGW32__]: Do not include errno.h.
+	Problem reported by Eli Zaretskii in:
+	http://lists.gnu.org/archive/html/emacs-devel/2016-12/msg00642.html
+	Is Plan 9 still a valid porting target, anyway?
+
 2016-12-15  Paul Eggert  <eggert@cs.ucla.edu>
 
 	safe-alloc: use xalloc-oversized
diff --git a/lib/stdio-impl.h b/lib/stdio-impl.h
index 766d693..1972a33 100644
--- a/lib/stdio-impl.h
+++ b/lib/stdio-impl.h
@@ -26,7 +26,9 @@
 # include <sys/param.h>
 #endif
 
+#ifndef __MINGW32__
 #include <errno.h>                             /* For detecting Plan9.  */
+#endif
 
 #if defined __sferror || defined __DragonFly__ || defined __ANDROID__
   /* FreeBSD, NetBSD, OpenBSD, DragonFly, Mac OS X, Cygwin, Android */
-- 
2.7.4

Re: [master b3cf281] Unbreak the MinGW build

[Bruno Haible]

Eli Zaretskii wrote:
> > I needed this commit to prevent temacs from crashing during dumping.
> > Don't ask me how including errno.h (both the one from Gnulib and the
> > MinGW one) could cause this, especially as the preprocessed __fpending
> > doesn't seem to change a bit as result of that, and it doesn't seem to
> > even be called during dumping.  The facts are stubborn: if I leave
> > that inclusion in place, I get a crash, removing it fixes the crash.

Paul Eggert replied:
> I propagated this fix back to gnulib by installing the attached patch to Gnulib. 
> I'll CC: this to Bruno Haible, who made the recent change to fpending. I don't 
> use MinGW myself.

On mingw, the fpending.o generated by this code, with or without this
#include <errno.h>, is identical (even with debugging information [-g]).

I wouldn't have applied this patch, as the cause of the crash is obviously
somewhere else. We have all seen Heisenbugs in complex pieces of code. The
suspicious complex piece of code in this case, for me, is the temacs
dumping code.

Eli:
- How often have you tried to temacs+dump with and without the change?
  Once each? Twice each? 10 times each? If it's a small number, you may
  be seeing a random result and not realize it was random.
- Did you run temacs in the same directory in both cases, or in different
  directories? Different directories could lead to a different memory
  layout in temacs, due to different filename lengths.
- After determining that disabling the include would fix the crash, did
  you test whether reenable the include would reenable the crash? If not,
  some leftover file on the file system could make the difference.

Bruno

Re: [master b3cf281] Unbreak the MinGW build

[Eli Zaretskii]

> From: Bruno Haible <bruno@clisp.org>
> Cc: bug-gnulib@gnu.org, Paul Eggert <eggert@cs.ucla.edu>, emacs-devel@gnu.org
> Date: Fri, 16 Dec 2016 19:11:31 +0100
> 
> On mingw, the fpending.o generated by this code, with or without this
> #include <errno.h>, is identical (even with debugging information [-g]).

Yes, I said that much.  Which is why this is a mystery in my eyes.

> I wouldn't have applied this patch, as the cause of the crash is obviously
> somewhere else.

I know.  I just don't know where, and have run out of ideas.
Suggestions welcome, but I cannot continue my work on Emacs without
being able to build the latest HEAD of the master branch.  Having
uncommitted/unpushed changes in master means I need to jump through
hoops each time I need to push a change upstream.

> - How often have you tried to temacs+dump with and without the change?
>   Once each? Twice each? 10 times each? If it's a small number, you may
>   be seeing a random result and not realize it was random.

I did it in two separate branches, 3 times in each one.  The results
are consistent.  This crash isn't random.

> - Did you run temacs in the same directory in both cases, or in different
>   directories? Different directories could lead to a different memory
>   layout in temacs, due to different filename lengths.

Same directory.

> - After determining that disabling the include would fix the crash, did
>   you test whether reenable the include would reenable the crash?

Yes.

Re: [master b3cf281] Unbreak the MinGW build

[Stephen Leake]

Eli Zaretskii <eliz@gnu.org> writes:

> I needed this commit to prevent temacs from crashing during dumping.
> Don't ask me how including errno.h (both the one from Gnulib and the
> MinGW one) could cause this, especially as the preprocessed __fpending
> doesn't seem to change a bit as result of that, and it doesn't seem to
> even be called during dumping.  The facts are stubborn: if I leave
> that inclusion in place, I get a crash, removing it fixes the crash.

To help with this, I'm trying to build master on Mingw64, but I'm
missing some threads library (I assume due to the recent addition of
concurrency support):

gcc.exe: error: thread.o: No such file or directory
gcc.exe: error: systhread.o: No such file or directory


mingw has several "threads" libraries;


mingw64/mingw-w64-x86_64-npth 1.2-2
    New portable threads library (mingw-w64)
mingw64/mingw-w64-x86_64-port-scanner 1.3-2
    A multi threaded TCP port scanner from SecPoint.com (mingw-w64)
mingw64/mingw-w64-x86_64-winpthreads-git 5.0.0.4573.628fdbf-1 (mingw-w64-x86_64-toolchain) [installed]
    MinGW-w64 winpthreads library

which one should I use?

--
-- Stephe

Re: [master b3cf281] Unbreak the MinGW build

[Stephen Leake]

Never mind, 'make bootstrap' fixed it.

Stephen Leake <stephen_leake@stephe-leake.org> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>> I needed this commit to prevent temacs from crashing during dumping.
>> Don't ask me how including errno.h (both the one from Gnulib and the
>> MinGW one) could cause this, especially as the preprocessed __fpending
>> doesn't seem to change a bit as result of that, and it doesn't seem to
>> even be called during dumping.  The facts are stubborn: if I leave
>> that inclusion in place, I get a crash, removing it fixes the crash.
>
> To help with this, I'm trying to build master on Mingw64, but I'm
> missing some threads library (I assume due to the recent addition of
> concurrency support):
>
> gcc.exe: error: thread.o: No such file or directory
> gcc.exe: error: systhread.o: No such file or directory
>
>
> mingw has several "threads" libraries;
>
>
> mingw64/mingw-w64-x86_64-npth 1.2-2
>     New portable threads library (mingw-w64)
> mingw64/mingw-w64-x86_64-port-scanner 1.3-2
>     A multi threaded TCP port scanner from SecPoint.com (mingw-w64)
> mingw64/mingw-w64-x86_64-winpthreads-git 5.0.0.4573.628fdbf-1
> (mingw-w64-x86_64-toolchain) [installed]
>     MinGW-w64 winpthreads library
>
> which one should I use?
>
> --
> -- Stephe

-- 
-- Stephe

Re: [master b3cf281] Unbreak the MinGW build

[Bruno Haible]

Eli Zaretskii wrote:
> > - How often have you tried to temacs+dump with and without the change?
> >   Once each? Twice each? 10 times each? If it's a small number, you may
> >   be seeing a random result and not realize it was random.
> 
> I did it in two separate branches, 3 times in each one.  The results
> are consistent.  This crash isn't random.
> 
> > - Did you run temacs in the same directory in both cases, or in different
> >   directories? Different directories could lead to a different memory
> >   layout in temacs, due to different filename lengths.
> 
> Same directory.
> 
> > - After determining that disabling the include would fix the crash, did
> >   you test whether reenable the include would reenable the crash?
> 
> Yes.

OK, that much for our "conventional" wisdom...

Since, as you say, the crash occurs during dumping, this is what I would
turn to now. Do you have a systematic approach for debugging crashes during
dump? If dump is based on malloc, does it help to set MALLOC_PERTURB_?
Can you use tools such as valgrind to debug it?

> Having
> uncommitted/unpushed changes in master means I need to jump through
> hoops each time I need to push a change upstream.

Yes, having to do
  $ git rebase -i HEAD~2
  $ git push origin HEAD~1:master
each time is extra work.

> > I wouldn't have applied this patch, as the cause of the crash is obviously
> > somewhere else.
> 
> I know.

Actually gnulib has a way to keep in your project changes relative to gnulib
that should not be pushed upstream: It's gnulib-tool's --local-dir option.
You would have had to create a small lib/stdio-impl.h.diff file that gets
applied on the fly each time you invoke gnulib-tool for emacs.

Bruno

Re: [master b3cf281] Unbreak the MinGW build

[Paul Eggert]

Bruno Haible wrote:
> gnulib has a way to keep in your project changes relative to gnulib
> that should not be pushed upstream

I'd rather not do that, as the Emacs build procedure is already too complicated.

Re: [master b3cf281] Unbreak the MinGW build

[Eli Zaretskii]

> From: Stephen Leake <stephen_leake@stephe-leake.org>
> Cc: Paul Eggert <eggert@cs.ucla.edu>,  emacs-devel@gnu.org
> Date: Fri, 16 Dec 2016 16:16:44 -0600
> 
> mingw has several "threads" libraries;
> 
> 
> mingw64/mingw-w64-x86_64-npth 1.2-2
>     New portable threads library (mingw-w64)
> mingw64/mingw-w64-x86_64-port-scanner 1.3-2
>     A multi threaded TCP port scanner from SecPoint.com (mingw-w64)
> mingw64/mingw-w64-x86_64-winpthreads-git 5.0.0.4573.628fdbf-1 (mingw-w64-x86_64-toolchain) [installed]
>     MinGW-w64 winpthreads library
> 
> which one should I use?

None.  On Windows we use the native OS threads, so no auxiliary
libraries are necessary.

Re: [master b3cf281] Unbreak the MinGW build

[Eli Zaretskii]

> From: Bruno Haible <bruno@clisp.org>
> Cc: bug-gnulib@gnu.org
> Date: Sat, 17 Dec 2016 00:17:31 +0100
> 
> Since, as you say, the crash occurs during dumping, this is what I would
> turn to now. Do you have a systematic approach for debugging crashes during
> dump? If dump is based on malloc, does it help to set MALLOC_PERTURB_?
> Can you use tools such as valgrind to debug it?

Why would I need them?  The dumping code on Windows is just C code
that reads and writes a binary file, so GDB is good enough, it shows
exactly what causes the crash.  Except that what it told me made very
little sense, so I tried the "blame the last change" approach.  Which
worked, even though examining the source-level changes clearly
indicates that the new fpending.c does the same as the old one did, as
long as you compare the executable C code in the two versions.

> Actually gnulib has a way to keep in your project changes relative to gnulib
> that should not be pushed upstream: It's gnulib-tool's --local-dir option.
> You would have had to create a small lib/stdio-impl.h.diff file that gets
> applied on the fly each time you invoke gnulib-tool for emacs.

I didn't ask to propagate the change back into Gnulib, that was Paul's
decision.  I understand his motivation, but I'm okay with anything you
decide about Gnulib.

I will try to look into this some more today, and see if I can unlock
the mystery.  It bothers me, too.

Thanks.

Re: [master b3cf281] Unbreak the MinGW build

[Eli Zaretskii]

> Date: Sat, 17 Dec 2016 09:51:15 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: bug-gnulib@gnu.org, emacs-devel@gnu.org
> 
> I will try to look into this some more today, and see if I can unlock
> the mystery.  It bothers me, too.

Bug squashed, see commit 0757b4f.  It was always there, we were just
lucky until now.

What the inclusion of errno.h did is increase the size of the object
file (due to massive debug info about macros), and thus the size of
temacs, by 1.5KB, and that caused writes to emacs.exe to cross the
page boundary beyond the allocated memory mapping, and segfault.

You can now remove the workaround from stdio-impl.h.  (I already did
that in the Emacs repository.)

Thanks.