DEFU* macro name for a extern DEFUN: DEFUE? DEFUNEX?

DEFU* macro name for a extern DEFUN: DEFUE? DEFUNEX?

[Paul Eggert]

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

I've been looking into trimming down the sets of symbols exported and
imported by each C source file in Emacs, down to the list of symbols
that are actually needed.  This should make it easier to read the
code, and to do better static analysis, and shrink Emacs a bit by
removing unused cruft.  The resulting analysis will need more thinking
and will comprise several patches (I don't know how many yet, as I've
only started turning the crank).

I have found several glitches in Emacs so far.  For example, font.c's
functions copy-font-spec and merge-font-spec are defined via DEFUN in
a strange way at the C level, so that they are not visible to Lisp.
It's been that way for nearly three years, so I assume the simplest
way to fix this is to rename the functions so that they don't look to
the C programmer as if they're visible to Lisp.  Other problems
include unreachable functions and unused variables.

There is a bit of technology needed to get this working, namely, we
need a way to mark Lisp-visible functions either static or external at
the C level.  Usually they're static, but often they need to be
external because other C modules call them directly.  To do this, I
changed DEFUN so that it generates static functions, and added a new
macro DEFUE that generates external functions (i.e., DEFUE acts like
the old DEFUN).  I chose the name DEFUE because the "E" stands for
"external", "DEFUE" is the same length as "DEFUN" (so that indenting
need not change), and "DEFUE" starts with "DEFU" (so that the
make-docfile need not change).  But if people would prefer another
identifier (DEFUNEX, say?) it'd be easy to change.

Here's the key part of the first patch:

--- src/lisp.h	2011-04-09 18:42:31 +0000
+++ src/lisp.h	2011-04-11 00:46:54 +0000
@@ -1804,8 +1804,12 @@
  `doc' is documentation for the user.  */

 /* This version of DEFUN declares a function prototype with the right
-   arguments, so we can catch errors with maxargs at compile-time.  */
-#define DEFUN(lname, fnname, sname, minargs, maxargs, intspec, doc)	\
+   arguments, so we can catch errors with maxargs at compile-time.
+   DEFUN defines an internal function, and DEFUE is similar but defines a
+   external function, which can be used in other C-language modules.  */
+#define DEFUN static DEFINE_FUNC
+#define DEFUE extern DEFINE_FUNC
+#define DEFINE_FUNC(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)),	\
@@ -2279,6 +2283,8 @@
    appropriate prototype.  */
 #define EXFUN(fnname, maxargs) \
   extern Lisp_Object fnname DEFUN_ARGS_ ## maxargs
+#define INFUN(fnname, maxargs) \
+  static Lisp_Object fnname DEFUN_ARGS_ ## maxargs

 /* Forward declarations for prototypes.  */
 struct window;


and here's an example use of the new DEFUE macro:

--- src/lread.c	2011-04-04 07:48:36 +0000
+++ src/lread.c	2011-04-11 00:41:52 +0000
@@ -681,7 +681,7 @@
   return val;
 }

-DEFUN ("read-char", Fread_char, Sread_char, 0, 3, 0,
+DEFUE ("read-char", Fread_char, Sread_char, 0, 3, 0,
        doc: /* Read a character from the command input (keyboard or macro).
 It is returned as a number.
 If the character has modifiers, they are resolved and reflected to the


I'm attaching the entire patch, gzipped, as most of it is pretty
mechanical substitution of DEFUE for DEFUN.  Comments, as usual, are
welcome.

[-- Attachment #2: patch-1.txt.gz --]
[-- Type: application/x-gzip, Size: 36874 bytes --]

Re: DEFU* macro name for a extern DEFUN: DEFUE? DEFUNEX?

[Dan Nicolaescu]

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

> I've been looking into trimming down the sets of symbols exported and
> imported by each C source file in Emacs, down to the list of symbols
> that are actually needed.  This should make it easier to read the
> code, and to do better static analysis, and shrink Emacs a bit by

Are you using gcc ?  If yes, and you are cleaning up the code to not
generate warnings with some -W flags, how about adding those flags to
the default set so that we do not regress?  (configure only sets very few -W
flags now, it could set more...)

> There is a bit of technology needed to get this working, namely, we
> need a way to mark Lisp-visible functions either static or external at
> the C level.  Usually they're static, but often they need to be
> external because other C modules call them directly.  To do this, I
> changed DEFUN so that it generates static functions, and added a new
> macro DEFUE that generates external functions (i.e., DEFUE acts like
> the old DEFUN).  I chose the name DEFUE because the "E" stands for
> "external", "DEFUE" is the same length as "DEFUN" (so that indenting
> need not change), and "DEFUE" starts with "DEFU" (so that the
> make-docfile need not change).  But if people would prefer another
> identifier (DEFUNEX, say?) it'd be easy to change.

How about the more explicit DEFUN_EXTERN and document DEFUN as
generating a static function?
The indentation changes should be minor, they would be one or two lines.


> --- src/lisp.h	2011-04-09 18:42:31 +0000
> +++ src/lisp.h	2011-04-11 00:46:54 +0000
> @@ -1804,8 +1804,12 @@
>   `doc' is documentation for the user.  */

>  #define EXFUN(fnname, maxargs) \
>    extern Lisp_Object fnname DEFUN_ARGS_ ## maxargs
> +#define INFUN(fnname, maxargs) \
> +  static Lisp_Object fnname DEFUN_ARGS_ ## maxargs

How about nothing instead of INFUN?
Explicit prototypes are much easier to read, and easier to figure out
the calling convention.
[IMHO we should also remove EXFUN, it might have been necessary before
using standard C, but it looks like just obfuscation now]

Re: DEFU* macro name for a extern DEFUN: DEFUE? DEFUNEX?

[Paul Eggert]

On 04/10/2011 11:27 PM, Dan Nicolaescu wrote:

> Are you using gcc ?  If yes, and you are cleaning up the code to not
> generate warnings with some -W flags, how about adding those flags to
> the default set so that we do not regress?  (configure only sets very few -W
> flags now, it could set more...)

Sure, I can do that.  However, I'm using quite a few flags, and they
are likely to cause problems during a normal "configure; make" if
they're made the default.  Even if a maintainer does a clean build on
(say) RHEL 5.6 it's likely that there will be lots of annoying
warnings when an installer runs on (say) Solaris.  What I plan to do
is what other GNU packages do in this situation, namely, add an
--enable-gcc-warnings option to "configure".  The default is no
warnings, but if you set the configure-time option you get lots of
nice warnings.

> How about the more explicit DEFUN_EXTERN and document DEFUN as
> generating a static function?

Sure, that'd be easy.

>> +#define INFUN(fnname, maxargs) \
>> +  static Lisp_Object fnname DEFUN_ARGS_ ## maxargs
> 
> How about nothing instead of INFUN?
> Explicit prototypes are much easier to read, and easier to figure out
> the calling convention.

Yes, that'd also be easy.

Re: DEFU* macro name for a extern DEFUN: DEFUE? DEFUNEX?

[Juanma Barranquero]

On Mon, Apr 11, 2011 at 08:27, Dan Nicolaescu <dann@gnu.org> wrote:

> How about the more explicit DEFUN_EXTERN and document DEFUN as
> generating a static function?

If the common case is non-static, it would be better IMHO to have
DEFUN_INTERNAL and DEFUN, or somesuch.

    Juanma

Re: DEFU* macro name for a extern DEFUN: DEFUE? DEFUNEX?

[Jason Rumney]

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

> There is a bit of technology needed to get this working, namely, we
> need a way to mark Lisp-visible functions either static or external at
> the C level.

Marking them static sounds like a recipe for future maintainence
headaches due to users' optimisation settings.  If they are referenced
outside of file scope, they are not static, whether the referencing code
is C or Lisp.

Re: DEFU* macro name for a extern DEFUN: DEFUE? DEFUNEX?

[Paul Eggert]

On 04/11/2011 03:39 AM, Juanma Barranquero wrote:
> If the common case is non-static, it would be better IMHO to have
> DEFUN_INTERNAL and DEFUN, or somesuch.

I agree, but the common case is static by far.
There are 809 static and 360 extern functions in my copy.

On 04/11/2011 07:34 AM, Jason Rumney wrote:
> Marking them static sounds like a recipe for future maintainence
> headaches due to users' optimisation settings.

I can't imagine what that headache would be.  Once a pointer
to a function escapes (from the C point of view), a C compiler
cannot optimize the function away.  Do you have a specific
scenario in mind?

> If they are referenced outside of file scope, they are not static,
> whether the referencing code is C or Lisp.

It's quite true that the functions are not static from the Lisp
point of view, and that C code in other modules can invoke
the functions by looking them up in the relevant tables;
but still, they are static from the C point of view and
knowing this can help people read the C code and do further
analysis.  (Doing this let me find the bug with copy-font-spec,
for example.)  I plan to discuss this a bit in the documentation
that it was suggested that I add.

PS.  I've been plowing ahead with more static analysis based on
changing 'extern' to 'static', and have found a bit more stuff
that's never used: encode_coding_gap, for example.  Some of the
stuff is used only on some platforms and can safely be removed
on platforms where it cannot be called: mark_backtrace, for example.

Re: DEFU* macro name for a extern DEFUN: DEFUE? DEFUNEX?

[Chong Yidong]

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

> I've been looking into trimming down the sets of symbols exported and
> imported by each C source file in Emacs, down to the list of symbols
> that are actually needed.  This should make it easier to read the
> code, and to do better static analysis, and shrink Emacs a bit by
> removing unused cruft.

I don't think this has been a problem for us in practice; there isn't
sufficient justification to replace DEFUNS throughout the tree.

Re: DEFU* macro name for a extern DEFUN: DEFUE? DEFUNEX?

[Paul Eggert]

On 04/11/2011 03:37 PM, Chong Yidong wrote:
> there isn't sufficient justification to replace DEFUNS throughout the tree

Hmm, well, now that I've gotten used to it, I would like for it
to continue to be easy to tell, when I'm reading the code, which
DEFUNs are called by other C modules.

How about this idea instead?  It would not replace any DEFUNs.
We put the line "static" in front of functions that are not exported.
For example:

static
DEFUN ("subrp", Fsubrp, Ssubrp, 1, 1, 0,
       doc: /* Return t if OBJECT is a built-in function.  */)
  (Lisp_Object object)
{
  if (SUBRP (object))
    return Qt;
  return Qnil;
}

This works too.  Would it be OK?  Other solutions are possible,
but this is the simplest I can think of, and it would be easy
for me to do.

Re: DEFU* macro name for a extern DEFUN: DEFUE? DEFUNEX?

[Stefan Monnier]

>> I've been looking into trimming down the sets of symbols exported and
>> imported by each C source file in Emacs, down to the list of symbols
>> that are actually needed.  This should make it easier to read the
>> code, and to do better static analysis, and shrink Emacs a bit by
>> removing unused cruft.

> I don't think this has been a problem for us in practice; there isn't
> sufficient justification to replace DEFUNS throughout the tree.

Agreed.  I could imagine distinguishing between "function only called
from Lisp" and "function both exported to Lisp and called from C", but
the static/non-static distinction would rarely if ever be of any help
for DEFUNs since those are always "called from some other file" anyway
(typically a Lisp file).


        Stefan

checking static vs extern symbols

[Paul Eggert]

No further comment, and it appears that there wasn't consensus
to mark DEFUNs as to whether they were static, so I removed that
part of the change.  Also, I removed the INFUN macro too, as
Dan Nicolaescu suggested.  What's was left, was changing several
C functions and variables to be 'static', plus removing some
code that was thereby exposed as being unused.  I merged this
to the trunk as bzr 103913.

I have an approximately 100-line shell script that reports
C variables and functions that are exported but shouldn't be.
It finds every extern symbol that is not used by any
other module, and isn't one of the exceptional cases (the
biggest exception being DEFUNs).  I plan to make this shell
script run as part of a new 'make maintainer-check' action.

I could call this shell script 'src/extern-check', or could
give it other names (in other directories) if people would prefer,
but the basic idea is that "make maintainer-check" is intended
to be used by maintainers who are running on platforms
that have the proper tools available, and that "make maintainer-check"
reports glitches such as unwanted exports.  (I have some
other maintainer checks in mind, but one step at a time.)

Re: DEFU* macro name for a extern DEFUN: DEFUE? DEFUNEX?

[Stefan Monnier]

> How about this idea instead?  It would not replace any DEFUNs.
> We put the line "static" in front of functions that are not exported.
> For example:

> static
> DEFUN ("subrp", Fsubrp, Ssubrp, 1, 1, 0,
>        doc: /* Return t if OBJECT is a built-in function.  */)
>   (Lisp_Object object)
> {
>   if (SUBRP (object))
>     return Qt;
>   return Qnil;
> }

> This works too.  Would it be OK?

No, this is going in the wrong direction.  What we want instead is to
extend make-docfile so that the DEFUNs, DEFSYMs (and any other similar
thing we may have) automatically create a global foo.h file which
declares all those objects, which are global anyway (because they're
global on the Lisp side).  I.e. we want to make all those C-and-Lisp
objects even less static than they are now.
We recently did that for DEFVARs and that was a very good change.


        Stefan

Re: checking static vs extern symbols

[Eli Zaretskii]

> Date: Thu, 14 Apr 2011 16:52:41 -0700
> From: Paul Eggert <eggert@cs.ucla.edu>
> 
> I have an approximately 100-line shell script that reports
> C variables and functions that are exported but shouldn't be.
> It finds every extern symbol that is not used by any
> other module, and isn't one of the exceptional cases (the
> biggest exception being DEFUNs).  I plan to make this shell
> script run as part of a new 'make maintainer-check' action.
> 
> I could call this shell script 'src/extern-check', or could
> give it other names (in other directories) if people would prefer

I think it should go somewhere under admin/.  That's where we put
stuff that is useful only for maintainers, such as admin.el.