Re: Dynamic objects (was: .ONESEHLL not working as expected in 3.82)

[Paul Smith]

On Mon, 2013-04-29 at 19:33 +0300, Eli Zaretskii wrote:
> > From: Paul Smith <address@hidden>
> > Cc: address@hidden
> > Date: Sat, 27 Apr 2013 16:58:54 -0400
> > 
> > On Sat, 2013-04-27 at 23:00 +0300, Eli Zaretskii wrote:
> > > That would be nice, indeed.
> > 
> > OK, pushed.  You should be able to simply write a new load_objects()
> > function and drop it in.  Or put it into a w32 file or whatever.

> 1. Doesn't the FSF frown upon capability to load _any_ dynamic
>    objects?  I think they like the GCC method whereby each extension
>    is required to define a symbol with a certain name
>    (plugin_is_GPL_compatible in GCC), which is tested at load time,
>    before the dynamic object is allowed to be loaded.

Hm.  I guess the concern is that someone will introduce a proprietary
"plug-in"?  My position on this is that it would be a violation of the
GPL.  I don't believe there is any useful way to utilize this feature
without becoming a derived work of GNU make, since the only way to do
anything with this feature is to invoke GNU make functions.  As GNU make
is GPL'd there's no dynamic linking exception.  I'll check with the
legal folks.

> 2. The fact that the dynamic object's file extension (.so) is exposed
>    to the Makefile is unfortunate, because it will hurt portability of
>    Makefiles: the extension on Windows is .dll.  Can we omit the
>    extension and append it internally?

Yes, that should be possible.  My concern is that, at least on UNIX, the
rules for this are complex and I don't want to reimplement the runtime
linker :-).  Maybe something like, first try the path as given and if
that fails, try adding arch-specific extensions?

The other problem here is that we want to allow rebuilding of dynamic
objects then re-exec'ing make... if we're trying different extensions
THAT can be a real problem... what order do we do this in?

I'm not sure I can come up with a reliable algorithm for this that's
understandable.

> 3. I suggest to extend the search for dynamic object to a
>    Make-specific directory (${prefix}/share/make/), before falling
>    back to the "system-specific path".  Or maybe even not fall back to
>    any system-specific defaults, because those are generally set for
>    shared libraries, not for plugins.  (You do NOT want to know where
>    Windows will look for shared libraries.)

I'm not sure about having a make-specific directory.  It's not so easy
to do in UNIX--we'd have to modify the LD_LIBRARY_PATH env. var. I
suppose.  Also we don't really have a precedent of a "make-specific"
directory like that.

On UNIX there's no way to avoid looking in the system-specific locations
except by forcing the object path to contain a "/".  I suppose that if
the object didn't contain a "/" we could prefix it with "./" to force
the avoiding of system paths.  On the other hand we DO have precedence
for searching system paths; for example make's "include" will search for
included makefiles in places like /usr/include, /usr/local/include, etc.
even though I can't see how THAT makes sense.

> 4. It would be good to have at least a single simple example of a
>    dynamic extension, either in the tarball or in the manual.  The
>    only ones I found are in the test suite; did I miss something?

No.  The documentation does need to be enhanced.

> 5. Is the following a valid 'load' directive?
> 
>      load /foo/bar/
> 
>    If it is valid, what is its semantics?  If it is invalid, the code
>    in load_object should detect it and give a diagnostics; currently
>    it will happily use this, and will try to call a symbol _gmk_setup.

Hm.  That's odd.  It shouldn't try to call an init function unless the
load of the dynamic object succeeds, and I would think that trying to
dlopen("/foo/bar/") would fail.  I'll check it out.

> 6. The diagnostics in read.c:
> 
>               if (! load_file (&ebuf->floc, &name, noerror) && ! noerror)
>                 fatal (&ebuf->floc, _("%s: failed to load"), name);
> 
>    is IMO misleading: it says "failed to load" also in the case that
>    the dynamic object was successfully loaded, but the function called
>    from it returned zero.  It would be better to make a more precise
>    message in that case.

Yes, good point.

> 6. API:
> 
>    . I suggest to request that the buffers for expansions and
>      evaluation by gmk_expand and gmk_eval be provided by the caller.
>      It is not safe (and not very convenient) to return buffers
>      allocated internally by these functions, because the dynamic
>      object might be compiled/linked with an incompatible
>      implementation of memory allocation routines.

I don't see how this can work easily.  We have no idea how large the
buffer will be until after we complete the expansion/eval.  The only way
this could work is to use the snprintf() model where we do the expansion
as now, and if it's too small we return the expected length and they'd
have to re-invoke with a larger buffer.  However, the thing they've
expanded or, especially, eval'd could well have side-effects.  Almost
certainly WILL, in the case of eval.  Doing it twice would likely be
very Not Good.

I think we should go with this for now.  Hopefully users that do have a
problem will find a way to make this work.  If it becomes a real problem
we can introduce a new variation of these functions that work as
above... then users will have to work around THOSE issues instead.

Or use Tim's alternative and allow users to specify their own alloc/free
functions... that could be tricky, to ensure we use it only when doing
the user's work, and ensuring that we don't do any extra allocations
that we keep around afterward.  I guess since make is single-threaded,
and already encapsulates all its memory allocation with xmalloc() etc.,
it would be do-able.

>    . The manual says that the setup function should return non-zero on
>      success, but the code actually requires a non-negative value;
>      anything else prevents the dynamic object from being recorded in
>      .LOADED.

Right I'll fix this.