FYI: shlib improvements

[Jaroslav Hajek]

hi all,

the changesets
http://hg.savannah.gnu.org/hgweb/octave/rev/80432f0ee895
http://hg.savannah.gnu.org/hgweb/octave/rev/5f3c10ecb150

are a first attempt at improving Octave's handling of shared
libraries. Summary of changes:
octave_shlib is rewritten; although a lot of the old code is retained,
the structure is different. octave_xshlib is gone; octave_shlib no
longer serves as both the shared pointer container and shared object,
avoiding the extremely unsafe things like

  union
    {
      octave_shlib *rep;
      int count;
    };

instead, there's now octave_shlib::shlib_rep, just like with most
other classes  (Array, octave_value, idx_vector etc).
"open" and "close" methods now work locally, instead of globally, i.e.
`shl.close ()' won't actually close the library if there are other
shared copies.
Similarly, `shl.open (file)' is now equivalent to `shl = octave_shlib
(file)'. In this sense the octave_shlib now behave much more like the
other reference-counted objects.
Also, pointers acquired from octave_shlib::search are now safe to use
as long as you own a copy of the originating octave_shlib, without the
danger that the library will be closed from somewhere else.

The shlib class also maintains an internal map of filenames to
instances; when a file is being opened that already exists, the
existing instance is referenced instead. If the instance is outdated
(but still referenced!), a warning is printed, but the old instance is
returned *nevertheless*, to prevent having multiple (possibly
different) copies of the same library file in memory.
Although, e.g. Linux allows this, I think it's confusing because the
multiple instances won't share global data, so functions may easily
get out of sync. See also the example below.

When a library is found to be outdated, Octave will still attempt to
clear the existing functions that reference it; for this purpose, the
shlib object still maintains a set of "hook names", and this set is
shared amongst the copies. I left it there because I wanted a minimum
change. octave_shlib::mark_relative is gone, because apparently
octave_shlib::is_relative was not used anywhere; besides, I was not
sure whether the attribute should or should not be shared.

The following session using the triangular matrix type from
OctaveForge demonstrates the improvements (you must paste it to prompt
directly, split at the marked line, otherwise it won't demonstrate the
problem):

autoload ("istri", "uppertri.oct"); autoload ("lowertri", "uppertri.oct");
u = uppertri (rand (5))
istri (u)
l = lowertri (rand (5))
istri (l)
pause (5)
system ("touch uppertri.oct"); # cause trouble


# wait for prompt here, otherwise Octave won't notice the date above
istri (l)
istri (u)
u = uppertri (u)

with a recent tip, (OpenSUSE 10.3, Linux 2.6.25.20-0.5-default), it
sometimes works, sometimes crashes, sometimes produces the following
output:

octave:1> autoload ("istri", "uppertri.oct"); autoload ("lowertri",
"uppertri.oct");
warning: autoload: `uppertri.oct' is not an absolute file name
warning: autoload: `uppertri.oct' is not an absolute file name
octave:2> u = uppertri (rand (5))
u =

   0.34921   0.61019   0.79686   0.87073   0.15353
   0.00000   0.27759   0.29805   0.46740   0.26896
   0.00000   0.00000   0.83489   0.36557   0.52264
   0.00000   0.00000   0.00000   0.60199   0.92847
   0.00000   0.00000   0.00000   0.00000   0.85805
Upper Triangular

octave:3> istri (u)
ans =  1
octave:4> l = lowertri (rand (5))
l =

   0.00922   0.00000   0.00000   0.00000   0.00000
   0.54787   0.53847   0.00000   0.00000   0.00000
   0.88890   0.48315   0.94455   0.00000   0.00000
   0.95401   0.84670   0.49888   0.71387   0.00000
   0.12956   0.83618   0.21708   0.19430   0.93946
Lower Triangular

octave:5> istri (l)
ans =  1
octave:6> pause (5)
octave:7> system ("touch uppertri.oct"); # cause trouble
octave:8> # wait for prompt here, otherwise Octave won't notice the date above
octave:8> istri (l)
warning: reloading
/home/hajek/devel/octave-forge/extra/triangular/src/uppertri.oct
clears the following functions:
warning:   lowertri
warning:   uppertri
ans = 0
octave:9> istri (u)
ans = 0
octave:10> u = uppertri (u)
warning: duplicate unary operator `-' for type `triangular matrix'
warning: duplicate unary operator `+' for type `triangular matrix'
warning: duplicate unary operator `.'' for type `triangular matrix'
warning: duplicate unary operator `'' for type `triangular matrix'
warning: duplicate binary operator `\' for types `triangular matrix'
and `triangular matrix'
warning: duplicate assignment operator `=' for types `triangular
matrix' and `matrix'
warning: duplicate assignment operator `=' for types `triangular
matrix' and `triangular matrix'
warning: duplicate assignment operator `=' for types `triangular
matrix' and `scalar'
u =

   0.34921   0.61019   0.79686   0.87073   0.15353
   0.00000   0.27759   0.29805   0.46740   0.26896
   0.00000   0.00000   0.83489   0.36557   0.52264
   0.00000   0.00000   0.00000   0.60199   0.92847
   0.00000   0.00000   0.00000   0.00000   0.85805
Upper Triangular


What is happening probably wants an explanation (took me a while to
figure out, too). After out_of_date_check notices the file was
updated, a reload is attempted. During the reload, Octave attempts to
clear the autoloaded functions, but in reality it doesn't, because
they're mlocked. However, it then proceeds to dlclose the library and
dlopen it again. There are still pointers to the symbols from the old
handle, however; namely the virtual methods of the triangular class as
well as the functions uppertri and lowertri. At points 8 and 9, istri
returns false because istri was reloaded by out_of_date_check, and it
thinks that the type is not loaded, because it checks an internal flag
from the new copy.
At point 10, uppertri is referenced, found to be outdated and
reloaded. octave_triangular::register_type is called in the new shlib
copy. octave_value_typeinfo::register_type detects that the named type
was already registered, so it returns back the old typeid, so the new
class likewise has the old typeid. This results in the duplicate
operator registration warnings.

Here's what I get with the new patches:

octave:1> autoload ("istri", "uppertri.oct"); autoload ("lowertri",
"uppertri.oct");
warning: autoload: `uppertri.oct' is not an absolute file name
warning: autoload: `uppertri.oct' is not an absolute file name
octave:2> u = uppertri (rand (5))
u =

   0.89064   0.50240   0.36990   0.38869   0.62995
   0.00000   0.79093   0.06172   0.80979   0.91203
   0.00000   0.00000   0.70594   0.28386   0.69995
   0.00000   0.00000   0.00000   0.24535   0.12760
   0.00000   0.00000   0.00000   0.00000   0.77621
Upper Triangular

octave:3> istri (u)
ans =  1
octave:4> l = lowertri (rand (5))
l =

   0.31376   0.00000   0.00000   0.00000   0.00000
   0.44083   0.25953   0.00000   0.00000   0.00000
   0.56355   0.11286   0.85326   0.00000   0.00000
   0.03922   0.61700   0.08585   0.00748   0.00000
   0.18269   0.22508   0.50977   0.32411   0.79792
Lower Triangular

octave:5> istri (l)
ans =  1
octave:6> pause (5)
octave:7> system ("touch uppertri.oct"); # cause trouble
octave:8> # wait for prompt here, otherwise Octave won't notice the date above
octave:8> istri (l)
warning: reloading
/home/hajek/devel/octave-forge/extra/triangular/src/uppertri.oct
clears the following functions:
warning:   istri
warning:   lowertri
warning:   uppertri
warning: library
/home/hajek/devel/octave-forge/extra/triangular/src/uppertri.oct not
reloaded due to existing references
ans =

   0.31376   0.00000   0.00000   0.00000   0.00000
   0.00000   0.25953   0.00000   0.00000   0.00000
   0.00000   0.00000   0.85326   0.00000   0.00000
   0.00000   0.00000   0.00000   0.00748   0.00000
   0.00000   0.00000   0.00000   0.00000   0.79792
Upper Triangular

octave:9> istri (u)
ans =

   0.89064   0.50240   0.36990   0.38869   0.62995
   0.00000   0.79093   0.06172   0.80979   0.91203
   0.00000   0.00000   0.70594   0.28386   0.69995
   0.00000   0.00000   0.00000   0.24535   0.12760
   0.00000   0.00000   0.00000   0.00000   0.77621
Upper Triangular

octave:10> u = uppertri (u)
u =

   0.89064   0.50240   0.36990   0.38869   0.62995
   0.00000   0.79093   0.06172   0.80979   0.91203
   0.00000   0.00000   0.70594   0.28386   0.69995
   0.00000   0.00000   0.00000   0.24535   0.12760
   0.00000   0.00000   0.00000   0.00000   0.77621
Upper Triangular

octave:11>

up to point 8 the behavior is equal. At point 8, clearing the
functions is attempted but failed, because they're mlocked. The code
correctly determines that the old shlib is still referenced and does a
"fake reload", i.e. updates just the internal time stamp, so that
subsequent function references no longer attempt to reload the library
(unless it is touched again). A warning is printed about the fake
reload.

With this change, it will be safer to create permanent objects (in
particular octave_base_value subclasses) from DLD functions. All that
is needed is to get the copy of the octave_shlib and store it in a
safe place until objects are destroyed. The simplest option is to
simply make it a field of the derived class.
To retrieve the current shlib object, an user may either use
`get_current_shlib ()', or use an octave_auto_shlib object that does
it automatically. The latter can be conveniently used for deriving new
classes, as a field:

class
octave_my_type : public octave_base_value
{
   // .....
  private:
  // .....
    octave_auto_shlib shlib;
};

or as a parent:

class
octave_my_type : private octave_auto_shlib, public octave_base_value
{
   // .....
};

The latter method is in fact preferable because it most correctly
ensures that the shlib object is destroyed after everything else has
been done. However, people are often scared of multiple inheritance,
so the former should be safe unless some of the other fields also have
destructors defined by the shlib (in which case they should come
before the shlib field) or unless inherited destructors call virtual
methods (which is a bad idea anyway).

The new change introduced a little redundancy between the octave_shlib
class and octave_dynamic_loader singleton, because both classes look
up existing shlib instances by file name. For now, I tried to make a
minimal change, but in the future maybe octave_dynamic_loader could be
simplified.

Comments? Questions?
I hope compiled OO packages will adopt this style so that we'll see
less mysterious crashes at "clear all" and such.

best regards

PS. another minor fix related to the above is that autoloaded
functions are no longer reloaded at each call. This can be
demonstrated with the patch:

diff --git a/src/ov-dld-fcn.cc b/src/ov-dld-fcn.cc
--- a/src/ov-dld-fcn.cc
+++ b/src/ov-dld-fcn.cc
@@ -41,12 +41,14 @@
                                     "dynamically-linked function");


+#include <iostream>
 octave_dld_function::octave_dld_function
   (octave_builtin::fcn ff, const octave_shlib& shl,
    const std::string& nm, const std::string& ds)
   : octave_builtin (ff, nm, ds), sh_lib (shl)
 {
   mark_fcn_file_up_to_date (time_parsed ());
+  std::cerr << "DLD constructor " << nm << '\n';

   std::string file_name = fcn_file_name ();

and trying from the command line:

address@hidden:~/devel/octave/release-3-2-x> ./run-octave -q
DLD constructor find
DLD constructor cellfun
DLD constructor getpwuid
octave:1> min (1, 2);
DLD constructor min
octave:2> min (1, 2);
DLD constructor min
octave:3> min (1, 2);
DLD constructor min
octave:4> min (1, 2);
DLD constructor min


-- 
RNDr. Jaroslav Hajek
computing expert & GNU Octave developer
Aeronautical Research and Test Institute (VZLU)
Prague, Czech Republic
url: www.highegg.matfyz.cz