Stage 19a: sort and cache builtins loaded by a module.

* m4/m4module.h (m4_set_symbol_value_builtin): Delete.  Use
m4_builtin_find_by_* instead.
(m4_builtin_find_by_func): Change return type.
* m4/m4private.h (m4__builtin): New struct.
(m4_module): Add sorted list of loaded builtins.
(struct m4_symbol_value): Change type of builtin value.
(m4__set_symbol_value_builtin): New prototype and fast accessor.
(m4_get_symbol_value_func, m4_get_symbol_value_builtin): Adjust to
new field type.
* m4/symtab.c (m4_set_symbol_value_builtin): Rename...
(m4__set_symbol_value_builtin): ...and populate additional fields,
based on new type.
(m4_get_symbol_value_func, m4_get_symbol_value_builtin): Adjust to
new field type.
* m4/module.c (install_builtin_table): Use cached table.
(compare_builtin_CB): New helper function.
(m4__module_open): Populate table.
(module_remove): Free table.
* m4/builtin.c (compare_builtin_name_CB): New helper function.
(m4_builtin_find_by_name): Rewrite to use sorted table.
(m4_builtin_find_by_func): Change return type.
* m4/input.c (struct m4_input_block): Simplify u_b, since most
fields can be determined from builtin.
(builtin_peek, builtin_read, builtin_unget, init_builtin_token)
(m4__next_token): Alter parsing so that only init_builtin_token
consumes a builtin.
(builtin_print, m4_push_builtin): Adjust all users.
* tests/macros.at (Arity, defn, and freeze): Fix typo.

Signed-off-by: Eric Blake <ebb9@byu.net>

8 files changed, 199 insertions, 134 deletions

diff --git a/ChangeLog b/ChangeLog
index 1e1d8112..9c982413 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,41 @@
 2008-03-13  Eric Blake  <ebb9@byu.net>
 
+	Stage 19a: sort and cache builtins loaded by a module.
+	Rather than repeatedly using dlsym to browse the builtin table,
+	copy off the table at module load time.  Then, the input engine
+	merely refers to the copy instead of duplicating information.
+	Memory impact: slight penalty, due to more memory per module, but
+	offset by less memory in input engine.
+	Speed impact: slight improvement, due to faster builtin	lookups.
+	* m4/m4module.h (m4_set_symbol_value_builtin): Delete.  Use
+	m4_builtin_find_by_* instead.
+	(m4_builtin_find_by_func): Change return type.
+	* m4/m4private.h (m4__builtin): New struct.
+	(m4_module): Add sorted list of loaded builtins.
+	(struct m4_symbol_value): Change type of builtin value.
+	(m4__set_symbol_value_builtin): New prototype and fast accessor.
+	(m4_get_symbol_value_func, m4_get_symbol_value_builtin): Adjust to
+	new field type.
+	* m4/symtab.c (m4_set_symbol_value_builtin): Rename...
+	(m4__set_symbol_value_builtin): ...and populate additional fields,
+	based on new type.
+	(m4_get_symbol_value_func, m4_get_symbol_value_builtin): Adjust to
+	new field type.
+	* m4/module.c (install_builtin_table): Use cached table.
+	(compare_builtin_CB): New helper function.
+	(m4__module_open): Populate table.
+	(module_remove): Free table.
+	* m4/builtin.c (compare_builtin_name_CB): New helper function.
+	(m4_builtin_find_by_name): Rewrite to use sorted table.
+	(m4_builtin_find_by_func): Change return type.
+	* m4/input.c (struct m4_input_block): Simplify u_b, since most
+	fields can be determined from builtin.
+	(builtin_peek, builtin_read, builtin_unget, init_builtin_token)
+	(m4__next_token): Alter parsing so that only init_builtin_token
+	consumes a builtin.
+	(builtin_print, m4_push_builtin): Adjust all users.
+	* tests/macros.at (Arity, defn, and freeze): Fix typo.
+
 	Consistently cast malloc results, for C++ compilation.
 	* m4/builtin.c (m4_builtin_find_by_name): Add cast.
 	* m4/hash.c (m4_hash_new, m4_get_hash_iterator_next, node_new)
diff --git a/m4/builtin.c b/m4/builtin.c
index 9003b5b0..7f9d9578 100644
--- a/m4/builtin.c
+++ b/m4/builtin.c
@@ -25,6 +25,15 @@
 
 #include "m4private.h"
 
+/* Comparison function, for use in bsearch, which compares NAME
+   against the name of BUILTIN.  */
+static int
+compare_builtin_name_CB (const void *name, const void *b)
+{
+  const m4__builtin *builtin = (const m4__builtin *) b;
+  return strcmp ((const char *) name, builtin->builtin.name);
+}
+
 /* Find the builtin which has NAME.  If MODULE is not NULL, then
    search only in MODULE's builtin table.  The result is a malloc'd
    symbol value, suitable for use in the symbol table or for an
@@ -33,26 +42,17 @@ m4_symbol_value *
 m4_builtin_find_by_name (m4_module *module, const char *name)
 {
   m4_module *cur = module ? module : m4__module_next (NULL);
+  m4__builtin *bp;
 
   do
     {
-      const m4_builtin *builtin =
-	(m4_builtin *) lt_dlsym (cur->handle, BUILTIN_SYMBOL);
-
-      if (builtin)
+      bp = (m4__builtin *) bsearch (name, cur->builtins, cur->builtins_len,
+				    sizeof *bp, compare_builtin_name_CB);
+      if (bp)
 	{
-	  for (; builtin->name != NULL; builtin++)
-	    if (!strcmp (builtin->name, name))
-	      {
-		m4_symbol_value *token;
-		token = (m4_symbol_value *) xzalloc (sizeof *token);
-		m4_set_symbol_value_builtin (token, builtin);
-		VALUE_MODULE (token) = cur;
-		VALUE_FLAGS (token) = builtin->flags;
-		VALUE_MIN_ARGS (token) = builtin->min_args;
-		VALUE_MAX_ARGS (token) = builtin->max_args;
-		return token;
-	      }
+	  m4_symbol_value *token = (m4_symbol_value *) xzalloc (sizeof *token);
+	  m4__set_symbol_value_builtin (token, bp);
+	  return token;
 	}
     }
   while (!module && (cur = m4__module_next (cur)));
@@ -61,23 +61,25 @@ m4_builtin_find_by_name (m4_module *module, const char *name)
 }
 
 /* Find the builtin which has FUNC.  If MODULE argument is supplied
-   then search only in MODULE's builtin table.  */
-const m4_builtin *
+   then search only in MODULE's builtin table.  The result is a
+   malloc'd symbol value, suitable for use in the symbol table or for
+   an argument to m4_push_builtin.  */
+m4_symbol_value *
 m4_builtin_find_by_func (m4_module *module, m4_builtin_func *func)
 {
   m4_module *cur = module ? module : m4__module_next (NULL);
+  size_t i;
 
   do
     {
-      const m4_builtin *builtin =
-	(const m4_builtin *) lt_dlsym (cur->handle, BUILTIN_SYMBOL);
-
-      if (builtin)
-	{
-	  for (; builtin->name != NULL; builtin++)
-	    if (builtin->func == func)
-	      return builtin;
-	}
+      for (i = 0; i < cur->builtins_len; i++)
+	if (cur->builtins[i].builtin.func == func)
+	  {
+	    m4_symbol_value *token =
+	      (m4_symbol_value *) xzalloc (sizeof *token);
+	    m4__set_symbol_value_builtin (token, &cur->builtins[i]);
+	    return token;
+	  }
     }
   while (!module && (cur = m4__module_next (cur)));
 
diff --git a/m4/input.c b/m4/input.c
index 5ebf87f3..b7012286 100644
--- a/m4/input.c
+++ b/m4/input.c
@@ -180,15 +180,7 @@ struct m4_input_block
 	  bool_bitfield line_start : 1;	/* Saved start_of_input_line state.  */
 	}
       u_f;	/* See file_funcs.  */
-      struct
-	{
-	  const m4_builtin *builtin;	/* Pointer to builtin's function.  */
-	  m4_module *module;		/* Originating module.  */
-	  bool_bitfield read : 1;	/* True iff block has been read.  */
-	  int flags : 24;		/* Flags tied to the builtin. */
-	  m4_hash *arg_signature;	/* Argument signature for builtin.  */
-	}
-      u_b;	/* See builtin_funcs.  */
+      const m4__builtin *builtin;	/* A builtin, see builtin_funcs.  */
       struct
 	{
 	  m4__symbol_chain *chain;	/* Current link in chain.  */
@@ -397,42 +389,36 @@ static int
 builtin_peek (m4_input_block *me, m4 *context M4_GNUC_UNUSED,
 	      bool allow_argv M4_GNUC_UNUSED)
 {
-  if (me->u.u_b.read)
-    return CHAR_RETRY;
-
-  return CHAR_BUILTIN;
+  return me->u.builtin ? CHAR_BUILTIN : CHAR_RETRY;
 }
 
 static int
 builtin_read (m4_input_block *me, m4 *context M4_GNUC_UNUSED,
 	      bool allow_quote M4_GNUC_UNUSED, bool safe M4_GNUC_UNUSED)
 {
-  if (me->u.u_b.read)
-    return CHAR_RETRY;
-
-  me->u.u_b.read = true;
-  return CHAR_BUILTIN;
+  /* Not consumed here - wait until init_builtin_token.  */
+  return me->u.builtin ? CHAR_BUILTIN : CHAR_RETRY;
 }
 
 static void
 builtin_unget (m4_input_block *me, int ch)
 {
-  assert (ch == CHAR_BUILTIN && me->u.u_b.read);
-  me->u.u_b.read = false;
+  assert (ch == CHAR_BUILTIN && me->u.builtin);
 }
 
 static void
 builtin_print (m4_input_block *me, m4 *context, m4_obstack *obs)
 {
-  const m4_builtin *bp = me->u.u_b.builtin;
-  const char *text = bp->name;
+  const m4__builtin *bp = me->u.builtin;
+  const char *text = bp->builtin.name;
 
+  assert (bp);
   obstack_1grow (obs, '<');
   obstack_grow (obs, text, strlen (text));
   obstack_1grow (obs, '>');
   if (m4_is_debug_bit (context, M4_DEBUG_TRACE_MODULE))
     {
-      text = m4_get_module_name (me->u.u_b.module);
+      text = m4_get_module_name (bp->module);
       obstack_1grow (obs, '{');
       obstack_grow (obs, text, strlen (text));
       obstack_1grow (obs, '}');
@@ -461,17 +447,7 @@ m4_push_builtin (m4 *context, m4_symbol_value *token)
   i->funcs = &builtin_funcs;
   i->file = m4_get_current_file (context);
   i->line = m4_get_current_line (context);
-
-  i->u.u_b.builtin	= m4_get_symbol_value_builtin (token);
-  i->u.u_b.module	= VALUE_MODULE (token);
-  i->u.u_b.arg_signature = VALUE_ARG_SIGNATURE (token);
-  i->u.u_b.flags	= VALUE_FLAGS (token);
-  /* Check for bitfield truncation.  */
-  assert (i->u.u_b.flags == VALUE_FLAGS (token)
-	  && i->u.u_b.builtin->min_args == VALUE_MIN_ARGS (token)
-	  && i->u.u_b.builtin->max_args == VALUE_MAX_ARGS (token));
-  i->u.u_b.read		= false;
-
+  i->u.builtin = token->u.builtin;
   i->prev = isp;
   isp = i;
   input_change = true;
@@ -1079,20 +1055,18 @@ m4_pop_wrapup (m4 *context)
   return true;
 }
 
-/* When a BUILTIN token is seen, m4__next_token () uses init_builtin_token
-   to retrieve the value of the function pointer.  */
+/* Populate TOKEN with the builtin token at the top of the input
+   stack, then consume the input.  */
 static void
 init_builtin_token (m4 *context, m4_symbol_value *token)
 {
+  int ch = next_char (context, false, true);
   m4_input_block *block = isp;
-  assert (block->funcs->read_func == builtin_read && !block->u.u_b.read);
-
-  m4_set_symbol_value_builtin (token, block->u.u_b.builtin);
-  VALUE_MODULE (token)		= block->u.u_b.module;
-  VALUE_FLAGS (token)		= block->u.u_b.flags;
-  VALUE_ARG_SIGNATURE (token)	= block->u.u_b.arg_signature;
-  VALUE_MIN_ARGS (token)	= block->u.u_b.builtin->min_args;
-  VALUE_MAX_ARGS (token)	= block->u.u_b.builtin->max_args;
+  assert (ch == CHAR_BUILTIN && block->funcs->read_func == builtin_read
+	  && block->u.builtin);
+
+  m4__set_symbol_value_builtin (token, block->u.builtin);
+  block->u.builtin = NULL;
 }
 
 /* When a QUOTE token is seen, convert VALUE to a composite (if it is
@@ -1501,7 +1475,6 @@ m4__next_token (m4 *context, m4_symbol_value *token, int *line,
     if (ch == CHAR_BUILTIN)		/* BUILTIN TOKEN */
       {
 	init_builtin_token (context, token);
-	next_char (context, false, true);
 #ifdef DEBUG_INPUT
 	m4_print_token (context, "next_token", M4_TOKEN_MACDEF, token);
 #endif
diff --git a/m4/m4module.h b/m4/m4module.h
index c1f13608..05bc7af5 100644
--- a/m4/m4module.h
+++ b/m4/m4module.h
@@ -295,8 +295,6 @@ extern const char *	m4_get_symbol_value_placeholder	(m4_symbol_value *);
 extern void		m4_set_symbol_value_text  (m4_symbol_value *,
 						   const char *, size_t,
 						   unsigned int);
-extern void		m4_set_symbol_value_builtin	(m4_symbol_value *,
-							 const m4_builtin *);
 extern void		m4_set_symbol_value_placeholder	(m4_symbol_value *,
 							 const char *);
 
@@ -305,7 +303,7 @@ extern void		m4_set_symbol_value_placeholder	(m4_symbol_value *,
 /* --- BUILTIN MANAGEMENT --- */
 
 extern m4_symbol_value	*m4_builtin_find_by_name (m4_module *, const char *);
-extern const m4_builtin	*m4_builtin_find_by_func (m4_module *,
+extern m4_symbol_value	*m4_builtin_find_by_func (m4_module *,
 						  m4_builtin_func *);
 
 
diff --git a/m4/m4private.h b/m4/m4private.h
index 9e104412..c6cc639e 100644
--- a/m4/m4private.h
+++ b/m4/m4private.h
@@ -148,6 +148,22 @@ struct m4 {
    exported in m4module.h.  */
 #define m4__get_search_path(C)			((C)->search_path)
 
+
+/* --- BUILTIN MANAGEMENT --- */
+
+/* Internal representation of loaded builtins.  */
+struct m4__builtin
+{
+  /* Copied from module's BUILTIN_SYMBOL table, although builtin.flags
+     can be used for additional bits beyond what is allowed for
+     modules.  */
+  m4_builtin builtin;
+  m4_module *module;		/* Module that owns this builtin.  */
+};
+typedef struct m4__builtin m4__builtin;
+
+extern void m4__set_symbol_value_builtin (m4_symbol_value *,
+					  const m4__builtin *);
 
 
 /* --- MODULE MANAGEMENT --- */
@@ -158,15 +174,13 @@ struct m4 {
 #define INIT_SYMBOL		"m4_init_module"
 #define FINISH_SYMBOL		"m4_finish_module"
 
+/* Representation of a loaded m4 module.  */
 struct m4_module
 {
-  lt_dlhandle	handle;		/* ltdl module information.  */
-  int		refcount;	/* count of loads not matched by unload.  */
-  /* TODO: add struct members, such as copy of builtins (so that we
-     can store additional information about builtins, and so that the
-     list isn't changed by the module behind our backs once we have
-     initialized it) or cached pointers (to reduce the number of calls
-     needed to lt_dlsym).  */
+  lt_dlhandle handle;		/* All ltdl module information.  */
+  int refcount;			/* Count of loads not matched by unload.  */
+  m4__builtin *builtins;	/* Sorted array of builtins.  */
+  size_t builtins_len;		/* Number of builtins.  */
 };
 
 extern void	    m4__module_init (m4 *context);
@@ -248,11 +262,11 @@ struct m4_symbol_value
     {
       size_t		len;	/* Length of string.  */
       const char *	text;	/* String contents.  */
-      /* Quote age when this string was built, or zeroto force a
+      /* Quote age when this string was built, or zero to force a
 	 rescan of the string.  Ignored for 0 len.  */
       unsigned int	quote_age;
     } u_t;			/* Valid when type is TEXT, PLACEHOLDER.  */
-    const m4_builtin *	builtin;/* Valid when type is FUNC.  */
+    const m4__builtin *	builtin;/* Valid when type is FUNC.  */
     struct
     {
       m4__symbol_chain *chain;		/* First link of the chain.  */
@@ -348,8 +362,8 @@ extern void	m4__push_arg_quote	(m4 *, m4_obstack *, m4_macro_args *,
 #  define m4_get_symbol_value_text(V)	((V)->u.u_t.text)
 #  define m4_get_symbol_value_len(V)	((V)->u.u_t.len)
 #  define m4_get_symbol_value_quote_age(V)	((V)->u.u_t.quote_age)
-#  define m4_get_symbol_value_func(V)	((V)->u.builtin->func)
-#  define m4_get_symbol_value_builtin(V) ((V)->u.builtin)
+#  define m4_get_symbol_value_func(V)	((V)->u.builtin->builtin.func)
+#  define m4_get_symbol_value_builtin(V) (&(V)->u.builtin->builtin)
 #  define m4_get_symbol_value_placeholder(V)				\
 					((V)->u.u_t.text)
 #  define m4_symbol_value_groks_macro(V) (BIT_TEST ((V)->flags,		\
@@ -358,10 +372,14 @@ extern void	m4__push_arg_quote	(m4 *, m4_obstack *, m4_macro_args *,
 #  define m4_set_symbol_value_text(V, T, L, A)				\
   ((V)->type = M4_SYMBOL_TEXT, (V)->u.u_t.text = (T),			\
    (V)->u.u_t.len = (L), (V)->u.u_t.quote_age = (A))
-#  define m4_set_symbol_value_builtin(V, B)				\
-  ((V)->type = M4_SYMBOL_FUNC, (V)->u.builtin = (B))
 #  define m4_set_symbol_value_placeholder(V, T)				\
   ((V)->type = M4_SYMBOL_PLACEHOLDER, (V)->u.u_t.text = (T))
+#  define m4__set_symbol_value_builtin(V, B)				\
+  ((V)->type = M4_SYMBOL_FUNC, (V)->u.builtin = (B),			\
+   VALUE_MODULE (V) = (B)->module,					\
+   VALUE_FLAGS (V) = (B)->builtin.flags,				\
+   VALUE_MIN_ARGS (V) = (B)->builtin.min_args,				\
+   VALUE_MAX_ARGS (V) = (B)->builtin.max_args)
 #endif
 
 
diff --git a/m4/module.c b/m4/module.c
index 7ef6ab32..4b157005 100644
--- a/m4/module.c
+++ b/m4/module.c
@@ -1,6 +1,6 @@
 /* GNU m4 -- A simple macro processor
-   Copyright (C) 1989, 1990, 1991, 1992, 1993, 1994, 1998, 1999, 2002, 2003,
-   2004, 2005, 2006, 2007, 2008 Free Software Foundation, Inc.
+   Copyright (C) 1989, 1990, 1991, 1992, 1993, 1994, 1998, 1999, 2002,
+   2003, 2004, 2005, 2006, 2007, 2008 Free Software Foundation, Inc.
 
    This file is part of GNU M4.
 
@@ -135,47 +135,28 @@ m4_module_import (m4 *context, const char *module_name,
 static void
 install_builtin_table (m4 *context, m4_module *module)
 {
-  const m4_builtin *bp;
+  size_t i;
 
   assert (context);
   assert (module);
-
-  bp = (m4_builtin *) lt_dlsym (module->handle, BUILTIN_SYMBOL);
-  if (bp)
+  for (i = 0; i < module->builtins_len; i++)
     {
-      for (; bp->name != NULL; bp++)
-	{
-	  m4_symbol_value *value = m4_symbol_value_create ();
-	  const char *	   name;
+      m4_symbol_value *value = m4_symbol_value_create ();
+      const char *name = module->builtins[i].builtin.name;
 
-	  /* Sanity check that builtins meet the required interface.  */
-	  assert (bp->min_args <= bp->max_args);
-	  assert (bp->min_args > 0
-		  || (bp->flags & (M4_BUILTIN_BLIND
-				   | M4_BUILTIN_SIDE_EFFECT)) == 0);
-	  assert ((bp->flags & ~M4_BUILTIN_FLAGS_MASK) == 0);
-
-	  m4_set_symbol_value_builtin (value, bp);
-	  VALUE_MODULE   (value)	= module;
-	  VALUE_FLAGS    (value)	= bp->flags;
-	  VALUE_MIN_ARGS (value)	= bp->min_args;
-	  VALUE_MAX_ARGS (value)	= bp->max_args;
-
-	  if (m4_get_prefix_builtins_opt (context))
-	    name = xasprintf ("m4_%s", bp->name);
-	  else
-	    name = bp->name;
-
-	  m4_symbol_pushdef (M4SYMTAB, name, value);
-
-	  if (m4_get_prefix_builtins_opt (context))
-	    free ((char *) name);
-	}
+      m4__set_symbol_value_builtin (value, &module->builtins[i]);
+      if (m4_get_prefix_builtins_opt (context))
+	name = xasprintf ("m4_%s", name);
 
-      m4_debug_message (context, M4_DEBUG_TRACE_MODULE,
-			_("module %s: builtins loaded"),
-			m4_get_module_name (module));
+      m4_symbol_pushdef (M4SYMTAB, name, value);
+
+      if (m4_get_prefix_builtins_opt (context))
+	free ((char *) name);
     }
+  if (i)
+    m4_debug_message (context, M4_DEBUG_TRACE_MODULE,
+		      _("module %s: builtins loaded"),
+		      m4_get_module_name (module));
 }
 
 static void
@@ -199,7 +180,7 @@ install_macro_table (m4 *context, m4_module *module)
 	  assert (mp->min_args <= mp->max_args);
 
 	  m4_set_symbol_value_text (value, xmemdup (mp->value, len + 1),
-                                    len, 0);
+				    len, 0);
 	  VALUE_MODULE (value) = module;
 	  VALUE_MIN_ARGS (value) = mp->min_args;
 	  VALUE_MAX_ARGS (value) = mp->max_args;
@@ -390,6 +371,19 @@ m4__module_init (m4 *context)
 }
 
 
+/* Compare two builtins A and B for sorting, as in qsort.  */
+static int
+compare_builtin_CB (const void *a, const void *b)
+{
+  const m4__builtin *builtin_a = (const m4__builtin *) a;
+  const m4__builtin *builtin_b = (const m4__builtin *) b;
+  int result = strcmp (builtin_a->builtin.name, builtin_b->builtin.name);
+  /* A builtin module should never provide two builtins with the same
+     name.  */
+  assert (result || a == b);
+  return result;
+}
+
 /* Load a module.  NAME can be a absolute file name or, if relative,
    it is searched for in the module path.  The module is unloaded in
    case of error.  */
@@ -429,10 +423,44 @@ m4__module_open (m4 *context, const char *name, m4_obstack *obs)
 	{
 	  void *old;
 	  const char *err;
+	  const m4_builtin *bp;
 
 	  module = (m4_module *) xzalloc (sizeof *module);
 	  module->handle = handle;
 
+	  /* TODO - change module interface to return function pointer
+	     that supplies both table and length of table, rather than
+	     returning data pointer that must have a sentinel
+	     entry?  */
+	  bp = (m4_builtin *) lt_dlsym (module->handle, BUILTIN_SYMBOL);
+	  if (bp)
+	    {
+	      const m4_builtin *tmp;
+	      m4__builtin *builtin;
+	      for (tmp = bp; tmp->name; tmp++)
+		module->builtins_len++;
+	      module->builtins =
+		(m4__builtin *) xnmalloc (module->builtins_len,
+					  sizeof *module->builtins);
+	      for (builtin = module->builtins; bp->name != NULL;
+		   bp++, builtin++)
+		{
+		  /* Sanity check that builtins meet the required
+		     interface.  */
+		  assert (bp->min_args <= bp->max_args);
+		  assert (bp->min_args > 0
+			  || (bp->flags & (M4_BUILTIN_BLIND
+					   | M4_BUILTIN_SIDE_EFFECT)) == 0);
+		  assert ((bp->flags & ~M4_BUILTIN_FLAGS_MASK) == 0);
+
+		  memcpy (&builtin->builtin, bp, sizeof *bp);
+		  builtin->builtin.name = xstrdup (bp->name);
+		  builtin->module = module;
+		}
+	    }
+	  qsort (module->builtins, module->builtins_len,
+		 sizeof *module->builtins, compare_builtin_CB);
+
 	  /* clear out any stale errors, since we have to use
 	     lt_dlerror to distinguish between success and
 	     failure.  */
@@ -615,7 +643,13 @@ module_remove (m4 *context, m4_module *module, m4_obstack *obs)
     m4_error (context, EXIT_FAILURE, 0, NULL,
 	      _("cannot close module `%s': %s"), name, module_dlerror ());
   if (last_reference)
-    free (module);
+    {
+      size_t i;
+      for (i = 0; i < module->builtins_len; i++)
+	free ((char *) module->builtins[i].builtin.name);
+      free (module->builtins);
+      free (module);
+    }
 
   DELETE (name);
 
diff --git a/m4/symtab.c b/m4/symtab.c
index f8b84bfc..8b68132a 100644
--- a/m4/symtab.c
+++ b/m4/symtab.c
@@ -788,7 +788,7 @@ m4_builtin_func *
 m4_get_symbol_value_func (m4_symbol_value *value)
 {
   assert (value && value->type == M4_SYMBOL_FUNC);
-  return value->u.builtin->func;
+  return value->u.builtin->builtin.func;
 }
 
 #undef m4_get_symbol_value_builtin
@@ -796,7 +796,7 @@ const m4_builtin *
 m4_get_symbol_value_builtin (m4_symbol_value *value)
 {
   assert (value && value->type == M4_SYMBOL_FUNC);
-  return value->u.builtin;
+  return &value->u.builtin->builtin;
 }
 
 #undef m4_get_symbol_value_placeholder
@@ -823,15 +823,19 @@ m4_set_symbol_value_text (m4_symbol_value *value, const char *text, size_t len,
   value->u.u_t.quote_age = quote_age;
 }
 
-#undef m4_set_symbol_value_builtin
+#undef m4__set_symbol_value_builtin
 void
-m4_set_symbol_value_builtin (m4_symbol_value *value, const m4_builtin *builtin)
+m4__set_symbol_value_builtin (m4_symbol_value *value,
+			      const m4__builtin *builtin)
 {
-  assert (value);
-  assert (builtin);
+  assert (value && builtin);
 
-  value->type   = M4_SYMBOL_FUNC;
+  value->type = M4_SYMBOL_FUNC;
   value->u.builtin = builtin;
+  VALUE_MODULE (value) = builtin->module;
+  VALUE_FLAGS (value) = builtin->builtin.flags;
+  VALUE_MIN_ARGS (value) = builtin->builtin.min_args;
+  VALUE_MAX_ARGS (value) = builtin->builtin.max_args;
 }
 
 #undef m4_set_symbol_value_placeholder
diff --git a/tests/macros.at b/tests/macros.at
index d0dd7e13..c4db9e60 100644
--- a/tests/macros.at
+++ b/tests/macros.at
@@ -82,7 +82,7 @@ AT_DATA([[input.m4]],
 [[defun
 defun(`foo')
 defun(`foo', `bar')
-defun(`foo', `bar', baz')
+defun(`foo', `bar', `baz')
 ]])
 
 AT_DATA([[expout]],