Re: module cleanup [3/n]

[Eric Blake]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

According to Eric Blake on 9/6/2007 4:56 PM:
> - Module ref-counting is screwy.  The current unload code assumes that 
> builtins 
> associated with a module must only be unloaded once the module refcount is at 
> one and will be dropping to 0.  But that's a bit presumptuous, since the 
> libltdl ref-count could include 3rd party loads of the same module that 
> should 
> have no impact to our symbol table.  Also, we weren't decreasing the 
> ref-count 
> of resident modules, which meant that symbols from such a module weren't 
> always 
> being removed from the symbol table at the right time
> 
> - Speaking of refcounts, the m4modules builtin only shows a module once, even 
> if it has been loaded multiple times.  Maybe it is worth adding another 
> builtin 
> to the load module that can query the current refcount of a given module name?

This adds a refcount builtin to the load module.  I still think it is a
bit odd that importing helper symbols from an m4 library affects the
symbol table, but didn't see an easy way to fix it as part of this patch.

$ m4 -m load
refcount(`m4')
1
unload(`m4')refcount(`m4') dnl no longer in table
0 dnl no longer in table

esyscmd(`echo')


refcount(`m4') dnl esyscmd imports from m4 module, reloading macros
1

> 
> - If we add symbol caching, then it becomes trivial to store information 
> associated with a builtin (back to my original thought of adding the ability 
> to 
> trace a builtin regardless of the name it is invoked by); without a cache, 
> there is no convenient place to do per-builtin tracking since we can't 
> necessarily write into the memory owned by a module

Not quite symbol caching, but I did speed up the lt_dlinterface_id filter.
 The idea is that if we have ever associated a non-NULL m4_module* with a
given lt_dlhandle, then we have previously determined that the handle
meets our needs, so we need not waste time with further lt_dlsym calls to
check that point.

2007-09-07  Eric Blake  <address@hidden>

        Add refcount builtin.
        * modules/load.c (refcount): New builtin.
        (m4modules): Use correct type.
        * doc/m4.texinfo (Refcount): New section.
        * m4/m4private.h (struct m4_module): Add refcount member.
        (m4_module_refcount) [NDEBUG]: Add faster accessor macro.
        * m4/module.c (m4_module_load): Add symbols to table on first
        load by m4, regardless of other libltdl loads of same module.
        (m4_module_refcount): Use new struct member, rather than relying
        on libltdl count which might be inflated by unrelated loads.
        (m4__module_interface): Optimize.
        (m4__module_next, m4__module_find): Avoid assertions that could
        trigger with unrelated libltdl loads.
        (m4__module_open): Track m4 load count.
        (m4__module_exit): Only unload what m4 loaded.
        (module_remove): Track m4 unloads.
        * NEWS: Document new builtin.

- --
Don't work too hard, make some time for fun as well!

Eric Blake             address@hidden
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.5 (Cygwin)
Comment: Public key at home.comcast.net/~ericblake/eblake.gpg
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFG4dLg84KuGfSFAYARApPPAJ9MXSlUPyEb/6SxnTGOVuXnxq/PlACgiH4v
Muf3tqf36yBNW/QGLkAS/zo=
=7Z1Q
-----END PGP SIGNATURE-----

Index: NEWS
===================================================================
RCS file: /sources/m4/m4/NEWS,v
retrieving revision 1.44
diff -u -p -r1.44 NEWS
--- NEWS        10 Jul 2007 20:48:07 -0000      1.44
+++ NEWS        7 Sep 2007 22:32:34 -0000
@@ -114,6 +114,9 @@ promoted to 2.0.
 *** New `mkdtemp' builtin parallels `mkstemp', but allows the creation of
     temporary directories instead of files.
 
+*** New `refcount' builtin allows tracking how many times a module has
+    been loaded.
+
 *** New `renamesyms' builtin allows programmatic renaming of all symbols
     according to a regular expression.
   - FIXME: This feature can cause core dumps when renaming multiple
Index: doc/m4.texinfo
===================================================================
RCS file: /sources/m4/m4/doc/m4.texinfo,v
retrieving revision 1.110
diff -u -p -r1.110 m4.texinfo
--- doc/m4.texinfo      10 Aug 2007 15:09:46 -0000      1.110
+++ doc/m4.texinfo      7 Sep 2007 22:32:35 -0000
@@ -236,6 +236,7 @@ Extending M4 with dynamic runtime module
 * M4modules::                   Listing loaded modules
 * Load::                        Loading additional modules
 * Unload::                      Removing loaded modules
+* Refcount::                    Tracking module references
 * Standard Modules::            Standard bundled modules
 
 Macros for text handling
@@ -5485,6 +5486,7 @@ two modes of startup.
 * M4modules::                   Listing loaded modules
 * Load::                        Loading additional modules
 * Unload::                      Removing loaded modules
+* Refcount::                    Tracking module references
 * Standard Modules::            Standard bundled modules
 @end menu
 
@@ -5542,7 +5544,7 @@ m4modules
 @section Removing loaded modules
 
 @deffn {Builtin (load)} unload (@var{module-name})
-Any loaded modules that can be listed by the @code{modules} macro can be
+Any loaded modules that can be listed by the @code{m4modules} macro can be
 removed by naming them as the @var{module-name} parameter of the
 @code{unload} macro.  Unloading a module consists of removing all of the
 macros it provides from the internal table of visible macros, and
@@ -5562,6 +5564,49 @@ m4modules
 @result{}load,gnu,m4
 @end example
 
address@hidden Refcount
address@hidden Tracking module references
+
address@hidden {Builtin (load)} refcount (@var{module-name})
+This macro expands to an integer representing the number of times
address@hidden has been loaded but not yet unloaded.  No warning is
+issued, even if @var{module-name} does not represent a valid module.
+
+The macro @code{refcount} is recognized only with parameters.
address@hidden deffn
+
+This example demonstrates tracking the reference count of the gnu
+module.
+
address@hidden options: -m load
address@hidden
+$ @kbd{m4 -m load}
+m4modules
address@hidden,gnu,m4
+refcount(`gnu')
address@hidden
+m4modules
address@hidden,gnu,m4
+load(`gnu')
address@hidden
+refcount(`gnu')
address@hidden
+unload(`gnu')
address@hidden
+m4modules
address@hidden,gnu,m4
+refcount(`gnu')
address@hidden
+unload(`gnu')
address@hidden
+m4modules
address@hidden,m4
+refcount(`gnu')
address@hidden
+refcount(`NoSuchModule')
address@hidden
address@hidden example
+
 @node Standard Modules
 @section Standard bundled modules
 
Index: m4/m4private.h
===================================================================
RCS file: /sources/m4/m4/m4/m4private.h,v
retrieving revision 1.83
diff -u -p -r1.83 m4private.h
--- m4/m4private.h      6 Sep 2007 22:58:26 -0000       1.83
+++ m4/m4private.h      7 Sep 2007 22:32:35 -0000
@@ -147,6 +147,7 @@ struct m4 {
 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
@@ -161,6 +162,11 @@ extern void            m4__module_exit (m4 *con
 extern m4_module *  m4__module_next (m4_module *);
 extern m4_module *  m4__module_find (const char *name);
 
+/* Fast macro versions of symbol table accessor functions, that also
+   have an identically named function exported in m4module.h.  */
+#ifdef NDEBUG
+# define m4_module_refcount(M) ((M)->refcount)
+#endif
 
 
 /* --- SYMBOL TABLE MANAGEMENT --- */
Index: m4/module.c
===================================================================
RCS file: /sources/m4/m4/m4/module.c,v
retrieving revision 1.53
diff -u -p -r1.53 module.c
--- m4/module.c 6 Sep 2007 22:58:26 -0000       1.53
+++ m4/module.c 7 Sep 2007 22:32:35 -0000
@@ -84,8 +84,6 @@
 static const char*  module_dlerror (void);
 static int         module_remove  (m4 *context, m4_module *handle,
                                    m4_obstack *obs);
-//static void      module_close   (m4 *context, m4_module *handle,
-//                                 m4_obstack *obs);
 
 static void        install_builtin_table (m4*, m4_module *);
 static void        install_macro_table   (m4*, m4_module *);
@@ -104,7 +102,7 @@ m4_get_module_name (const m4_module *m4_
 
   info = lt_dlgetinfo (m4_handle->handle);
 
-  return info ? info->name : 0;
+  return info ? info->name : NULL;
 }
 
 void *
@@ -212,26 +210,10 @@ m4_module_load (m4 *context, const char 
 {
   m4_module *handle = m4__module_open (context, name, obs);
 
-  if (handle)
+  if (handle && handle->refcount == 1)
     {
-      const lt_dlinfo  *info   = lt_dlgetinfo (handle->handle);
-
-      if (!info)
-       {
-         /* If name is not set we are getting a reflective handle, but we
-            need to display an error message so we set an appropriate
-            value here.  */
-         if (!name)
-           name = MODULE_SELF_NAME;
-
-         m4_error (context, 0, 0, _("cannot load module `%s': %s"),
-                   name, module_dlerror ());
-       }
-      else if (info->ref_count == 1)
-       {
-         install_builtin_table (context, handle);
-         install_macro_table (context, handle);
-       }
+      install_builtin_table (context, handle);
+      install_macro_table (context, handle);
     }
 
   return handle;
@@ -246,22 +228,6 @@ m4_module_makeresident (m4_module *m4_ha
   return lt_dlmakeresident (m4_handle->handle) ? module_dlerror () : NULL;
 }
 
-/* Return the current refcount, or times that module HANDLE has been
-   opened.  */
-int
-m4_module_refcount (const m4_module *handle)
-{
-  /* FIXME - we should track refcounts in struct m4_module, since it
-     is conceivable that the m4 load refcount could be different than
-     the libltdl refcount, if a single module can be loaded both by
-     m4_module_load and as a dependent of some other module.  */
-  const lt_dlinfo *info;
-  assert (handle);
-  info = lt_dlgetinfo (handle->handle);
-  assert (info);
-  return info->ref_count;
-}
-
 /* Unload a module.  */
 void
 m4_module_unload (m4 *context, const char *name, m4_obstack *obs)
@@ -297,6 +263,13 @@ m4_module_unload (m4 *context, const cha
 static int
 m4__module_interface (lt_dlhandle handle, const char *id_string)
 {
+  /* Shortcut.  If we've already associated our wrapper with this
+     handle, then we've validated the handle in the past, and don't
+     need to waste any time on additional lt_dlsym calls.  */
+  m4_module *m4_handle = (m4_module *) lt_dlcaller_get_data (iface_id, handle);
+  if (m4_handle)
+    return 0;
+
   /* A valid m4 module must provide at least one of these symbols.  */
   return !(lt_dlsym (handle, INIT_SYMBOL)
           || lt_dlsym (handle, FINISH_SYMBOL)
@@ -321,15 +294,9 @@ m4__module_next (m4_module *m4_handle)
       if (!handle)
        return NULL;
       m4_handle = (m4_module *) lt_dlcaller_get_data (iface_id, handle);
-      if (m4_handle)
-       assert (m4_handle->handle == handle);
-      else
-       {
-         assert (lt_dlisresident (handle));
-         assert (lt_dlgetinfo (handle)->ref_count <= 1);
-       }
     }
   while (!m4_handle);
+  assert (m4_handle->handle == handle);
   return m4_handle;
 }
 
@@ -348,11 +315,6 @@ m4__module_find (const char *name)
   m4_handle = (m4_module *) lt_dlcaller_get_data (iface_id, handle);
   if (m4_handle)
     assert (m4_handle->handle == handle);
-  else
-    {
-      assert (lt_dlisresident (handle));
-      assert (lt_dlgetinfo (handle)->ref_count <= 1);
-    }
   return m4_handle;
 }
 
@@ -440,6 +402,14 @@ m4__module_open (m4 *context, const char
       /* If we have a handle, there must be handle info.  */
       assert (info);
 
+#ifdef DEBUG_MODULES
+      if (info->ref_count > 1)
+       {
+         fprintf (stderr, "module %s: now has %d libtool references.",
+                  name, info->ref_count);
+       }
+#endif /* DEBUG_MODULES */
+
       m4_debug_message (context, M4_DEBUG_TRACE_MODULE,
                        _("module %s: opening file `%s'"),
                        name ? name : MODULE_SELF_NAME, info->filename);
@@ -452,7 +422,6 @@ m4__module_open (m4 *context, const char
          void *old;
          const char *err;
 
-         assert (info->ref_count == 1);
          m4_handle = (m4_module *) xzalloc (sizeof *m4_handle);
          m4_handle->handle = handle;
 
@@ -470,6 +439,7 @@ m4__module_open (m4 *context, const char
 
       /* Find and run any initializing function in the opened module,
         each time the module is opened.  */
+      m4_handle->refcount++;
       init_func = (m4_module_init_func *) lt_dlsym (handle, INIT_SYMBOL);
       if (init_func)
        {
@@ -507,15 +477,14 @@ m4__module_exit (m4 *context)
 
   while (handle && !errors)
     {
-      const lt_dlinfo *info    = lt_dlgetinfo (handle->handle);
       m4_module *      pending = handle;
 
       /* If we are about to unload the final reference, move on to the
         next handle before we unload the current one.  */
-      if (info->ref_count <= 1)
+      if (pending->refcount <= 1)
        handle = m4__module_next (handle);
 
-      errors = module_remove (context, pending, 0);
+      errors = module_remove (context, pending, NULL);
     }
 
   assert (iface_id);           /* need to have called m4__module_init */
@@ -576,12 +545,12 @@ module_remove (m4 *context, m4_module *m
 #ifdef DEBUG_MODULES
   if (info->ref_count > 1)
     {
-      fprintf (stderr, "module %s: now has %d references.",
+      fprintf (stderr, "module %s: now has %d libtool references.",
               name, info->ref_count - 1);
     }
 #endif /* DEBUG_MODULES */
 
-  if (info->ref_count == 1)
+  if (m4_handle->refcount-- == 1)
     {
       /* Remove the table references only when ref_count is *exactly*
         equal to 1.  If module_close is called again on a
@@ -644,3 +613,21 @@ module_remove (m4 *context, m4_module *m
 
   return errors;
 }
+
+
+/* Below here are the accessor functions behind fast macros.  Declare
+   them last, so the rest of the file can use the macros.  */
+
+/* Return the current refcount, or times that module HANDLE has been
+   opened.  */
+#undef m4_module_refcount
+int
+m4_module_refcount (const m4_module *handle)
+{
+  const lt_dlinfo *info;
+  assert (handle);
+  info = lt_dlgetinfo (handle->handle);
+  assert (info);
+  assert (handle->refcount <= info->ref_count);
+  return handle->refcount;
+}
Index: modules/load.c
===================================================================
RCS file: /sources/m4/m4/modules/load.c,v
retrieving revision 1.25
diff -u -p -r1.25 load.c
--- modules/load.c      6 Sep 2007 22:58:26 -0000       1.25
+++ modules/load.c      7 Sep 2007 22:32:35 -0000
@@ -34,8 +34,9 @@
 
           function     macros  blind   side    minargs maxargs */
 #define builtin_functions                                      \
-  BUILTIN (m4modules,  false,  false,  false,  0,      0  )    \
   BUILTIN (load,       false,  true,   false,  1,      1  )    \
+  BUILTIN (m4modules,  false,  false,  false,  0,      0  )    \
+  BUILTIN (refcount,   false,  true,   false,  1,      1  )    \
   BUILTIN (unload,     false,  true,   false,  1,      1  )    \
 
 
@@ -94,7 +95,7 @@ M4BUILTIN_HANDLER (m4modules)
 {
   /* The expansion of this builtin is a comma separated list of
      loaded modules.  */
-  lt_dlhandle handle = m4__module_next (NULL);
+  m4_module *handle = m4__module_next (NULL);
 
   if (handle)
     do
@@ -108,6 +109,15 @@ M4BUILTIN_HANDLER (m4modules)
 }
 
 /**
+ * refcount(module)
+ **/
+M4BUILTIN_HANDLER (refcount)
+{
+  m4_module *handle = m4__module_find (M4ARG (1));
+  m4_shipout_int (obs, handle ? m4_module_refcount (handle) : 0);
+}
+
+/**
  * load(MODULE-NAME)
  **/
 M4BUILTIN_HANDLER (load)