Re: Patch for Libtool : partial linking of .lo objects

[Ralf Wildenhues]

Hi Marc,

* Marc Alff wrote on Fri, Apr 07, 2006 at 01:27:22PM CEST:
> 
> Well, I tried to see how to change libtool to address that, and I 
> eventually found what code to fix,
> so here is a proposed patch :

OK, here's a quick and not very thorough review.  And thanks for working
on this!

> The patch is based on Libtool-1.5.23a (CVS)

Better would be CVS HEAD, but this is still better than nothing.  :)

Comments inline.  Some are very nitpicky, so they are not really bad,
just to let you know how you could send even better patches.  :-)

> diff -Naur libtool/ChangeLog libtool-reloc/ChangeLog

ChangeLog entries are best posted plain, not as diff output, because at
the time the patch is applied, they will probably not apply cleanly any
more.  (Otherwise, the ChangeLog entry is remarkly well done for a first
try.  ;)

> diff -Naur libtool/ltmain.in libtool-reloc/ltmain.in
> --- libtool/ltmain.in 2006-04-07 10:17:59.000000000 +0000
> +++ libtool-reloc/ltmain.in   2006-04-07 10:55:44.000000000 +0000
> @@ -4292,7 +4292,7 @@
>        fi
>  
>        # Create the old-style object.
> -      reload_objs="$objs$old_deplibs "`$echo "X$libobjs" | $SP2NL | $Xsed -e 
> '/\.'${libext}$'/d' -e '/\.lib$/d' -e "$lo2o" | $NL2SP`" $reload_conv_objs" 
> ### testsuite: skip nested quoting test
> +      reload_objs="$objs$old_deplibs "`$echo "X$non_pic_objects" | $SP2NL | 
> $Xsed -e '/\.'${libext}$'/d' -e '/\.lib$/d' -e "$lo2o" | $NL2SP`" 
> $reload_conv_objs" ### testsuite: skip nested quoting test
>  
>        output="$obj"
>        cmds=$reload_cmds

This looks wrong (but I admit to not have tried it).
See below for explanation.

> @@ -4331,7 +4331,7 @@
>        if test -n "$pic_flag" || test "$pic_mode" != default; then
>       # Only do commands if we really have different PIC objects.
>       reload_objs="$libobjs $reload_conv_objs"
> -     output="$libobj"
> +     output="$objdir/$obj"
>       cmds=$reload_cmds
>       save_ifs="$IFS"; IFS='~'
>       for cmd in $cmds; do

This looks ok.

> @@ -4343,6 +4343,26 @@
>       IFS="$save_ifs"
>        fi
>  
> +    $run $rm "$libobj"
> +
> +    $show "creating libtool object $libobj"
> +
> +    # Create a libtool object file (analogous to a ".la" file),
> +    # but don't create it if we're doing a dry run.
> +    test -z "$run" && cat > ${libobj} <<EOF
> +# $libobj - a libtool object file
> +# Generated by $PROGRAM - GNU $PACKAGE $VERSION$TIMESTAMP
> +#
> +# Please DO NOT delete this file!
> +# It is necessary for linking the library.
> +
> +# Name of the PIC object.
> +pic_object='$objdir/$obj'
> +
> +# Name of the non-PIC object.
> +non_pic_object='$obj'
> +EOF
> +
>        if test -n "$gentop"; then
>       $show "${rm}r $gentop"
>       $run ${rm}r $gentop

This looks wrong.  In general, you cannot assume that either PIC objects
or non-PIC objects are built; merely that at least one of them is.  The
user can choose whether or not to disable shared or static libraries;
but note also that on AIX (in non-runtimelinking mode) by default static
libraries are not built, and only the `pic_objects' are built (the
shared libraries are named `*.a', and really are shared, albeit with a
bit uncommon semantics).  We set not-built object names to `none' in the
libtool object (.lo) file.

> +# link-reload.test - make sure that linking reloadable objects works.
> +# Written by Marc Alff
> +
> +# Test script header.
> +need_prefix=no
> +if test -z "$srcdir"; then
> +  srcdir=`echo "$0" | sed 's%/[^/]*$%%'`
> +  test "$srcdir" = "$0" && srcdir=.
> +  test "${VERBOSE+set}" != "set" && VERBOSE=yes
> +fi
> +
> +set -e
> +
> +. $srcdir/defs
> +
> +# Expected linker
> +$libtool --config | grep "^LD=" | sed -e "s/^LD=\"//g" -e "s/\"$//g" > 
> expected_ld

Better:
  eval `$libtool --config | grep "^LD="`
  $echo "$LD"

but you don't need this anyway; see below.

> +export expected_ld=`cat expected_ld`

  export variable=value
is not portable;
  variable=value
  export variable
is.

> +
> +# Expected linker flag
> +$libtool --config | grep "^reload_flag=" | sed -e "s/^reload_flag=\"//g" -e 
> "s/\"$//g" > expected_ld_flag
> +export expected_ld_flag=`cat expected_ld_flag`
> +
> +# Try a sample link command with non PIC objects
> +echo "Test 1"
> +$libtool -n --mode=link $CC -o all.o a.o b.o > linkresult

You can't assume object names are `*.o'.  Use $OBJEXT instead.

> +test $? -eq 0

You use `set -e' above, so no need to test here: the shell will stop on
non-zero return value.

> +echo "Actual result"
> +cat linkresult
> +echo "Expected result"
> +grep "$expected_ld$expected_ld_flag -o all\.o a\.o b\.o" linkresult

Let's not go this route (of comparing visual output).
It's very error-prone; link-order still doesn't work correctly because I
tried that there.

Much better instead is a test that tries expected functionality.
For example: Create two objects, partially link them, then create a
program that needs only the symbol(s) from one of the objects.
And then let us try to make sure the other symbol also ends up in the
program.  Or just the relinked-object, FWIW (because in final programs,
symbols may be stripped).

If you want to, feel free to work on such a test.  I'll otherwise do so,
but it'll be a bit until I can get to it.

Cheers,
Ralf