Re: 330-gary-ltdl-vs-need-lib-prefix-unknown

[Ralf Wildenhues]

Hi Gary,

* Gary V. Vaughan wrote on Thu, Jun 21, 2007 at 04:06:25AM CEST:
>>>> Anyway, your new test is failing on i686-pc-linux-gnu, log attached.
>>>
>>> Hmmm... thats pretty weird!  It works for me, and the log you sent me 
>>> seems as though the test program runs fine, but doesn't produce any
>>> output.  Could you use gdb to figure out why the moduletest function
>>> isn't printing anything?
>>
>> Ping?
>
> Ping?

Oh, this went off my radar, with all the other stuff.

Then, my testing back then was, to say the least, subterranean: I
had an empty value of global_symbol_to_c_name_address_lib_prefix stuck
in my config.cache file and tested without removing that.  This caused
of course all kinds of weird followup behavior, as no symbols were put
into the symlists file.

Please accept my apologies for both.

I've retested now.  Things seem to work well for me, on GNU/Linux.
I do have a BeOS installation on my old, dusty PC, but it will be at
least a couple of weeks until I have enough time to actually try to
build and test Libtool HEAD on it.

Some minor nits in what is a partial review so far, slightly reordered:

> --- /dev/null
> +++ libtool--devo--0/tests/need_lib_prefix.at
[...]

> +AT_DATA([main.c],
> +[[#include <ltdl.h>
> +#include <stdio.h>
> +
> +typedef int funcp (int);

Is that shorthand for "function pointer"?  If yes, then I'd either drop
the p here or make it a pointer:
  typedef int (*funcp) (int);

and adjust the code below using funcp.  Just for clarity.

> +AT_SETUP([enforced lib prefix])
> +AT_KEYWORDS([libltdl])
[...]
> +# Create our own libtool, forcing need_lib_prefix setting
> +sed 's,^\(need_lib_prefix\)=.*$,\1=unknown,' $LIBTOOL > ./libtool
> +LIBTOOL="$SHELL ./libtool"

I think you should still be able to also add the `libtool' keyword to
this test, so that it is run with low max_cmd_len, too.  (Key is here
that tests/cmdline_wrap.at modifies $LIBTOOL but does not put $SHELL
in it, so above should still work.

We should document the libltdl keyword in HACKING.

> +AT_CHECK([$LIBTOOL --mode=link $CC -module -avoid-version $CFLAGS $LDFLAGS \
> +          -o foo1.la foo1.lo -rpath /foo], [], [ignore], [ignore])
> +AT_CHECK([$LIBTOOL --mode=link $CC -module -avoid-version $CFLAGS $LDFLAGS \
> +          -o libfoo2.la foo2.lo -rpath /foo], [], [ignore], [ignore])
> +AT_CHECK([$LIBTOOL --mode=link $CC -static $CFLAGS $LDFLAGS -o main \
> +          main.$OBJEXT -dlpreopen foo1.la -dlpreopen libfoo2.la $LIBLTDL],
> +          [], [ignore], [ignore])

Why is that -static doing in there?  Over here it seems to work fine
without.  Also, can we please put the link command lines (the first
arguments to AT_CHECK here) on one line so that 'testsuite -x' displays
their exact value?  It makes debugging-by-cut-n-paste so much easier.

Should install tests be tried, too (I mean installing the libs and the
program and running the installed program)?

> +LT_AT_NOINST_EXEC_CHECK([./main], [-dlopen foo1.la -dlopen libfoo2.la],
> +           [], [expout], [])


> --- libtool--devo--0.orig/libltdl/config/ltmain.m4sh
> +++ libtool--devo--0/libltdl/config/ltmain.m4sh
> @@ -978,8 +978,14 @@ lt_${my_prefix}_LTX_preloaded_symbols[] 
>  {\
>    { \"$my_originator\", (void *) 0 },"
>  
> -       eval "$global_symbol_to_c_name_address" < "$nlist" >> 
> "$output_objdir/$my_dlsyms"
> -
> +       case $need_lib_prefix in
> +       no)
> +         eval "$global_symbol_to_c_name_address" < "$nlist" >> 
> "$output_objdir/$my_dlsyms"
> +         ;;
> +       *)
> +         eval "$global_symbol_to_c_name_address_lib_prefix" < "$nlist" >> 
> "$output_objdir/$my_dlsyms"
> +         ;;
> +       esac

It strikes me as a bit odd that ltmain should need any change at all.
Clean would be to just set global_symbol_to_c_name_address to the value
of $global_symbol_to_c_name_address_lib_prefix if the system needed it,
no?  Of course there are two points associated with this:
- it would make the simulated test more difficult,
- currently configure code is ordered/organized wrongly for this to work.

Reorganizing code is out of the question ATM.  OTOH we should not lure
people into global_symbol_to_c_name_address_lib_prefix being a stable
interface (so a patch documenting it at the end of libtool.texi should
mentions its volatility).

Maybe global_symbol_to_c_name_address can be generalized to do the work
in both cases... (I guess we can postpone addressing this, as it's prone
to shell quoting portability issues again), similar to how, say,
$reload_cmds carries expandable values in it, but with something like
\$lib_prefix.  Oh well, I've tried this for 5 minutes and seen that it's
more than just a teensy bit of work.  Also, if we change the value of
$global_symbol_to_c_name_address at all that invalidates all existing
cache files, and if we change its quote escaping then even more.

Hmm.  Maybe leave it as it is now is the easiest thing, however unclean
it is.

[...]
> --- libtool--devo--0.orig/libltdl/m4/libtool.m4
> +++ libtool--devo--0/libltdl/m4/libtool.m4
> @@ -3216,6 +3216,7 @@ lt_cv_sys_global_symbol_to_cdecl="sed -n
>  
>  # Transform an extracted symbol line into symbol name and symbol address
>  lt_cv_sys_global_symbol_to_c_name_address="sed -n -e 's/^: \([[^ ]]*\) $/  
> {\\\"\1\\\", (void *) 0},/p' -e 's/^$symcode* \([[^ ]]*\) \([[^ ]]*\)$/  
> {\"\2\", (void *) \&\2},/p'"
> +lt_cv_sys_global_symbol_to_c_name_address_lib_prefix="sed -n -e 's/^: \([[^ 
> ]]*\) $/  {\\\"\1\\\", (void *) 0},/p' -e 's/^$symcode* \([[^ ]]*\) \(lib[[^ 
> ]]*\)$/  {\"\2\", (void *) \&\2},/p' -e 's/^$symcode* \([[^ ]]*\) \([[^ 
> ]]*\)$/  {\"lib\2\", (void *) \&\2},/p'"


> Index: libtool--devo--0/libltdl/ltdl.c
> ===================================================================
> --- libtool--devo--0.orig/libltdl/ltdl.c
> +++ libtool--devo--0/libltdl/ltdl.c
> @@ -120,7 +120,8 @@ static    int     try_dlopen            (lt_dlhandle
>                                      lt_dladvise advise);
>  static       int     tryall_dlopen         (lt_dlhandle *handle,
>                                      const char *filename,
> -                                    lt_dladvise advise);
> +                                    lt_dladvise advise,
> +                                    const lt_dlvtable *vtable);
>  static       int     unload_deplibs        (lt_dlhandle handle);
>  static       lt__advise *advise_dup        (lt__advise *advise);
>  static       int     lt_argz_insert        (char **pargz, size_t *pargz_len,
[...]

> @@ -332,17 +337,22 @@ lt_dlexit (void)
>  }
>  
>  
> -/* Try all dlloaders for FILENAME.  If the library is not successfully
> -   loaded, return non-zero.  Otherwise, the dlhandle is stored at the
> -   address given in PHANDLE.  */
> +/* Try VTABLE or, if VTABLE is NULL, all available loaders for FILENAME.
> +   If the library is not successfully loaded, return non-zero.  Otherwise,
> +   the dlhandle is stored at the address given in PHANDLE.  */
>  static int
>  tryall_dlopen (lt_dlhandle *phandle, const char *filename,
> -            lt_dladvise advise)
> +            lt_dladvise advise, const lt_dlvtable *vtable)
>  {
>    lt__handle *       handle          = (lt__handle *) handles;
>    const char *       saved_error     = 0;
>    int                errors          = 0;
>  
> +#ifdef LT_DEBUG_LOADERS
> +  fprintf (stderr, "tryall_dlopen (%s, %s)\n", filename,
> +        vtable ? vtable->name : "(ALL)");

Need to test filename == NULL and output "(null)" in that case.

> +#endif
> +
>    LT__GETERROR (saved_error);
>  
>    /* check whether the module was already opened */
> @@ -391,19 +401,31 @@ tryall_dlopen (lt_dlhandle *phandle, con
>      }
>  
>    {
> -    const lt_dlvtable *vtable = 0;
> -    lt_dlloader *loader = 0;
> +    lt_dlloader loader = lt_dlloader_next (0);
> +    const lt_dlvtable *loader_vtable;
>  
> -    while ((loader = (lt_dlloader *) lt_dlloader_next (loader)))
> +    do
>        {
>       lt__advise *advise_taken = 0;
>  
>       if (advise)
>         advise_taken = advise_dup (advise);
>  
> -     vtable = lt_dlloader_get (loader);
> -     handle->module = (*vtable->module_open) (vtable->dlloader_data,
> -                                              filename, advise_taken);
> +     if (vtable)
> +       loader_vtable = vtable;
> +     else
> +       loader_vtable = lt_dlloader_get (loader);
> +
> +#ifdef LT_DEBUG_LOADERS
> +     fprintf (stderr, "Calling %s->module_open (%s)\n",
> +              loader_vtable->name, filename);

Need to check filename != NULL.

> +#endif
> +     handle->module = (*loader_vtable->module_open) 
> (loader_vtable->dlloader_data,
> +                                                     filename, advise_taken);
> +#ifdef LT_DEBUG_LOADERS
> +     fprintf (stderr, "  Result: %s\n",
> +              handle->module ? "Success" : "Failed");
> +#endif
>  
>       if (handle->module != 0)
>         {
> @@ -1107,6 +1134,10 @@ try_dlopen (lt_dlhandle *phandle, const 
>    assert (phandle);
>    assert (*phandle == 0);
>  
> +#ifdef LT_DEBUG_LOADERS
> +  fprintf (stderr, "try_dlopen (%s, %s)\n", filename, ext);

Need to test both filename and ext for NULL.

> +#endif
> +
>    LT__GETERROR (saved_error);
>  
>    /* dlopen self? */
[...]
> @@ -1210,6 +1241,35 @@ try_dlopen (lt_dlhandle *phandle, const 
>      name[ext - base_name] = LT_EOS_CHAR;
>    }
>  
> +  /* Before trawling through the filesystem in search of a module,
> +     check whether we are opening a preloaded module.  */
> +  if (!dir)
> +    {
> +      const lt_dlvtable *vtable      = lt_dlloader_find ("lt_preopen");
> +
> +      if (vtable)
> +     {
> +               *phandle = (lt_dlhandle) lt__zalloc (sizeof (lt__handle));

Inconsistent TAB vs. space indentation (yeah I'm nitpicky ;-).

> +
> +       if (*phandle == NULL)
> +         {
> +           ++errors;
> +           goto cleanup;
> +         }
> +          newhandle = *phandle;
> +
> +          if (tryall_dlopen (&newhandle, filename, advise, vtable) == 0)
> +         {
> +           goto register_handle;
> +         }
> +
> +       /* If we're still here, there was no matching preloaded module,
> +          so put things back as we found them, and continue searching.  */
> +       FREE (*phandle);
> +       newhandle = NULL;
> +        }
> +    }
> +
>    /* Check whether we are opening a libtool module (.la extension).  */
>    if (ext && streq (ext, archive_ext))
>      {

I haven't been able to review the rest of the changes in
ltdl.c:tryall_dlopen and try_dlopen yet, sorry; the other changes in
your patch look good.  Judging from the number of different bits, I
wonder whether they bugs they fix are covered well enough in our
testsuite, though?

Cheers,
Ralf