Re: module cleanup [1/n]

[Gary V. Vaughan]

Hi Eric,

First of all: Nice work! :-)

On Sep 6, 2007, at 11:56 PM, Eric Blake wrote:
> While investigating the possibility of tracing builtins regardless  
> of the name
> used to invoke them (or lack thereof, as in builtin(`divnum'))[1],  
> I noticed a
> number of (potential) issues with modules (not all of them fixable  
> in M4):
>
> - Right now, load(nosuchmodule) and unload(nosuchmodule) both exit the
> program.  Maybe they should give a warning, but continue processing  
> input?

Errors should be diagnosed as early as possible, and a script that  
loads a module
must do so for additional builtins.  Better to exit with a failed  
load early, than
to quietly copy text badly due to a missing builtin that causes  
something not to
expand.

Perhaps we could add an options parameter to load() like this?

   load(`nosuchmodule', `nocomplain')

> - The current module interface uses data exports for the builtin  
> and macro
> tables.  Libtool recommends using function exports instead of data  
> exports;
> changing this would be a change in the module interface, but might  
> make porting
> to obscure platforms easier

Agreed.  The module interface has evolved through several iterations  
in previous
alpha releases, and it is clearly marked as experimental in README  
and/or NEWS
IIRC, so further improvements at the expense of backward  
compatibility are quite
acceptable prior to release IMHO.

> - Based on the above point, it would be nice if we made ALL m4- 
> compatible
> modules export a function that advertised what version of m4 module  
> interface
> they comply with.  Absence of the version query function implies  
> interface
> version 1 (ie. the current version), where at least one of the  
> builtin table,
> macro table, init function, or finish function must exist.  Then,  
> we can do a
> better job about supporting new features in new modules, while  
> still being able
> to load old modules

Excellent idea.  Absence of the version query should imply  
unsupported alpha
module interface though.  Lets have version 1 be the version used by  
the first
official release with modules.

> - There is an open issue about freezing module-dependent state,  
> such as the m4
> module saving the current state of the sysval macro, which would  
> require a new
> API - having a module interface versioning scheme would make this  
> nicer to
> implement

ACK.

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

It certainly can't do any harm to be more careful about this.  HEAD  
libltdl does
assume that only one module loading extension point will be  
responsible for loading
of any module.  That is, if a module is written to be loaded by the  
m4 module loader,
then it shouldn't be loaded by a different module system (say the  
libsnprintfv
extension point -- if we reimplemented m4's format builtin using  
libsnprintfv).

> 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

Nice catch; I missed that one.

> - 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?

I'm loathe to keep adding builtins where we can extend existing  
builtins to get
the same functionality in a clean intuitive way.  m4modules is not  
yet bound to
provide backwards compatibility, so we could simply have it add newly  
loaded modules
to the list each time they are successfully loaded.  Membership tests  
would be
unaffected, and a macro to count occurences could easily work out the  
refcount of
a named module.

> - We aren't very robust to modules that change their mind  
> midstream.  It is
> currently possible for a stateful module to crash m4, by altering  
> the array
> that the exported builtin table points at based on what actions are  
> done via
> module builtins.  Also, I'm not sure how expensive lt_dlsym is, but  
> do know
> that system calls are more expensive than memory checks.  So rather  
> than re-
> query lt_dlsym every time we need to search the module for a  
> particular
> builtin, it would be more robust to copy and cache the builtin  
> table provided
> by the module when it was first loaded, then refer to the cache  
> thereafter

Agreed.

In the case of existing modules, they are preloaded, so lt_dlsym is  
actually
looking at the compiled in cache.  Your point is certainly valid for  
user loaded
modules.

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

Libltdl has APIs to attach data structures to a loaded module.  That  
may or may
not prove to be the easier place to hang things...

> - We currently expose the libltdl interface to all clients of  
> m4module.h.  That
> is poor interface design

ACK.

> - Error messages from libltdl are not translated

Patches to libtool gratefully accepted :-) (post 2.2!!)

Cheers,
        Gary
--
  ())_.              Email me: address@hidden
  ( '/           Read my blog: http://blog.azazil.net
  / )=         ...and my book: http://sources.redhat.com/autobook
`(_~)_      Join my AGLOCO Network: http://www.agloco.com/r/BBBS7912

PGP.sig
Description: This is a digitally signed message part