GNU bug report logs - #13783
simplify data_start configuration for Emacs

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 13783 in the body.
You can then email your comments to 13783 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: bug-gnu-emacs <at> gnu.org
Cc: Eli Zaretskii <eliz <at> gnu.org>
Subject: simplify data_start configuration for Emacs
Date: Fri, 22 Feb 2013 00:18:37 -0800

[Message part 1 (text/plain, inline)]

Tags: patch

Attached is a proposed patch that follows on to the Bug#13650
fixes, by simplifying the code to remove most of the
data_start stuff, which is obsolete.  This affects the
Windows port so I'm CC:ing this to Eli.

This patch is to Emacs trunk bzr 111855.

[data_start.txt (text/plain, attachment)]

Message #8 received at 13783 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 13783 <at> debbugs.gnu.org
Subject: Re: simplify data_start configuration for Emacs
Date: Fri, 22 Feb 2013 15:05:35 +0200

> Date: Fri, 22 Feb 2013 00:18:37 -0800
> From: Paul Eggert <eggert <at> cs.ucla.edu>
> CC: Eli Zaretskii <eliz <at> gnu.org>
> 
> Attached is a proposed patch that follows on to the Bug#13650
> fixes, by simplifying the code to remove most of the
> data_start stuff, which is obsolete.  This affects the
> Windows port so I'm CC:ing this to Eli.

Could you please explain more about what this is intended to
accomplish?  Some things seem wrong (a few specific comments below),
but I don't feel I understand this enough to suggest better/more
correct ways to do what you want.  E.g., are we removing the
data-start thing on all platforms where GCPRO is a no-op, i.e. those
which support USE_LSB_TAG?

> === modified file 'src/unexcoff.c'
> --- src/unexcoff.c	2013-01-02 16:13:04 +0000
> +++ src/unexcoff.c	2013-02-22 08:11:05 +0000
> @@ -99,7 +99,7 @@
>  
>  #include <sys/file.h>
>  
> -#include "mem-limits.h"
> +extern unsigned char *get_data_start (void);
>  
>  static long block_copy_start;		/* Old executable start point */
>  static struct filehdr f_hdr;		/* File header */
> @@ -168,7 +168,7 @@
>    pagemask = getpagesize () - 1;
>  
>    /* Adjust text/data boundary. */
> -  data_start = (int) start_of_data ();
> +  data_start = (int) get_data_start ();
>    data_start = ADDR_CORRECT (data_start);
>    data_start = data_start & ~pagemask; /* (Down) to page boundary. */

This part is wrong: unexcoff.c is used only by the MSDOS build, but
get_data_start is defined in w32heap.c, which is for the MS-Windows
build.  I'm not sure how to DTRT here (see above); all I can say at
this point is that the MSDOS build currently defines DATA_START, see
msdos/sed2v2.inp, and that definition is then used in start_of_data.

> +/* Start of data.  It is OK if this is approximate; it's used only as
> +   a heuristic.  */
> +extern char data_start[];
> +#ifndef HAVE_DATA_START
> +/* Initialize to nonzero, so that it's put into data and not bss.
> +   Link this file's object code first, so that this symbol is near the
> +   start of data.  */
> +char data_start[1] = { 1 };
> +#endif

Should platforms that HAVE_DATA_START initialize data_start to some
value?  Should _all_ platforms do that?  If not, which ones should?

>  #ifdef REL_ALLOC
> -  extern POINTER (*real_morecore) (ptrdiff_t);
> +  extern void *(*real_morecore) (ptrdiff_t);

This needs corresponding changes in ralloc.c, I think, at least for
consistency if nothing else.

> -  extern POINTER (*__morecore) (ptrdiff_t);
> +  extern void *(*__morecore) (ptrdiff_t);

Likewise.

Thanks.

Message #11 received at 13783 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 13783 <at> debbugs.gnu.org
Subject: Re: simplify data_start configuration for Emacs
Date: Fri, 22 Feb 2013 12:56:25 -0800

[Message part 1 (text/plain, inline)]

On 02/22/2013 05:05 AM, Eli Zaretskii wrote:

> Could you please explain more about what this is intended to
> accomplish?

Mostly, it's simplification.  There's a lot of cruft in there, about
linking Emacs specially and so forth, that was needed way back when we
made pure space read-only but is now no longer needed.

> are we removing the data-start thing on all platforms where GCPRO is
> a no-op, i.e. those which support USE_LSB_TAG?

USE_LSB_TAG is a different thing.  The patch omits data_start on hosts
that use the system malloc, since data_start is now needed only on
hosts where GNU Emacs supplies malloc.

USE_LSB_TAG is more closely related to DATA_SEG_BITS, but that
is not changed by this patch.

> Should platforms that HAVE_DATA_START initialize data_start to some
> value?  Should _all_ platforms do that?  If not, which ones should?

No, data_start's contents are irrelevant.  Only its address matters.
On GNU hosts the linker supplies the address automatically;
on non-GNU hosts we approximate it by declaring a
variable whose contents don't matter.

>> -  extern POINTER (*real_morecore) (ptrdiff_t);
>> +  extern void *(*real_morecore) (ptrdiff_t);
>
> This needs corresponding changes in ralloc.c, I think, at least for
> consistency if nothing else.

Done in trunk bzr 111859, which simplifies the attached patch.
Revised patch attached, relative to trunk bzr 111859.

>> -  data_start = (int) start_of_data ();
>> +  data_start = (int) get_data_start ();
>...
> This part is wrong: unexcoff.c is used only by the MSDOS build

Thanks for catching that.  I'll fix that as follows;
this fix is contained in the attached patch.

--- src/unexcoff.c	2013-02-22 08:11:05 +0000
+++ src/unexcoff.c	2013-02-22 19:39:27 +0000
@@ -99,7 +99,7 @@
 
 #include <sys/file.h>
 
-extern unsigned char *get_data_start (void);
+extern int etext[];
 
 static long block_copy_start;		/* Old executable start point */
 static struct filehdr f_hdr;		/* File header */
@@ -168,7 +168,7 @@
   pagemask = getpagesize () - 1;
 
   /* Adjust text/data boundary. */
-  data_start = (int) get_data_start ();
+  data_start = (int) (etext + 1);
   data_start = ADDR_CORRECT (data_start);
   data_start = data_start & ~pagemask; /* (Down) to page boundary. */

[data_start.txt (text/plain, attachment)]

Message #14 received at 13783 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 13783 <at> debbugs.gnu.org
Subject: Re: simplify data_start configuration for Emacs
Date: Fri, 22 Feb 2013 23:34:05 +0200

> Date: Fri, 22 Feb 2013 12:56:25 -0800
> From: Paul Eggert <eggert <at> cs.ucla.edu>
> CC: 13783 <at> debbugs.gnu.org
> 
> The patch omits data_start on hosts that use the system malloc,
> since data_start is now needed only on hosts where GNU Emacs
> supplies malloc.

You mean, gmalloc?  How does that come into play?

It seems like data_start and data_size derived from it matter only
when displaying memory usage warnings -- is that correct?  If so, why
doesn't this matter when system malloc is used?

> On GNU hosts the linker supplies the address automatically;
> on non-GNU hosts we approximate it by declaring a
> variable whose contents don't matter.

The MS-Windows build computes data_start and data_size inside
unexw32.c at dump time -- is that good enough?  (I'm asking because
your change doesn't touch the Windows build.)

Thanks.

Message #17 received at 13783 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 13783 <at> debbugs.gnu.org
Subject: Re: simplify data_start configuration for Emacs
Date: Fri, 22 Feb 2013 17:36:13 -0800

[Message part 1 (text/plain, inline)]

On 02/22/13 13:34, Eli Zaretskii wrote:
>> From: Paul Eggert <eggert <at> cs.ucla.edu>
>>
>> The patch omits data_start on hosts that use the system malloc,
>> since data_start is now needed only on hosts where GNU Emacs
>> supplies malloc.
> 
> You mean, gmalloc?  How does that come into play?

With the patch, data_start is used only by vm-limit.c.
And vm-limit.c is compiled only when using gmalloc,
as its isn't valid when using a system malloc.

> It seems like data_start and data_size derived from it matter only
> when displaying memory usage warnings -- is that correct?

Yes.

> If so, why doesn't this matter when system malloc is used?

When the system malloc is used Emacs can't warn about low memory,
because it has no way of knowing when memory is low.

> The MS-Windows build computes data_start and data_size inside
> unexw32.c at dump time -- is that good enough?  (I'm asking because
> your change doesn't touch the Windows build.)

Yes, it should be, but that points out a problem with the latest
patch -- it should use DATA_START for MS platforms.  Here's a further
patch to fix that, and I'm attaching the resulting combined patch,
relative to trunk bzr 111860.

=== modified file 'src/vm-limit.c'
--- src/vm-limit.c	2013-02-22 19:26:07 +0000
+++ src/vm-limit.c	2013-02-23 01:28:04 +0000
@@ -22,6 +22,7 @@
 
 #ifdef MSDOS
 #include <dpmi.h>
+extern int etext;
 #endif
 
 /* Some systems need this before <sys/resource.h>.  */
@@ -38,12 +39,16 @@
 
 /* Start of data.  It is OK if this is approximate; it's used only as
    a heuristic.  */
+#ifdef DATA_START
+# define data_start ((char *) DATA_START)
+#else
 extern char data_start[];
-#ifndef HAVE_DATA_START
+# ifndef HAVE_DATA_START
 /* Initialize to nonzero, so that it's put into data and not bss.
    Link this file's object code first, so that this symbol is near the
    start of data.  */
 char data_start[1] = { 1 };
+# endif
 #endif
 
 /*

[data_start.txt (text/plain, attachment)]

Message #20 received at 13783 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 13783 <at> debbugs.gnu.org
Subject: Re: simplify data_start configuration for Emacs
Date: Sat, 23 Feb 2013 12:47:17 +0200

> Date: Fri, 22 Feb 2013 17:36:13 -0800
> From: Paul Eggert <eggert <at> cs.ucla.edu>
> CC: 13783 <at> debbugs.gnu.org
> 
> With the patch, data_start is used only by vm-limit.c.
> And vm-limit.c is compiled only when using gmalloc,
> as its isn't valid when using a system malloc.
> 
> > It seems like data_start and data_size derived from it matter only
> > when displaying memory usage warnings -- is that correct?
> 
> Yes.
> 
> > If so, why doesn't this matter when system malloc is used?
> 
> When the system malloc is used Emacs can't warn about low memory,
> because it has no way of knowing when memory is low.

I see, thanks.

> > The MS-Windows build computes data_start and data_size inside
> > unexw32.c at dump time -- is that good enough?  (I'm asking because
> > your change doesn't touch the Windows build.)
> 
> Yes, it should be, but that points out a problem with the latest
> patch -- it should use DATA_START for MS platforms.  Here's a further
> patch to fix that, and I'm attaching the resulting combined patch,
> relative to trunk bzr 111860.

Thanks.

> --- src/unexcoff.c	2013-01-02 16:13:04 +0000
> +++ src/unexcoff.c	2013-02-22 19:43:17 +0000
> @@ -99,7 +99,7 @@
>  
>  #include <sys/file.h>
>  
> -#include "mem-limits.h"
> +extern int etext[];
>  
>  static long block_copy_start;		/* Old executable start point */
>  static struct filehdr f_hdr;		/* File header */
> @@ -168,7 +168,7 @@
>    pagemask = getpagesize () - 1;
>  
>    /* Adjust text/data boundary. */
> -  data_start = (int) start_of_data ();
> +  data_start = (int) (etext + 1);

I suggest to use DATA_START here (which is defined to '&etext + 1' by
msdos/sed2v2.inp).

Message #25 received at 13783-done <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 13783-done <at> debbugs.gnu.org
Subject: Re: simplify data_start configuration for Emacs
Date: Sun, 24 Feb 2013 21:57:01 -0800

On 02/23/2013 02:47 AM, Eli Zaretskii wrote:
> I suggest to use DATA_START here (which is defined to '&etext + 1' by
> msdos/sed2v2.inp).

Thanks, I did that and installed the fix as trunk bzr 111870.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.