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

[Gary V. Vaughan]

On Jun 23, 2007, at 5:27 AM, Ralf Wildenhues wrote:
> Hi Gary,

Hallo Ralf,

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

No problem, I figured as much and sent the ping to your radar :-)

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

Sure.

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

I hope you don't mean you want the patch to bitrot for another couple of
weeks before approving :-(

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

Sure.  I'll repost presently.

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

Done.

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

I don't recall.  Perhaps I was thinking about preloading static  
libraries.
Removed.

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

Sure, I've added a comment to HACKING to that effect too.

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

You talked me into it :-)

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

Since it's just debug code, and fprintf usually handles that  
automatically
it didn't seem worth the trouble.

> > +  /* 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 ;-).

That's cos Emacs on OS X sucks.  So I ditched it!

Maybe we should write a sed script that cleans this up?  I'll do it  
by hand
this time.

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

Absolutely our testsuite has less than full coverage.  Should we  
start to
record a TODO list of needed tests in CVS?

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