Compiling (development) Emacs with MSVC

Compiling (development) Emacs with MSVC

[Fabrice Popineau]

Hi guys,

Some of you may remember this thread :
http://thread.gmane.org/gmane.emacs.devel/97959
This issue has been solved, the ASLR feature
was not at stake and I have been using several
variants of emacs 22.x compiled with msvc and linked
to msvcrt.dll.

I'm digging out this thread because I have been beaten
by this very feature using the most recent msvc compiler
(Microsoft (R) 32-bit C/C++ Optimizing Compiler Version 16.00.30319.01)
together with their linker
(Microsoft (R) Incremental Linker Version 10.00.30319.01)
This time, the ASLR feature is turned on _by default_
and the dumped emacs is not able to run (various function
pointers off).
Hint: use the /dynamicbase:no option to the linker.

I have been able to compile the current emacs source
tree with msvc/mscvcrt.dll (debug mode for the moment).
It just works (almost) out of the box. I have been able
to bootstrap and run the whole thing.

Best regards,

Fabrice

Re: Compiling (development) Emacs with MSVC

[Eli Zaretskii]

> From: Fabrice Popineau <fabrice.popineau@supelec.fr>
> Date: Mon, 6 Dec 2010 21:21:49 +0000 (UTC)
> 
> I'm digging out this thread because I have been beaten
> by this very feature using the most recent msvc compiler
> (Microsoft (R) 32-bit C/C++ Optimizing Compiler Version 16.00.30319.01)
> together with their linker
> (Microsoft (R) Incremental Linker Version 10.00.30319.01)
> This time, the ASLR feature is turned on _by default_
> and the dumped emacs is not able to run (various function
> pointers off).
> Hint: use the /dynamicbase:no option to the linker.
> 
> I have been able to compile the current emacs source
> tree with msvc/mscvcrt.dll (debug mode for the moment).
> It just works (almost) out of the box. I have been able
> to bootstrap and run the whole thing.

Any patches that are worth adding to mainline?

TIA

Re: Compiling (development) Emacs with MSVC

[Fabrice Popineau]

[-- Attachment #1.1: Type: text/plain, Size: 1248 bytes --]

Patch attached.

I guess I can explain the reason for all of it, albeit the solution I have
taken may not be the easiest to maintain in the source tree.
Apparently the ANSI C spec says that bit fields are unsigned, and enum are
signed, hence the patch in lisp.h for various bit fields.
Anyway, the MS compiler did extend sign and that was the main failure.
Almost everything else is about linking with the right functions in msvcrt.

I have traced through the whole initialization process and I have not seen
any interference between gmalloc.c and the msvcrt allocator. In fact I
wonder if the gmalloc heap initialization has to occur that sooner. It seems
to me that malloc()/gmalloc.c is called for the first time when sorting
arguments from the command line, so quite late.

The "-dynamicbase:no" linker option is mandatory, else emacs is relocated at
some different address in memory, and that makes it crash, especially
because of some function pointers dumped into the exe. Hard to trace
exactly, because the crash is always with a corrupt stack, albeit always in
gmalloc.c or alloc.c. But the flag fixed the problem.

The portable dumper (once written for emacs 21 and avalaible in xemacs)
would be a very good idea.

Best regards,

Fabrice

[-- Attachment #1.2: Type: text/html, Size: 1443 bytes --]

[-- Attachment #2: emacs-msvc.diff --]
[-- Type: application/octet-stream, Size: 14837 bytes --]

diff -ruN ..\mirror\emacs-bzr\trunk\src/lisp.h emacs-gnu\src/lisp.h
--- ..\mirror\emacs-bzr\trunk\src/lisp.h	2010-11-18 08:53:04.000000000 +0100
+++ emacs-gnu\src/lisp.h	2010-12-13 21:50:58.000000000 +0100
@@ -124,8 +124,11 @@
 # ifndef DECL_ALIGN
 /* What compiler directive should we use for non-gcc compilers?  -stef  */
 #  if defined (__GNUC__)
-#   define DECL_ALIGN(type, var) \
+#   define DECL_ALIGN(type, var)                                \
      type __attribute__ ((__aligned__ (1 << GCTYPEBITS))) var
+#  elseif defined(_MSC_VER)
+#   define DECL_ALIGN(type, var) \
+     type __declspec(align(1 << GCTYPEBITS)) var
 #  endif
 # endif
 #endif
@@ -1007,7 +1010,11 @@
      2 : it's a localized var, the value is in the `blv' object.
      3 : it's a forwarding variable, the value is in `forward'.
    */
+#ifdef _MSC_VER
+  unsigned short redirect : 3;
+#else
   enum symbol_redirect redirect : 3;
+#endif

   /* Non-zero means symbol is constant, i.e. changing its value
      should signal an error.  If the value is 3, then the var
@@ -1216,7 +1223,11 @@

 struct Lisp_Misc_Any		/* Supertype of all Misc types.  */
 {
+#ifdef _MSC_VER
+  unsigned short type : 16;
+#else
   enum Lisp_Misc_Type type : 16;		/* = Lisp_Misc_??? */
+#endif
   unsigned gcmarkbit : 1;
   int spacer : 15;
   /* Make it as long as "Lisp_Free without padding". */
@@ -1225,7 +1236,11 @@

 struct Lisp_Marker
 {
+#ifdef _MSC_VER
+  unsigned short type : 16;
+#else
   enum Lisp_Misc_Type type : 16;		/* = Lisp_Misc_Marker */
+#endif
   unsigned gcmarkbit : 1;
   int spacer : 13;
   /* This flag is temporarily used in the functions
@@ -1375,7 +1390,11 @@
    I.e. 9words plus 2 bits, 3words of which are for external linked lists.
 */
   {
+#ifdef _MSC_VER
+    unsigned short type : 16;
+#else
     enum Lisp_Misc_Type type : 16;	/* = Lisp_Misc_Overlay */
+#endif
     unsigned gcmarkbit : 1;
     int spacer : 15;
     struct Lisp_Overlay *next;
@@ -1394,7 +1413,11 @@
    This type of object is used in the arg to record_unwind_protect.  */
 struct Lisp_Save_Value
   {
+#ifdef _MSC_VER
+    unsigned short type : 16;
+#else
     enum Lisp_Misc_Type type : 16;	/* = Lisp_Misc_Save_Value */
+#endif
     unsigned gcmarkbit : 1;
     int spacer : 14;
     /* If DOGC is set, POINTER is the address of a memory
@@ -1408,7 +1431,11 @@
 /* A miscellaneous object, when it's on the free list.  */
 struct Lisp_Free
   {
+#ifdef _MSC_VER
+    unsigned short type : 16;
+#else
     enum Lisp_Misc_Type type : 16;	/* = Lisp_Misc_Free */
+#endif
     unsigned gcmarkbit : 1;
     int spacer : 15;
     union Lisp_Misc *chain;
@@ -1800,13 +1827,23 @@

 /* 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 ;				\
   DECL_ALIGN (struct Lisp_Subr, sname) =				\
     { PVEC_SUBR | (sizeof (struct Lisp_Subr) / sizeof (EMACS_INT)),	\
-      { .a ## maxargs = fnname },				\
+      { (Lisp_Object (__cdecl *)(void))fnname },                        \
       minargs, maxargs, lname, intspec, 0};				\
   Lisp_Object fnname
+#else
+#define DEFUN(lname, fnname, sname, minargs, maxargs, intspec, doc)	\
+  Lisp_Object fnname DEFUN_ARGS_ ## maxargs ;				\
+  DECL_ALIGN (struct Lisp_Subr, sname) =				\
+    { PVEC_SUBR | (sizeof (struct Lisp_Subr) / sizeof (EMACS_INT)),	\
+      { .a ## maxargs = fnname },					\
+      minargs, maxargs, lname, intspec, 0};				\
+  Lisp_Object fnname
+#endif

 /* Note that the weird token-substitution semantics of ANSI C makes
    this work for MANY and UNEVALLED.  */
diff -ruN ..\mirror\emacs-bzr\trunk\src/makefile.w32-in emacs-gnu\src/makefile.w32-in
--- ..\mirror\emacs-bzr\trunk\src/makefile.w32-in	2010-11-18 08:53:04.000000000 +0100
+++ emacs-gnu\src/makefile.w32-in	2010-12-13 21:48:10.000000000 +0100
@@ -303,14 +303,14 @@
 	$(MAKE) $(MFLAGS) TAGS-LISP-$(MAKETYPE)

 TAGS-gmake:
-	../lib-src/$(BLD)/etags.exe --include=TAGS-LISP --include=../nt/TAGS \
-	  --regex=@../nt/emacs-src.tags \
-	  $(patsubst $(BLD)%.$(O),$(CURDIR)%.c,$(OBJ0))
-	../lib-src/$(BLD)/etags.exe -a --regex=@../nt/emacs-src.tags \
-	  $(patsubst $(BLD)%.$(O),$(CURDIR)%.c,$(OBJ1))
-	../lib-src/$(BLD)/etags.exe -a --regex=@../nt/emacs-src.tags \
-	  $(patsubst $(BLD)%.$(O),$(CURDIR)%.c,$(OBJ2)) \
-	  $(CURDIR)/*.h
+# ../lib-src/$(BLD)/etags.exe --include=TAGS-LISP --include=../nt/TAGS \
+#   --regex=@../nt/emacs-src.tags \
+#   $(patsubst $(BLD)%.$(O),$(CURDIR)%.c,$(OBJ0))
+# ../lib-src/$(BLD)/etags.exe -a --regex=@../nt/emacs-src.tags \
+#   $(patsubst $(BLD)%.$(O),$(CURDIR)%.c,$(OBJ1))
+# ../lib-src/$(BLD)/etags.exe -a --regex=@../nt/emacs-src.tags \
+#   $(patsubst $(BLD)%.$(O),$(CURDIR)%.c,$(OBJ2)) \
+#   $(CURDIR)/*.h

 TAGS-nmake:
 	echo This target is not supported with NMake
diff -ruN ..\mirror\emacs-bzr\trunk\src/ralloc.c emacs-gnu\src/ralloc.c
--- ..\mirror\emacs-bzr\trunk\src/ralloc.c	2010-11-18 08:53:04.000000000 +0100
+++ emacs-gnu\src/ralloc.c	2010-12-13 21:57:24.000000000 +0100
@@ -651,7 +651,7 @@
       else
 	{
 	  memmove (bloc->new_data, bloc->data, old_size);
-	  memset (bloc->new_data + old_size, 0, size - old_size);
+	  memset ((char *) bloc->new_data + old_size, 0, size - old_size);
 	  *bloc->variable = bloc->data = bloc->new_data;
 	}
     }
diff -ruN ..\mirror\emacs-bzr\trunk\src/regex.c emacs-gnu\src/regex.c
--- ..\mirror\emacs-bzr\trunk\src/regex.c	2010-11-18 08:53:04.000000000 +0100
+++ emacs-gnu\src/regex.c	2010-12-13 16:07:44.000000000 +0100
@@ -569,7 +569,12 @@
 #define MIN(a, b) ((a) < (b) ? (a) : (b))

 /* Type of source-pattern and string chars.  */
+#ifdef _MSC_VER
+/* MSVC barks with double const */
+typedef unsigned char re_char;
+#else
 typedef const unsigned char re_char;
+#endif

 typedef char boolean;
 #define false 0
diff -ruN ..\mirror\emacs-bzr\trunk\src/s/ms-w32.h emacs-gnu\src/s/ms-w32.h
--- ..\mirror\emacs-bzr\trunk\src/s/ms-w32.h	2010-11-18 08:53:04.000000000 +0100
+++ emacs-gnu\src/s/ms-w32.h	2010-12-13 23:20:18.000000000 +0100
@@ -87,6 +87,10 @@
 #define IS_ANY_SEP(_c_) (IS_DIRECTORY_SEP (_c_) || IS_DEVICE_SEP (_c_))

 #include <sys/types.h>
+#ifdef _MSC_VER
+typedef unsigned long sigset_t;
+#endif
+
 struct sigaction {
   int sa_flags;
   void (*sa_handler)(int);
@@ -190,6 +194,12 @@

 #ifdef emacs

+#ifdef _MSC_VER
+#include <sys/timeb.h>
+#include <sys/stat.h>
+#include <signal.h>
+#endif
+
 /* Calls that are emulated or shadowed.  */
 #undef access
 #define access  sys_access
@@ -208,6 +218,9 @@
 #undef dup2
 #define dup2    sys_dup2
 #define fopen   sys_fopen
+#if 1
+#define fstat(a, b)   sys_fstat(a, b)
+#endif
 #define link    sys_link
 #define mkdir   sys_mkdir
 #undef mktemp
@@ -221,9 +234,13 @@
 #define rmdir   sys_rmdir
 #define select  sys_select
 #define sleep   sys_sleep
+#if 1
+#define stat(a, b)   sys_stat(a, b)
+#endif
 #define strerror sys_strerror
 #undef unlink
 #define unlink  sys_unlink
+#define utime   sys_utime
 #undef write
 #define write   sys_write

@@ -325,13 +342,17 @@
 #define _WINSOCK_H

 /* Defines size_t and alloca ().  */
-#ifdef USE_CRT_DLL
+#ifdef emacs
 #define malloc e_malloc
 #define free   e_free
 #define realloc e_realloc
 #define calloc e_calloc
 #endif
+#if 0
 #include <malloc.h>
+#else
+#define alloca _alloca
+#endif

 #include <sys/stat.h>

diff -ruN ..\mirror\emacs-bzr\trunk\src/w32.c emacs-gnu\src/w32.c
--- ..\mirror\emacs-bzr\trunk\src/w32.c	2010-11-18 08:53:04.000000000 +0100
+++ emacs-gnu\src/w32.c	2010-12-13 21:52:37.000000000 +0100
@@ -92,7 +92,10 @@

 #include <tlhelp32.h>
 #include <psapi.h>
+#ifdef _MSC_VER
+#else
 #include <w32api.h>
+#endif
 #if !defined(__MINGW32__) || __W32API_MAJOR_VERSION < 3 || (__W32API_MAJOR_VERSION == 3 && __W32API_MINOR_VERSION < 15)
 /* This either is not in psapi.h or guarded by higher value of
    _WIN32_WINNT than what we use.  w32api suplied with MinGW 3.15
@@ -1541,7 +1544,7 @@
 	 read-only filesystem, like CD-ROM or a write-protected floppy.
 	 The only way to be really sure is to actually create a file and
 	 see if it succeeds.  But I think that's too much to ask.  */
-      if (tmp && _access (tmp, D_OK) == 0)
+      if (tmp && sys_access (tmp, D_OK) == 0)
 	{
 	  char * var = alloca (strlen (tmp) + 8);
 	  sprintf (var, "TMPDIR=%s", tmp);
@@ -1887,6 +1890,8 @@
   return configuration_buffer;
 }

+#define stringer( x ) ( #x )
+
 char *
 get_emacs_configuration_options (void)
 {
@@ -1900,10 +1905,10 @@
     /* configure.bat already sets USER_CFLAGS and USER_LDFLAGS
        with a starting space to save work here.  */
 #ifdef USER_CFLAGS
-    " --cflags", USER_CFLAGS,
+    " --cflags", stringer(USER_CFLAGS),
 #endif
 #ifdef USER_LDFLAGS
-    " --ldflags", USER_LDFLAGS,
+    " --ldflags", stringer(USER_LDFLAGS),
 #endif
     NULL
   };
diff -ruN ..\mirror\emacs-bzr\trunk\src/w32fns.c emacs-gnu\src/w32fns.c
--- ..\mirror\emacs-bzr\trunk\src/w32fns.c	2010-11-18 08:53:04.000000000 +0100
+++ emacs-gnu\src/w32fns.c	2010-12-13 21:59:17.000000000 +0100
@@ -246,7 +246,7 @@
 };

 /* Reportedly, VS 6 does not have this in its headers.  */
-#if defined (_MSC_VER) && _MSC_VER < 1300
+#if defined(_MSC_VER) /* && _MSC_VER < 1300 */
 DECLARE_HANDLE(HMONITOR);
 #endif

diff -ruN ..\mirror\emacs-bzr\trunk\lib-src/movemail.c emacs-gnu\lib-src/movemail.c
--- ..\mirror\emacs-bzr\trunk\lib-src/movemail.c	2010-11-18 08:53:04.000000000 +0100
+++ emacs-gnu\lib-src/movemail.c	2010-12-13 23:21:27.000000000 +0100
@@ -164,6 +164,10 @@
 /* Nonzero means this is name of a lock file to delete on fatal error.  */
 char *delete_lockname;

+#ifdef _MSC_VER
+typedef int ssize_t;
+#endif
+
 int
 main (int argc, char **argv)
 {
diff -ruN ..\mirror\emacs-bzr\trunk\nt/configure.bat emacs-gnu\nt/configure.bat
--- ..\mirror\emacs-bzr\trunk\nt/configure.bat	2010-11-18 08:53:04.000000000 +0100
+++ emacs-gnu\nt/configure.bat	2010-12-13 16:10:24.000000000 +0100
@@ -294,6 +294,18 @@
 rem   Auto-detect compiler if not specified, and validate GCC if chosen.

 :checkcompiler
+
+rem set SDK environment
+if (%noopt%) == (Y) (
+   call "c:\Program Files\Microsoft SDKs\Windows\v7.1\Bin\SetEnv.cmd" /x86 /win7 /Debug
+   set nodebug=N
+)
+
+if (%nodebug%) == (Y) (
+   call "c:\Program Files\Microsoft SDKs\Windows\v7.1\Bin\SetEnv.cmd" /x86 /win7 /Release
+   set noopt=N
+)
+
 if (%COMPILER%)==(cl) goto compilercheckdone
 if (%COMPILER%)==(gcc) goto checkgcc

diff -ruN ..\mirror\emacs-bzr\trunk\nt/makefile.w32-in emacs-gnu\nt/makefile.w32-in
--- ..\mirror\emacs-bzr\trunk\nt/makefile.w32-in	2010-11-18 08:53:04.000000000 +0100
+++ emacs-gnu\nt/makefile.w32-in	2010-12-13 21:43:45.000000000 +0100
@@ -36,6 +36,7 @@

 .PHONY: $(ALL)

+ARCH_CFLAGS	= $(ARCH_CFLAGS) -I../nt/inc -I../src

 addpm:		  stamp_BLD $(BLD)/addpm.exe
 $(BLD)/addpm.exe: $(BLD)/addpm.$(O)
@@ -304,15 +305,15 @@
 	$(MAKE) $(MFLAGS) clean
 	cd ..\doc\lispintro
 	$(MAKE) $(MFLAGS) clean
-	cd ..\doc\lispref
+	cd ..\..\doc\lispref
 	$(MAKE) $(MFLAGS) clean
-	cd ..\leim
+	cd ..\..\leim
 	$(MAKE) $(MFLAGS) clean
 	cd ..\doc\emacs
 	$(MAKE) $(MFLAGS) clean
-	cd ..\doc\misc
+	cd ..\..\doc\misc
 	$(MAKE) $(MFLAGS) clean
-	cd ..\nt
+	cd ..\..\nt

 clean-other-dirs-gmake:
 	$(MAKE) $(MFLAGS) $(XMFLAGS) -C ../lib-src clean
@@ -366,13 +367,13 @@
 	$(MAKE) $(MFLAGS) distclean
 	cd ..\doc\emacs
 	$(MAKE) $(MFLAGS) distclean
-	cd ..\doc\misc
+	cd ..\..\doc\misc
 	$(MAKE) $(MFLAGS) distclean
-	cd ..\doc\lispintro
+	cd ..\..\doc\lispintro
 	$(MAKE) $(MFLAGS) distclean
-	cd ..\doc\lispref
+	cd ..\..\doc\lispref
 	$(MAKE) $(MFLAGS) distclean
-	cd ..\nt
+	cd ..\..\nt

 distclean-other-dirs-gmake:
 	$(MAKE) $(MFLAGS) $(XMFLAGS) -C ../lib-src distclean
@@ -397,13 +398,13 @@
 	$(MAKE) $(MFLAGS) maintainer-clean
 	cd ..\doc\emacs
 	$(MAKE) $(MFLAGS) maintainer-clean
-	cd ..\doc\misc
+	cd ..\..\doc\misc
 	$(MAKE) $(MFLAGS) maintainer-clean
-	cd ..\doc\lispintro
+	cd ..\..\doc\lispintro
 	$(MAKE) $(MFLAGS) maintainer-clean
-	cd ..\doc\lispref
+	cd ..\..\doc\lispref
 	$(MAKE) $(MFLAGS) maintainer-clean
-	cd ..\nt
+	cd ..\..\nt

 maintainer-clean-other-dirs-gmake:
 	$(MAKE) $(MFLAGS) $(XMFLAGS) -C ../lib-src maintainer-clean
@@ -422,9 +423,9 @@

 frc:
 TAGS-gmake: frc
-	../lib-src/$(BLD)/etags $(CURDIR)/*.c
-	$(MAKE) $(MFLAGS) -C ../src TAGS TAGS-LISP
-	$(MAKE) $(MFLAGS) -C ../lib-src TAGS
+# ../lib-src/$(BLD)/etags $(CURDIR)/*.c
+# $(MAKE) $(MFLAGS) -C ../src TAGS TAGS-LISP
+# $(MAKE) $(MFLAGS) -C ../lib-src TAGS

 TAGS-nmake:
 	echo This target is not supported with NMake
diff -ruN ..\mirror\emacs-bzr\trunk\nt/nmake.defs emacs-gnu\nt/nmake.defs
--- ..\mirror\emacs-bzr\trunk\nt/nmake.defs	2010-11-18 08:53:04.000000000 +0100
+++ emacs-gnu\nt/nmake.defs	2010-12-13 16:13:22.000000000 +0100
@@ -110,7 +110,13 @@
 RC_OUT		= -Fo
 RC_INCLUDE	= -i

-libc		= libc.lib
+!ifdef USE_CRT_DLL
+libc		= msvcrt$(D).lib
+XCFLAGS		= -I../nt/inc -I../src -D_DLL -D_MT -DUSE_CRT_DLL=1
+!else
+libc		= libcmt$(D).lib
+XCFLAGS		= -I../nt/inc -I../src -D_MT
+!endif
 baselibs	=
 O		= obj
 A		= lib
@@ -143,11 +149,15 @@
 CHECKING_CFLAGS	=
 !endif

-CFLAGS          = -I. $(ARCH_CFLAGS) \
+CFLAGS          = -I. $(XCFLAGS) $(ARCH_CFLAGS) \
 		  $(DEBUG_CFLAGS) $(CHECKING_CFLAGS) $(USER_CFLAGS) $(LOCAL_FLAGS)
 EMACS_EXTRA_C_FLAGS =

-SYS_LDFLAGS	= -nologo -release -incremental:no -version:3.10 -swaprun:cd -swaprun:net setargv.obj
+!ifdef NOOPT
+SYS_LDFLAGS	= -nologo -manifest -dynamicbase:no -debug -incremental:no -version:3.10 -swaprun:cd -swaprun:net setargv.obj
+!else
+SYS_LDFLAGS	= -nologo -manifest -dynamicbase:no -release -incremental:no -version:3.10 -swaprun:cd -swaprun:net setargv.obj
+!endif

 # see comments in allocate_heap in w32heap.c before changing any of the
 # -stack, -heap, or -base settings.
@@ -183,16 +193,18 @@
 !ifdef NODEBUG
 DEBUG_FLAG =
 DEBUG_LINK =
+D =
 !else
 DEBUG_FLAG = -Zi
-DEBUG_LINK = -debug:full
+DEBUG_LINK = -debug
+D = d
 !endif

 !if "$(ARCH)" == "i386"
 !ifdef NOOPT
-ARCH_CFLAGS     = -nologo -c -Zel -W2 -H63 -Od -G3d -Zp8 $(DEBUG_FLAG)
+ARCH_CFLAGS     = -nologo -D_X86_=1 -c -Zl -Zp8 -W2 -Od -Gd $(DEBUG_FLAG)
 !else
-ARCH_CFLAGS     = -nologo -c -Zel -W2 -H63 -Oxsb2 -Oy- -G6dF -Zp8 $(DEBUG_FLAG)
+ARCH_CFLAGS     = -nologo -D_X86_=1 -c -Zl -Zp8 -W2 -Oi -Ot -Oy- -Ob2 -GF -Gy -Gd $(DEBUG_FLAG)
 !endif
 ARCH_LDFLAGS	= $(SYS_LDFLAGS)

Re: Compiling (development) Emacs with MSVC

[Eli Zaretskii]

> From: Fabrice Popineau <fabrice.popineau@supelec.fr>
> Date: Mon, 13 Dec 2010 23:40:56 +0100
> Cc: emacs-devel@gnu.org
> 
> Patch attached.

Thanks.  I finally found time to review it, and I have a few comments
and questions:

> Apparently the ANSI C spec says that bit fields are unsigned, and enum are
> signed, hence the patch in lisp.h for various bit fields.
> Anyway, the MS compiler did extend sign and that was the main failure.

The problem and the solution you suggest are clear, but I'd like to
try to find a cleaner solution, if possible, one that doesn't require
so many #ifdef's.  Can you think of some change in one or two places
that would resolve this?  Can MSVC be forced in some way to use an
unsigned type for enumerations, for example?  If not, since the
problem is only with a single enumeration, would it work to use
something like this:

enum Lisp_Misc_Type
  {
    Lisp_Misc_Free = 0x5eab,
    Lisp_Misc_Marker = 0x5eac,
    Lisp_Misc_Intfwd = 0x5ead,

etc.?  Or maybe use 0x5eabU etc., with an explicitly unsigned value?

A few other issues with the rest of the patch:

> -#   define DECL_ALIGN(type, var) \
> +#   define DECL_ALIGN(type, var)                                \
>       type __attribute__ ((__aligned__ (1 << GCTYPEBITS))) var

Is this just some inadvertent reformatting, or is there some deeper
problem here?

>  TAGS-gmake:
> -	../lib-src/$(BLD)/etags.exe --include=TAGS-LISP --include=../nt/TAGS \
> -	  --regex=@../nt/emacs-src.tags \
> -	  $(patsubst $(BLD)%.$(O),$(CURDIR)%.c,$(OBJ0))
> -	../lib-src/$(BLD)/etags.exe -a --regex=@../nt/emacs-src.tags \
> -	  $(patsubst $(BLD)%.$(O),$(CURDIR)%.c,$(OBJ1))
> -	../lib-src/$(BLD)/etags.exe -a --regex=@../nt/emacs-src.tags \
> -	  $(patsubst $(BLD)%.$(O),$(CURDIR)%.c,$(OBJ2)) \
> -	  $(CURDIR)/*.h
> +# ../lib-src/$(BLD)/etags.exe --include=TAGS-LISP --include=../nt/TAGS \
> +#   --regex=@../nt/emacs-src.tags \
> +#   $(patsubst $(BLD)%.$(O),$(CURDIR)%.c,$(OBJ0))
> +# ../lib-src/$(BLD)/etags.exe -a --regex=@../nt/emacs-src.tags \
> +#   $(patsubst $(BLD)%.$(O),$(CURDIR)%.c,$(OBJ1))
> +# ../lib-src/$(BLD)/etags.exe -a --regex=@../nt/emacs-src.tags \
> +#   $(patsubst $(BLD)%.$(O),$(CURDIR)%.c,$(OBJ2)) \
> +#   $(CURDIR)/*.h

Why did you comment out these commands?  They are not supposed to be
invoked under nmake, only under GNU Make.

> -	  memset (bloc->new_data + old_size, 0, size - old_size);
> +	  memset ((char *) bloc->new_data + old_size, 0, size - old_size);

Is this a real problem, or just a left-over from some attempt to debug
memory allocation problems, resolved by "-dynamicbase:no"?  If this is
a real problem, can you tell what it is?

> +#define fstat(a, b)   sys_fstat(a, b)
> +#define stat(a, b)   sys_stat(a, b)
> +#define utime   sys_utime

Why did you need to do this?  What happens if you don't?

> -      if (tmp && _access (tmp, D_OK) == 0)
> +      if (tmp && sys_access (tmp, D_OK) == 0)

What's wrong with calling _access from the MS runtime?

> -    " --cflags", USER_CFLAGS,
> +    " --cflags", stringer(USER_CFLAGS),

Why did you need to stringify here?  USER_CFLAGS is supposed to be
defined to a quoted string, like " -DFOO".  Can you tell why this
didn't work for you?

> +rem set SDK environment
> +if (%noopt%) == (Y) (
> +   call "c:\Program Files\Microsoft SDKs\Windows\v7.1\Bin\SetEnv.cmd" /x86 /win7 /Debug
> +   set nodebug=N
> +)
> +
> +if (%nodebug%) == (Y) (
> +   call "c:\Program Files\Microsoft SDKs\Windows\v7.1\Bin\SetEnv.cmd" /x86 /win7 /Release
> +   set noopt=N
> +)

Is there a less system-dependent way of doing this?  E.g., is there an
environment variable to replace the path to SetEnv.cmd?  Also, how to
set the /win7 switch portably, so that it works on non-Windows 7
platforms as well?  For that matter, is it at all necessary to call
SetEnv.cmd, in addition to using appropriate compiler/linker switches?

Thanks.

Re: Compiling (development) Emacs with MSVC

[Andreas Schwab]

Eli Zaretskii <eliz@gnu.org> writes:

>> -	  memset (bloc->new_data + old_size, 0, size - old_size);
>> +	  memset ((char *) bloc->new_data + old_size, 0, size - old_size);
>
> Is this a real problem, or just a left-over from some attempt to debug
> memory allocation problems, resolved by "-dynamicbase:no"?  If this is
> a real problem, can you tell what it is?

Arithmetic with pointers to void are not portable.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Compiling (development) Emacs with MSVC

[Fabrice Popineau]

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

Sorry for the long delay before replying.

The problem and the solution you suggest are clear, but I'd like to
> try to find a cleaner solution, if possible, one that doesn't require
> so many #ifdef's.  Can you think of some change in one or two places
> that would resolve this?  Can MSVC be forced in some way to use an
> unsigned type for enumerations, for example?  If not, since the
> problem is only with a single enumeration, would it work to use
> something like this:
>
> enum Lisp_Misc_Type
>  {
>    Lisp_Misc_Free = 0x5eab,
>    Lisp_Misc_Marker = 0x5eac,
>    Lisp_Misc_Intfwd = 0x5ead,
>
> etc.?  Or maybe use 0x5eabU etc., with an explicitly unsigned value?
>

No, that doesn't work. And somehow, I'm glad that the compiler can't be
fooled this way.
After looking around and googling about this issue, the problem is trickier
than what I thought
at first. Even my first explanation may not be complete. Anyway, mixing enum
and bit fields
seems to be incompatible with portability. Either you need to step back and
type bit fields
with int or unsigned int (and loose typechecking) or  you need to get around
with macros
taking care of using enum or unsigned ints depending on the compiler.




> A few other issues with the rest of the patch:
>
> > -#   define DECL_ALIGN(type, var) \
> > +#   define DECL_ALIGN(type, var)                                \
> >       type __attribute__ ((__aligned__ (1 << GCTYPEBITS))) var
>
> Is this just some inadvertent reformatting, or is there some deeper
> problem here?
>

Only reformatting, sorry. I'm not even sure that I needed to define
DECL_ALIGN for msvc,
because I set an option on the command line to align everything.



> >  TAGS-gmake:
> > -     ../lib-src/$(BLD)/etags.exe --include=TAGS-LISP
> --include=../nt/TAGS \
> > -       --regex=@../nt/emacs-src.tags \
> > -       $(patsubst $(BLD)%.$(O),$(CURDIR)%.c,$(OBJ0))
> > -     ../lib-src/$(BLD)/etags.exe -a --regex=@../nt/emacs-src.tags \
> > -       $(patsubst $(BLD)%.$(O),$(CURDIR)%.c,$(OBJ1))
> > -     ../lib-src/$(BLD)/etags.exe -a --regex=@../nt/emacs-src.tags \
> > -       $(patsubst $(BLD)%.$(O),$(CURDIR)%.c,$(OBJ2)) \
> > -       $(CURDIR)/*.h
> > +# ../lib-src/$(BLD)/etags.exe --include=TAGS-LISP --include=../nt/TAGS \
> > +#   --regex=@../nt/emacs-src.tags \
> > +#   $(patsubst $(BLD)%.$(O),$(CURDIR)%.c,$(OBJ0))
> > +# ../lib-src/$(BLD)/etags.exe -a --regex=@../nt/emacs-src.tags \
> > +#   $(patsubst $(BLD)%.$(O),$(CURDIR)%.c,$(OBJ1))
> > +# ../lib-src/$(BLD)/etags.exe -a --regex=@../nt/emacs-src.tags \
> > +#   $(patsubst $(BLD)%.$(O),$(CURDIR)%.c,$(OBJ2)) \
> > +#   $(CURDIR)/*.h
>
> Why did you comment out these commands?  They are not supposed to be
> invoked under nmake, only under GNU Make.
>

They are not invoked, but the problem is that they are not parsable. Nmake
fails to read the makefile.



> > -       memset (bloc->new_data + old_size, 0, size - old_size);
> > +       memset ((char *) bloc->new_data + old_size, 0, size - old_size);
>
> Is this a real problem, or just a left-over from some attempt to debug
> memory allocation problems, resolved by "-dynamicbase:no"?  If this is
> a real problem, can you tell what it is?


MSVC can't/doesnt want to do arithmetic on void pointers.


> > +#define fstat(a, b)   sys_fstat(a, b)
> > +#define stat(a, b)   sys_stat(a, b)
> > +#define utime   sys_utime
>
> Why did you need to do this?  What happens if you don't?
>

If I don't do it, I get linking errors :
temacs2.lib(sysdep.obj) : error LNK2019: unresolved external symbol
__imp__stat referenced in function _sys_sigblock
temacs2.lib(image.obj) : error LNK2001: unresolved external symbol
__imp__stat
temacs1.lib(dired.obj) : error LNK2001: unresolved external symbol
__imp__stat
temacs1.lib(lread.obj) : error LNK2001: unresolved external symbol
__imp__stat
temacs1.lib(w32.obj) : error LNK2001: unresolved external symbol __imp__stat
temacs1.lib(fileio.obj) : error LNK2001: unresolved external symbol
__imp__stat
temacs2.lib(sysdep.obj) : error LNK2019: unresolved external symbol _utime
referenced in function _set_file_times

There are these 2 lines in src/s/ms-w32.h :

#if !defined (_MSC_VER) || (_MSC_VER < 1400)
#define tzname    _tzname
#define utime  _utime
#endif

If I raise the 1400 to a higher value to include my version of the compiler,
the compiler reports an error message about :

C:\Program Files (x86)\Microsoft Visual Studio 10.0\VC\INCLUDE\time.h(270) :
error C2090: function returns array

because tzname is defined as an array and that triggers a conflict.

Why would it be a problem to map thos three calls to their sys_ version ?
Performance ?

 > -      if (tmp && _access (tmp, D_OK) == 0)
> > +      if (tmp && sys_access (tmp, D_OK) == 0)
>
> What's wrong with calling _access from the MS runtime?


_access doesn't know about D_OK. Worse, D_OK is defined, it hides the fact
that _access
doesn't know about it. But _access crashes with this value. Anyway,
sys_access is defined, so why
not using it ?


>

> -    " --cflags", USER_CFLAGS,
> > +    " --cflags", stringer(USER_CFLAGS),
>
> Why did you need to stringify here?  USER_CFLAGS is supposed to be
> defined to a quoted string, like " -DFOO".  Can you tell why this
> didn't work for you?


Oops. I used this hack long ago, and I reintroduced it. But my real problem
is that user_cflags contains
various -I<path> to find include files, and these paths have \ that are not
doubled.
Stringifying is a bad idea and doesn't solve the problem at all. Need to
find a better solution.



> > +rem set SDK environment
> > +if (%noopt%) == (Y) (
> > +   call "c:\Program Files\Microsoft SDKs\Windows\v7.1\Bin\SetEnv.cmd"
> /x86 /win7 /Debug
> > +   set nodebug=N
> > +)
> > +
> > +if (%nodebug%) == (Y) (
> > +   call "c:\Program Files\Microsoft SDKs\Windows\v7.1\Bin\SetEnv.cmd"
> /x86 /win7 /Release
> > +   set noopt=N
> > +)
>
> Is there a less system-dependent way of doing this?  E.g., is there an
> environment variable to replace the path to SetEnv.cmd?


You may have several versions of the SDK. I put these lines to stress the
fact that people wanting
to compile emacs with the MS SDK need to set it up. Also, iti is needed to
set it up according to the debug/release mode.


> Also, how to
> set the /win7 switch portably, so that it works on non-Windows 7
> platforms as well?


This /win7 switch is unnecessary.


>  For that matter, is it at all necessary to call
> SetEnv.cmd, in addition to using appropriate compiler/linker switches?
>
>
I guess so, yes, to find the compiler, the include files and the libraries.

Also, note that to copile the whole thing, I need to run :

nmake USE_CRT_DLL=1

My best wishes for 2011.
Happy New Year.

Fabrice

[-- Attachment #2: Type: text/html, Size: 9573 bytes --]

Re: Compiling (development) Emacs with MSVC

[Stefan Monnier]

> After looking around and googling about this issue, the problem is
> trickier than what I thought at first.  Even my first explanation may
> not be complete.  Anyway, mixing enum and bit fields seems to be
> incompatible with portability.

IIUC gcc uses something like


   ENUM_BF(Lisp_Misc_Type) tag : 3;

where ENUM_BF(x) can either turn into "enum x" or "unsigned int"
depending on whether the compiler supports unsigned bit fields.
   

        Stefan

Re: Compiling (development) Emacs with MSVC

[Tom Tromey]

Stefan> IIUC gcc uses something like
Stefan>    ENUM_BF(Lisp_Misc_Type) tag : 3;
Stefan> where ENUM_BF(x) can either turn into "enum x" or "unsigned int"
Stefan> depending on whether the compiler supports unsigned bit fields.
   
gdb uses:

/* Be conservative and use enum bitfields only with GCC.
   This is copied from gcc 3.3.1, system.h.  */

#if defined(__GNUC__) && (__GNUC__ >= 2)
#define ENUM_BITFIELD(TYPE) enum TYPE
#else
#define ENUM_BITFIELD(TYPE) unsigned int
#endif


GCC uses:

/* Be conservative and only use enum bitfields with GCC.
   FIXME: provide a complete autoconf test for buggy enum bitfields.  */

#if (GCC_VERSION > 2000)
#define ENUM_BITFIELD(TYPE) __extension__ enum TYPE
#else
#define ENUM_BITFIELD(TYPE) unsigned int
#endif


Tom

Re: Compiling (development) Emacs with MSVC

[Andreas Schwab]

Fabrice Popineau <fabrice.popineau@supelec.fr> writes:

> The portable dumper (once written for emacs 21 and avalaible in xemacs)
> would be a very good idea.

Except that it isn't portable, it mixes function pointers with data
pointers which is a no-no.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Compiling (development) Emacs with MSVC

[Fabrice Popineau]

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

2010/12/18 Andreas Schwab <schwab@linux-m68k.org>

> Fabrice Popineau <fabrice.popineau@supelec.fr> writes:
>
> > The portable dumper (once written for emacs 21 and avalaible in xemacs)
> > would be a very good idea.
>
> Except that it isn't portable, it mixes function pointers with data
> pointers which is a no-no.


Ok, it would need adaptation to the current GNU Emacs Lisp structures, but
the fact that it mixes function pointers with data ... it does it better
than what does the undumping mechanism used by GNU Emacs for the moment. GNU
Emacs undumping mechanism is platform dependent and doesn't work with the
dynamic code relocation (Linux is also able to do that). I have not played
with xemacs recently, but the portable dumper considerably reduced problems
of this kind.

Fabrice

[-- Attachment #2: Type: text/html, Size: 1187 bytes --]

Re: Compiling (development) Emacs with MSVC

[Stephen J. Turnbull]

Fabrice Popineau writes:
 > 2010/12/18 Andreas Schwab <schwab@linux-m68k.org>
 > 
 > > Fabrice Popineau <fabrice.popineau@supelec.fr> writes:
 > >
 > > > The portable dumper (once written for emacs 21 and avalaible in xemacs)
 > > > would be a very good idea.

AFAIK Emacs's portable dumper and XEmacs's share no code.

 > > Except that it isn't portable, it mixes function pointers with data
 > > pointers which is a no-no.

Point taken.  But in practice it has worked everywhere except some
64-bit Intel platform which is sufficiently rare that nobody in XEmacs
core dev circles has access to one, and maybe with SELinux cranked up
to maximum orneriness.