bug#8267: unexec FIXME in src/emacs.c

bug#8267: unexec FIXME in src/emacs.c

[Paul Eggert]

Severity: minor

There's a longstanding FIXME in src/emacs.c that gcc 4.5.2 is also
warning about.  I'd like to install the following to address the
problem.  This fix adds a file src/unexec.h, which contains just the
declaration for unexec.  Another plausible fix would be to put the
unexec declaration into lisp.h and have the unexec modules that don't
already include lisp.h, include lisp.h.  Either way would work fine,
but this way is a tad cleaner.

=== modified file 'src/ChangeLog'
--- src/ChangeLog	2011-03-16 21:47:59 +0000
+++ src/ChangeLog	2011-03-17 00:03:31 +0000
@@ -1,5 +1,14 @@
  2011-03-16  Paul Eggert  <eggert@cs.ucla.edu>
  
+	New file unexec.h, the (simple) interface for unexec.
+	* unexec.h: New file.
+	* deps.mk (emacs.o, unexaix.o, unexcw.o, unexcoff.o, unexelf.o):
+	(unexhp9k800.o, unexmacosx.o, unexsol.o, unexw32.o):
+	Depend on unexec.h.
+	* emacs.c [!defined CANNOT_DUMP]: Include unexec.h.
+	* unexaix.c, unexcoff.c, unexcw.c, unexelf.c, unexhp9k800.c:
+	* unexmacosx.c, unexsol.c, unexw32.c: Include unexec.h.
+
  	* syntax.c (Fforward_comment, scan_lists): Rename locals to avoid
  	shadowing.
  	(back_comment, skip_chars): Mark vars as initialized.

=== modified file 'src/deps.mk'
--- src/deps.mk	2011-03-13 06:43:00 +0000
+++ src/deps.mk	2011-03-16 23:59:54 +0000
@@ -93,7 +93,7 @@
  emacs.o: emacs.c commands.h systty.h syssignal.h blockinput.h process.h \
     termhooks.h buffer.h atimer.h systime.h $(INTERVALS_H) lisp.h $(config_h) \
     globals.h ../lib/unistd.h window.h dispextern.h keyboard.h keymap.h \
-   frame.h coding.h gnutls.h msdos.h
+   frame.h coding.h gnutls.h msdos.h unexec.h
  fileio.o: fileio.c window.h buffer.h systime.h $(INTERVALS_H) character.h \
     coding.h msdos.h blockinput.h atimer.h lisp.h $(config_h) frame.h \
     commands.h globals.h ../lib/unistd.h
@@ -200,15 +200,15 @@
  tparam.o: tparam.c tparam.h lisp.h $(config_h)
  undo.o: undo.c buffer.h commands.h window.h dispextern.h msdos.h \
     lisp.h globals.h $(config_h)
-unexaix.o: unexaix.c lisp.h $(config_h)
+unexaix.o: unexaix.c lisp.h unexec.h $(config_h)
  unexalpha.o: unexalpha.c $(config_h)
-unexcw.o: unexcw.c lisp.h $(config_h)
-unexcoff.o: unexcoff.c lisp.h $(config_h)
-unexelf.o: unexelf.c ../lib/unistd.h $(config_h)
-unexhp9k800.o: unexhp9k800.c $(config_h)
-unexmacosx.o: unexmacosx.c $(config_h)
-unexsol.o: unexsol.c lisp.h $(config_h)
-unexw32.o: unexw32.c $(config_h)
+unexcw.o: unexcw.c lisp.h unexec.h $(config_h)
+unexcoff.o: unexcoff.c lisp.h unexec.h $(config_h)
+unexelf.o: unexelf.c unexec.h ../lib/unistd.h $(config_h)
+unexhp9k800.o: unexhp9k800.c unexec.h $(config_h)
+unexmacosx.o: unexmacosx.c unexec.h $(config_h)
+unexsol.o: unexsol.c lisp.h unexec.h $(config_h)
+unexw32.o: unexw32.c unexec.h $(config_h)
  w16select.o: w16select.c dispextern.h frame.h blockinput.h atimer.h systime.h \
     msdos.h buffer.h charset.h coding.h composite.h lisp.h $(config_h)
  widget.o: widget.c xterm.h frame.h dispextern.h widgetprv.h \

=== modified file 'src/emacs.c'
--- src/emacs.c	2011-03-14 05:36:36 +0000
+++ src/emacs.c	2011-03-16 23:59:54 +0000
@@ -2085,9 +2085,7 @@
  \f
  #ifndef CANNOT_DUMP
  
-/* FIXME: maybe this should go into header file, config.h seems the
-   only one appropriate. */
-extern int unexec (const char *, const char *);
+#include "unexec.h"
  
  DEFUN ("dump-emacs", Fdump_emacs, Sdump_emacs, 2, 2, 0,
         doc: /* Dump current state of Emacs into executable file FILENAME.

=== modified file 'src/unexaix.c'
--- src/unexaix.c	2011-01-25 04:08:28 +0000
+++ src/unexaix.c	2011-03-16 23:59:54 +0000
@@ -40,6 +40,8 @@
   */
  
  #include <config.h>
+#include "unexec.h"
+
  #define PERROR(file) report_error (file, new)
  #include <a.out.h>
  /* Define getpagesize () if the system does not.

=== modified file 'src/unexcoff.c'
--- src/unexcoff.c	2011-01-25 04:08:28 +0000
+++ src/unexcoff.c	2011-03-16 23:59:54 +0000
@@ -50,6 +50,8 @@
   */
  
  #include <config.h>
+#include "unexec.h"
+
  #define PERROR(file) report_error (file, new)
  
  #ifndef CANNOT_DUMP  /* all rest of file!  */

=== modified file 'src/unexcw.c'
--- src/unexcw.c	2011-01-25 04:08:28 +0000
+++ src/unexcw.c	2011-03-16 23:59:54 +0000
@@ -19,6 +19,8 @@
  along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.  */
  
  #include <config.h>
+#include "unexec.h"
+
  #include <setjmp.h>
  #include <lisp.h>
  #include <stdio.h>
@@ -299,4 +301,3 @@
  
    return (0);
  }
-

=== added file 'src/unexec.h'
--- src/unexec.h	1970-01-01 00:00:00 +0000
+++ src/unexec.h	2011-03-17 00:03:31 +0000
@@ -0,0 +1,1 @@
+void unexec (const char *, const char *);

=== modified file 'src/unexelf.c'
--- src/unexelf.c	2011-01-25 04:08:28 +0000
+++ src/unexelf.c	2011-03-16 23:59:54 +0000
@@ -386,6 +386,8 @@
     Instead we read the whole file, modify it, and write it out.  */
  
  #include <config.h>
+#include <unexec.h>
+
  extern void fatal (const char *msgid, ...);
  
  #include <sys/types.h>

=== modified file 'src/unexhp9k800.c'
--- src/unexhp9k800.c	2011-01-15 23:16:57 +0000
+++ src/unexhp9k800.c	2011-03-16 23:59:54 +0000
@@ -50,6 +50,8 @@
  */
  \f
  #include <config.h>
+#include "unexec.h"
+
  #include <stdio.h>
  #include <fcntl.h>
  #include <errno.h>
@@ -319,4 +321,3 @@
  	  hdr->unloadable_sp_location, hdr->unloadable_sp_size);
  }
  #endif /* DEBUG */
-

=== modified file 'src/unexmacosx.c'
--- src/unexmacosx.c	2011-03-12 19:19:47 +0000
+++ src/unexmacosx.c	2011-03-16 23:59:54 +0000
@@ -95,6 +95,9 @@
  #undef malloc
  #undef realloc
  #undef free
+
+#include "unexec.h"
+
  #include <stdio.h>
  #include <fcntl.h>
  #include <stdarg.h>

=== modified file 'src/unexsol.c'
--- src/unexsol.c	2011-01-15 23:16:57 +0000
+++ src/unexsol.c	2011-03-16 23:59:54 +0000
@@ -1,6 +1,8 @@
  /* Trivial unexec for Solaris.  */
  
  #include <config.h>
+#include "unexec.h"
+
  #include <dlfcn.h>
  #include <setjmp.h>
  

=== modified file 'src/unexw32.c'
--- src/unexw32.c	2011-01-25 04:08:28 +0000
+++ src/unexw32.c	2011-03-16 23:59:54 +0000
@@ -21,6 +21,7 @@
  */
  
  #include <config.h>
+#include "unexec.h"
  
  #include <stdio.h>
  #include <fcntl.h>

bug#8267: Acknowledgement (unexec FIXME in src/emacs.c)

[Paul Eggert]

I've committed a patch for this in bzr103679.

bug#8267: unexec FIXME in src/emacs.c

[Andreas Schwab]

Paul Eggert <eggert@cs.ucla.edu> writes:

> === modified file 'src/ChangeLog'
> --- src/ChangeLog	2011-03-16 21:47:59 +0000
> +++ src/ChangeLog	2011-03-17 00:03:31 +0000
> @@ -1,5 +1,14 @@
>  2011-03-16  Paul Eggert  <eggert@cs.ucla.edu>
>  +	New file unexec.h, the (simple) interface for unexec.
> +	* unexec.h: New file.

When sending patches you should not use format=flowed.

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."

bug#8267: Acknowledgement (unexec FIXME in src/emacs.c)

[Ken Brown]

On 3/17/2011 12:54 PM, Paul Eggert wrote:
> I've committed a patch for this in bzr103679.

In unexec.h you declare unexec() to return void, but in several of the 
unexec*.c files it has a return type of int.

bug#8267: Acknowledgement (unexec FIXME in src/emacs.c)

[Paul Eggert]

On 03/17/2011 11:02 AM, Ken Brown wrote:
> On 3/17/2011 12:54 PM, Paul Eggert wrote:
>> I've committed a patch for this in bzr103679.
>
> In unexec.h you declare unexec() to return void, but in several of the unexec*.c files it has a return type of int.

Thanks for catching that.  I didn't notice that, since I built
on an implementation where it returns void.  I changed the
other implementations to match the new header (bzr 103680).

It might make sense for unexec to return an int, so that Emacs
can report an error (presumably from errno) if unexec fails,
but if this is done it should be done consistently on all
platforms.  The previous code was inconsistent.  The implementations
that returned an int sometimes returned 0 even when there were
problems.

bug#8267: Acknowledgement (unexec FIXME in src/emacs.c)

[Ken Brown]

On 3/17/2011 2:45 PM, Paul Eggert wrote:
> On 03/17/2011 11:02 AM, Ken Brown wrote:
>> On 3/17/2011 12:54 PM, Paul Eggert wrote:
>>> I've committed a patch for this in bzr103679.
>>
>> In unexec.h you declare unexec() to return void, but in several of the unexec*.c files it has a return type of int.
>
> Thanks for catching that.  I didn't notice that, since I built
> on an implementation where it returns void.  I changed the
> other implementations to match the new header (bzr 103680).

OK, but your change removes a lot of "returns" that shouldn't be 
removed.  I think the function should still return in those places; it's 
just that it shouldn't return a value.

bug#8267: Acknowledgement (unexec FIXME in src/emacs.c)

[Paul Eggert]

On 03/17/2011 12:01 PM, Ken Brown wrote:
> OK, but your change removes a lot of "returns" that shouldn't be removed.

Right you are again, and thanks again.
I restored the "return"s to unexaix.c, unexcoff.c, and unexcw.c
(bzr 103682).