[jay_finger@hotmail.com: Two problems in Emacs-21.2.91 on Windows]

[jay_finger@hotmail.com: Two problems in Emacs-21.2.91 on Windows]

[Richard Stallman]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 3386 bytes --]

Any ideas for what to do here?

------- Start of forwarded message -------
Envelope-to: emacs-pretest-bug@gnu.org
Delivery-date: Mon, 21 Oct 2002 13:43:36 -0400
X-Originating-IP: [207.46.137.206]
From: "Jay Finger" <jay_finger@hotmail.com>
To: emacs-pretest-bug@gnu.org
Bcc: 
Subject: Two problems in Emacs-21.2.91 on Windows
Date: Mon, 21 Oct 2002 17:43:14 +0000
X-OriginalArrivalTime: 21 Oct 2002 17:43:14.0779 (UTC) FILETIME=[551402B0:01C27929]
X-Spam-Status: No, hits=0.6 required=5.0
	tests=SPAM_PHRASE_00_01
	version=2.41
X-Spam-Level: 

[Forgive me if this is the wrong alias for posting pretest bugs; I’ve been 
idling on this alias for a long time and my archive it doesn’t contain the 
procedure any more.  I’d be happy for pointers about where to rtfm about 
that.]

I’ve found two unrelated problems:  one when building with MSVC 7, the other 
in the w32 code.

1)  When building with MSVC 7:

A linker error comes up complaining about multiply defined symbols for 
_heap_init and _heap_term.  These are both defined in w32heap.c, apparently 
to prevent the functions with that name in the CRT from initializing the 
heap.  In the MSVC 7 libc.lib there are a couple of additional symbols 
defined in the object with these functions that cause that object to get 
sucked in, whereas it didn't used to get included.  Simply removing
libc.lib(heapinit.obj) from the library creates it's own problems as it 
removes other symbols needed by the CRT.

I found two hacks that seem to work, but I don't really like either because 
there are sure to be issues I don't understand:

Linking with the libc.lib from MSVC 6 works: it links, Emacs seems to work 
fine (I haven't played with any features added since 21.1.1, what I've been 
running).  I don't like this, though, since it requires hacking the build 
environment, and there may be dependencies between the compiler and libc.lib 
that I'm unaware of.  (You definitely need the V7 libc.lib if you want to 
compile with the /GS option, which was invaluable for finding the stack 
corruption described below).

The other hack that seems to work OK (it links and runs with caveats as 
above), is to just remove the definitions of _heap_init and _heap_term from 
w32heap.c.  The comment above those says "They are normally defined by the 
runtime, but we override them here so that the unnecessary HeapCreate call 
is not performed."  If it's just an extra heap that we're worried about, 
this should be slightly wasteful but harmless.  But maybe there is some 
problem caused by having that heap initialized that the comment doesn't 
record?

2) Bug in w32_term_init in w32term.c.

This function calls w32_defined_color to do lookups of colors "white" and 
"black".  It passes a pointer to a COLORDEF, but w32_term_init expects a 
pointer to an XColor.  Debug builds run fine, but on optimized builds you 
get a stack corruption and Emacs fails before painting the first window.  I 
hacked this by pasting in the definition for XColor into w32term.c and 
passing in one of those, but I figure somebody would want to actually move 
that structure to a header.  A function prototype wouldn't hurt either :-)

jay


_________________________________________________________________
Broadband? Dial-up? Get reliable MSN Internet Access. 
http://resourcecenter.msn.com/access/plans/default.asp
------- End of forwarded message -------

Re: [jay_finger@hotmail.com: Two problems in Emacs-21.2.91 on Windows]

[Juanma Barranquero]

On Wed, 23 Oct 2002 03:10:35 -0400, Richard Stallman <rms@gnu.org> wrote:

> Any ideas for what to do here?

With respect to:

> 2) Bug in w32_term_init in w32term.c.
> 
> This function calls w32_defined_color to do lookups of colors "white" and 
> "black".  It passes a pointer to a COLORDEF, but w32_term_init expects a 
> pointer to an XColor.  Debug builds run fine, but on optimized builds you 
> get a stack corruption and Emacs fails before painting the first window.  I 
> hacked this by pasting in the definition for XColor into w32term.c and 
> passing in one of those, but I figure somebody would want to actually move 
> that structure to a header.  A function prototype wouldn't hurt either :-)

the following patch would do, I think.

Basically it makes sense to extract the definition of XColor from the
three or four places it happens now and define it specifically in the
*gui.h files of the platforms that need it (w32gui.h and macgui.h at
this moment). The same bug is also present in EMACS_21_1_RC, BTW. If the
patch is approved it should be applied to the pretest too.

It works on my system. I've not commited the patch because I don't want
to change Mac files without Andrew Choi's approval, specially as I
can not test the changes on that platform. 

I've maintained the #ifndef HAVE_X_WINDOWS guards. On Window systems
probably that's unnecesary, but on Macs I bet they're needed if you
happen to build on Mac OS X.

Comments?

                                                           /L/e/k/t/u



Index: macfns.c
===================================================================
RCS file: /cvs/emacs/src/macfns.c,v
retrieving revision 1.16
diff -u -2 -r1.16 macfns.c
--- macfns.c	18 Oct 2002 09:57:48 -0000	1.16
+++ macfns.c	23 Oct 2002 11:48:30 -0000
@@ -100,14 +100,4 @@
 extern int quit_char;*/
 
-/* A definition of XColor for non-X frames.  */
-#ifndef HAVE_X_WINDOWS
-typedef struct {
-  unsigned long pixel;
-  unsigned short red, green, blue;
-  char flags;
-  char pad;
-} XColor;
-#endif
-
 extern char *lispy_function_keys[];
 
Index: macgui.h
===================================================================
RCS file: /cvs/emacs/src/macgui.h,v
retrieving revision 1.1
diff -u -2 -r1.1 macgui.h
--- macgui.h	26 Apr 2002 23:39:05 -0000	1.1
+++ macgui.h	23 Oct 2002 11:48:30 -0000
@@ -38,4 +38,14 @@
 #endif
 
+/* A definition of XColor for non-X frames.  */
+#ifndef HAVE_X_WINDOWS
+typedef struct {
+  unsigned long pixel;
+  unsigned short red, green, blue;
+  char flags;
+  char pad;
+} XColor;
+#endif
+
 #define FACE_DEFAULT (~0)
 
Index: w32fns.c
===================================================================
RCS file: /cvs/emacs/src/w32fns.c,v
retrieving revision 1.185
diff -u -2 -r1.185 w32fns.c
--- w32fns.c	18 Oct 2002 09:54:27 -0000	1.185
+++ w32fns.c	23 Oct 2002 11:48:31 -0000
@@ -65,14 +65,4 @@
 extern int quit_char;
 
-/* A definition of XColor for non-X frames.  */
-#ifndef HAVE_X_WINDOWS
-typedef struct {
-  unsigned long pixel;
-  unsigned short red, green, blue;
-  char flags;
-  char pad;
-} XColor;
-#endif
-
 extern char *lispy_function_keys[];
 
Index: w32gui.h
===================================================================
RCS file: /cvs/emacs/src/w32gui.h,v
retrieving revision 1.15
diff -u -2 -r1.15 w32gui.h
--- w32gui.h	20 Mar 2002 21:00:50 -0000	1.15
+++ w32gui.h	23 Oct 2002 11:48:31 -0000
@@ -87,4 +87,14 @@
 } XImage;
 
+/* A definition of XColor for non-X frames.  */
+#ifndef HAVE_X_WINDOWS
+typedef struct {
+  unsigned long pixel;
+  unsigned short red, green, blue;
+  char flags;
+  char pad;
+} XColor;
+#endif
+
 #define FACE_DEFAULT (~0)
 
Index: w32term.c
===================================================================
RCS file: /cvs/emacs/src/w32term.c,v
retrieving revision 1.166
diff -u -2 -r1.166 w32term.c
--- w32term.c	30 Aug 2002 13:19:45 -0000	1.166
+++ w32term.c	23 Oct 2002 11:48:32 -0000
@@ -11125,5 +11125,5 @@
   /* initialise palette with white and black */
   {
-    COLORREF color;
+    XColor color;
     w32_defined_color (0, "white", &color, 1);
     w32_defined_color (0, "black", &color, 1);
Index: xfaces.c
===================================================================
RCS file: /cvs/emacs/src/xfaces.c,v
retrieving revision 1.264
diff -u -2 -r1.264 xfaces.c
--- xfaces.c	27 Sep 2002 00:43:40 -0000	1.264
+++ xfaces.c	23 Oct 2002 11:48:33 -0000
@@ -298,19 +298,4 @@
 #define FACE_CACHE_BUCKETS_SIZE 1001
 
-/* A definition of XColor for non-X frames.  */
-
-#ifndef HAVE_X_WINDOWS
-
-typedef struct
-{
-  unsigned long pixel;
-  unsigned short red, green, blue;
-  char flags;
-  char pad;
-}
-XColor;
-
-#endif /* not HAVE_X_WINDOWS */
-
 /* Keyword symbols used for face attribute names.  */

Re: [jay_finger@hotmail.com: Two problems in Emacs-21.2.91 on Windows]

[Juanma Barranquero]

On Wed, 23 Oct 2002 03:10:35 -0400, Richard Stallman <rms@gnu.org> wrote:

> The other hack that seems to work OK (it links and runs with caveats as 
> above), is to just remove the definitions of _heap_init and _heap_term from 
> w32heap.c.  The comment above those says "They are normally defined by the 
> runtime, but we override them here so that the unnecessary HeapCreate call 
> is not performed."  If it's just an extra heap that we're worried about, 
> this should be slightly wasteful but harmless.

Also, we could perhaps get the heap handle (via GetProcessHeap ?) and
call HeapDestroy.


                                                           /L/e/k/t/u

Re: [jay_finger@hotmail.com: Two problems in Emacs-21.2.91 on Windows]

[jasonr]

> Basically it makes sense to extract the definition of XColor from the
> three or four places it happens now and define it specifically in the
> *gui.h files of the platforms that need it (w32gui.h and macgui.h at
> this moment).

I think the use of XColor in xfaces.c will affect
DOS as well. I'm not sure if there is a convenient
header to put the definition in for the DOS build.

Re: [jay_finger@hotmail.com: Two problems in Emacs-21.2.91 on Windows]

[Juanma Barranquero]

On Wed, 23 Oct 2002 14:37:35 +0100 (BST), jasonr@btinternet.com wrote:

> I think the use of XColor in xfaces.c will affect
> DOS as well. I'm not sure if there is a convenient
> header to put the definition in for the DOS build.

I suppose Eli's the one to know :)

Other than that, do you approve of the patch?

                                                           /L/e/k/t/u

Re: [jay_finger@hotmail.com: Two problems in Emacs-21.2.91 on Windows]

[jasonr]

> Other than that, do you approve of the patch?

Yes.

Re: [jay_finger@hotmail.com: Two problems in Emacs-21.2.91 on Windows]

[Andrew Choi]

Juanma Barranquero <lektu@terra.es> writes:

> [...] the following patch would do, I think.
> 
> Basically it makes sense to extract the definition of XColor from the
> three or four places it happens now and define it specifically in the
> *gui.h files of the platforms that need it (w32gui.h and macgui.h at
> this moment). The same bug is also present in EMACS_21_1_RC, BTW. If the
> patch is approved it should be applied to the pretest too.
> 
> It works on my system. I've not commited the patch because I don't want
> to change Mac files without Andrew Choi's approval, specially as I
> can not test the changes on that platform. 
> 
> I've maintained the #ifndef HAVE_X_WINDOWS guards. On Window systems
> probably that's unnecesary, but on Macs I bet they're needed if you
> happen to build on Mac OS X.
> 
> Comments?
> 
>                                                            /L/e/k/t/u
> [...]

Thank you for taking macgui.h into consideration.  I've just checked
that the change works fine on Mac OS X.

I believe the `#ifndef HAVE_X_WINDOWS' is needed when compiling for X
Window on Mac OS X.  Please keep it around.  Thanks.

Re: [jay_finger@hotmail.com: Two problems in Emacs-21.2.91 on Windows]

[Eli Zaretskii]

> From: jasonr@btinternet.com
> Date: Wed, 23 Oct 2002 14:37:35 +0100 (BST)
> 
> I think the use of XColor in xfaces.c will affect
> DOS as well.

Yes.

> I'm not sure if there is a convenient
> header to put the definition in for the DOS build.

msdos.h will do, I think (if it is #include'd by xfaces.c).

But I don't really understand why should the definition in xfaces.c be
removed.  xfaces.c is mostly platform-independent, so it would make
sense to actually leave that definition, perhaps with some more
#ifdef's if Windows and Mac need that, and remoev the other bunch.  Am
I missing something?

Re: [jay_finger@hotmail.com: Two problems in Emacs-21.2.91 on Windows]

[Eli Zaretskii]

> Date: Wed, 23 Oct 2002 18:47:08 +0200
> From: Juanma Barranquero <lektu@terra.es>
> 
> Ok. I'm going to commit the change. If it breaks DOS compilation Eli can
> say where to define XColor for DOS platforms.

What's the rush?  I'd rather we discussed this first, even if it will
cause a small delay.

Please note that I'm so swamped these days that any change that breaks
the MS-DOS build is likely to stay there for a long time.

Re: [jay_finger@hotmail.com: Two problems in Emacs-21.2.91 on Windows]

[Juanma Barranquero]

On Wed, 23 Oct 2002 10:19:19 -0600, Andrew Choi <akochoi@shaw.ca> wrote:

> I've just checked that the change works fine on Mac OS X.

Ok. I'm going to commit the change. If it breaks DOS compilation Eli can
say where to define XColor for DOS platforms.


                                                           /L/e/k/t/u

Re: [jay_finger@hotmail.com: Two problems in Emacs-21.2.91 on Windows]

[Juanma Barranquero]

On Wed, 23 Oct 2002 19:41:33 +0300, "Eli Zaretskii" <eliz@is.elta.co.il> wrote:

> msdos.h will do, I think (if it is #include'd by xfaces.c).

Ok.

> But I don't really understand why should the definition in xfaces.c be
> removed.  xfaces.c is mostly platform-independent, so it would make
> sense to actually leave that definition, perhaps with some more
> #ifdef's if Windows and Mac need that, and remoev the other bunch.  Am
> I missing something?

Well, on one hand I don't see the point of having XColor defined in
xfaces.c *and* other .c files (like w32term.c, for example). That's what
include files are for, aren't? But if I put XColor in a w32*.h, I had to
protect it from being redefined in xfaces.c, and that's just weird.

XColor is needed in some platforms (like Mac OS 9, Windows and DOS) and
not in others (like X or Mac OS X), so either you put it in a include
used by every build and guard it through #ifdef's, or put it in
platform-specific includes. That's what I chose because it seems
cleaner/simpler.

                                                           /L/e/k/t/u

Re: [jay_finger@hotmail.com: Two problems in Emacs-21.2.91 on Windows]

[Juanma Barranquero]

On Wed, 23 Oct 2002 19:43:24 +0300, "Eli Zaretskii" <eliz@is.elta.co.il> wrote:

> What's the rush?  I'd rather we discussed this first, even if it will
> cause a small delay.

Sorry for acting too fast. There was no rush, but is a bug to fix and I
didn't expect any problem or opposition. My fault.

> Please note that I'm so swamped these days that any change that breaks
> the MS-DOS build is likely to stay there for a long time.

OK. As I've already commited the fix to the other platforms, I'm going
to define XColor in msdos.h as you've suggested in another message.

If we determine later that is better to revert to the previous situation
or look for another fix, we can easily go back. 

                                                           /L/e/k/t/u

Re: [jay_finger@hotmail.com: Two problems in Emacs-21.2.91 on Windows]

[Eli Zaretskii]

On Thu, 24 Oct 2002, Juanma Barranquero wrote:

> Well, on one hand I don't see the point of having XColor defined in
> xfaces.c *and* other .c files (like w32term.c, for example). That's what
> include files are for, aren't?

We could use some header that is common to all platforms, with a suitable 
ifdef if needed.

Re: [jay_finger@hotmail.com: Two problems in Emacs-21.2.91 on Windows]

[Juanma Barranquero]

On Thu, 24 Oct 2002 09:13:26 +0200 (IST), Eli Zaretskii <eliz@is.elta.co.il> wrote:

> We could use some header that is common to all platforms, with a suitable 
> ifdef if needed.

I agree, if:

 - There's a suitable .h file (I mean, not lisp.h or any non-UI oriented
   .h file, which would be ugly).

 - It's already (directly or indirectly) included in the relevant .c
   files.

Otherwise we'll end adding XColor to a .h file and afterwards adding a
new include to several .c sources, just for one definition; when in fact
macgui.h and w32gui.h seem already very logical choices (and msdos.h too,
as it contains UI code).

But I'm *really* not arguing. If you know of such a .h file, I'm not
opposing to do it your way if people feels is better.

                                                           /L/e/k/t/u

Re: [jay_finger@hotmail.com: Two problems in Emacs-21.2.91 on Windows]

[jasonr]

> XColor is needed in some platforms (like Mac OS 9, Windows and DOS) and
> not in others (like X or Mac OS X), so either you put it in a include
> used by every build and guard it through #ifdef's, or put it in
> platform-specific includes. That's what I chose because it seems
> cleaner/simpler.

Having just one definition seems cleaner to me. Multiple
identical definitions lead to maintainence problems.
It has long been a goal to merge as much of the platform
specific display code as possible. So far only xfaces.c
has benefited due to lack of time. But we should at
least try to avoid introducing new duplicated code.

Re: [jay_finger@hotmail.com: Two problems in Emacs-21.2.91 on Windows]

[Juanma Barranquero]

On 24 Oct 2002 11:35:07 +0200, storm@cua.dk (Kim F. Storm) wrote:

> dispextern.h seems like a good choice.  It actually uses XColor in a prototype...

> I suggest adding the following near the top of the file:
> 
>   #ifdef HAVE_X_WINDOWS
>   #include <X11/Xlib.h>
>   #ifdef USE_X_TOOLKIT
>   #include <X11/Intrinsic.h>
>   #endif /* USE_X_TOOLKIT */
> + #else /* !HAVE_X_WINDOWS */
> +
> + /* X-related stuff used by non-X gui code. */
> +
> + typedef struct {
> +   unsigned long pixel;
> +   unsigned short red, green, blue;
> +   char flags;
> +   char pad;
> + } XColor;
> +
>   #endif /* HAVE_X_WINDOWS */

OK.

> Avoiding [verbatim] duplication of code or defs always make me feel better :-)

Yeah :)

OK, so I'll wait for a while and if no one objects I'll rework
yesterday's patch to use your suggested fix.

Should it also be done on EMACS_21_1_RC?


                                                           /L/e/k/t/u

Re: [jay_finger@hotmail.com: Two problems in Emacs-21.2.91 on Windows]

[Kim F. Storm]

Juanma Barranquero <lektu@terra.es> writes:

> On Thu, 24 Oct 2002 09:13:26 +0200 (IST), Eli Zaretskii <eliz@is.elta.co.il> wrote:
> 
> > We could use some header that is common to all platforms, with a suitable 
> > ifdef if needed.
> 
> I agree, if:
> 
>  - There's a suitable .h file (I mean, not lisp.h or any non-UI oriented
>    .h file, which would be ugly).

dispextern.h seems like a good choice.  It actually uses XColor in a prototype...

I suggest adding the following near the top of the file:

  #ifdef HAVE_X_WINDOWS
  #include <X11/Xlib.h>
  #ifdef USE_X_TOOLKIT
  #include <X11/Intrinsic.h>
  #endif /* USE_X_TOOLKIT */
+ #else /* !HAVE_X_WINDOWS */
+
+ /* X-related stuff used by non-X gui code. */
+
+ typedef struct {
+   unsigned long pixel;
+   unsigned short red, green, blue;
+   char flags;
+   char pad;
+ } XColor;
+
  #endif /* HAVE_X_WINDOWS */

> 
>  - It's already (directly or indirectly) included in the relevant .c
>    files.
> 
dispextern.h is included by all files using XColor!

> But I'm *really* not arguing. If you know of such a .h file, I'm not
> opposing to do it your way if people feels is better.

Avoiding [verbatim] duplication of code or defs always make me feel better :-)

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: [jay_finger@hotmail.com: Two problems in Emacs-21.2.91 on Windows]

[Eli Zaretskii]

> From: storm@cua.dk (Kim F. Storm)
> Date: 24 Oct 2002 11:35:07 +0200
> 
> dispextern.h seems like a good choice.

Yes, indeed.

Re: [jay_finger@hotmail.com: Two problems in Emacs-21.2.91 on Windows]

[Juanma Barranquero]

I've finally committed the patches to define XColor in dispextern.h
instead of platform-specific includes.

It should work seamlessly on Windows, Mac OS 9 and DOS, but any testing
would be welcome. Moreover, as the reported bug is a memory corruption
and so liable to cause instability, I suggest backporting the fix to
EMACS_21_1_RC.


                                                           /L/e/k/t/u

Re: [jay_finger@hotmail.com: Two problems in Emacs-21.2.91 on Windows]

[Harald.Maier.BW]

Richard Stallman <rms@gnu.org> writes:

> Any ideas for what to do here?
>
> From: "Jay Finger" <jay_finger@hotmail.com>
> Subject: Two problems in Emacs-21.2.91 on Windows
> 
> ...
>
> 1)  When building with MSVC 7:
>
> A linker error comes up complaining about multiply defined symbols for 
> _heap_init and _heap_term.  These are both defined in w32heap.c, apparently 
> to prevent the functions with that name in the CRT from initializing the 
> heap.  In the MSVC 7 libc.lib there are a couple of additional symbols 
> defined in the object with these functions that cause that object to get 
> sucked in, whereas it didn't used to get included.  Simply removing
> libc.lib(heapinit.obj) from the library creates it's own problems as it 
> removes other symbols needed by the CRT.
>
> I found two hacks that seem to work, but I don't really like either because 
> there are sure to be issues I don't understand:
>
> Linking with the libc.lib from MSVC 6 works: it links, Emacs seems to work 
> fine (I haven't played with any features added since 21.1.1, what I've been 
> running).  I don't like this, though, since it requires hacking the build 
> environment, and there may be dependencies between the compiler and libc.lib 
> that I'm unaware of.  (You definitely need the V7 libc.lib if you want to 
> compile with the /GS option, which was invaluable for finding the stack 
> corruption described below).
>
> The other hack that seems to work OK (it links and runs with caveats as 
> above), is to just remove the definitions of _heap_init and _heap_term from 
> w32heap.c.  The comment above those says "They are normally defined by the 
> runtime, but we override them here so that the unnecessary HeapCreate call 
> is not performed."  If it's just an extra heap that we're worried about, 
> this should be slightly wasteful but harmless.  But maybe there is some 
> problem caused by having that heap initialized that the comment doesn't 
> record?

I activated your later approach and this works fine. Personally, I
would take that approach beacause this works with less pain. Here a
patch with that compiling and linking works fine with MSVC 7. This is
only tested with the current development sources.

Harald

Index: w32heap.c
===================================================================
RCS file: /cvsroot/emacs/emacs/src/w32heap.c,v
retrieving revision 1.21
diff -c -r1.21 w32heap.c
*** w32heap.c	1 Jan 2002 19:10:04 -0000	1.21
--- w32heap.c	27 Oct 2002 16:50:10 -0000
***************
*** 282,288 ****
      sbrk (need_to_alloc);
  }
  
! #if (_MSC_VER >= 1000 && !defined(USE_CRT_DLL))
  
  /* MSVC 4.2 invokes these functions from mainCRTStartup to initialize
     a heap via HeapCreate.  They are normally defined by the runtime,
--- 282,288 ----
      sbrk (need_to_alloc);
  }
  
! #if (_MSC_VER >= 1000 && _MSC_VER < 1300 && !defined(USE_CRT_DLL))
  
  /* MSVC 4.2 invokes these functions from mainCRTStartup to initialize
     a heap via HeapCreate.  They are normally defined by the runtime,

Re: [jay_finger@hotmail.com: Two problems in Emacs-21.2.91 on Windows]

[Juanma Barranquero]

On Mon, 28 Oct 2002 06:57:42 +0100, Harald.Maier.BW@t-online.de wrote:

> I activated your later approach and this works fine. Personally, I
> would take that approach beacause this works with less pain. Here a
> patch with that compiling and linking works fine with MSVC 7. This is
> only tested with the current development sources.

I've installed it in the HEAD so more people can try it.  It (obviously)
works on MSVC 6 compilations.


                                                           /L/e/k/t/u