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

[Marc Alff]

Hi Ralf and all,

Ralf Wildenhues wrote:

> Hi Marc,
>  
>
> > The patch is based on Libtool-1.5.23a (CVS)
> >    
>
> Better would be CVS HEAD, but this is still better than nothing.  :)
>
>  
1.5.22 is the last stable release as I understand (and happen to be the 
one I am using),
and this is technically a bug fix, so I guess it will have to go in 1.5 
anyway.

Ok, I am looking at HEAD now ... it took me a while to find WHERE the 
code was for libtool,
but I think I can come up with a patch as the code impacted is similar.
I will post here once it's ready, working on it.

If the fix for CVS HEAD has to be back ported in 1.5 later, some minor 
work will be needed
as the code base has evolved.

> Comments inline.  Some are very nitpicky, so they are not really bad,
> just to let you know how you could send even better patches.  :-)
>
>  
Thanks for taking the time to write those. It shows there might be hope 
after all :-)
I am just adding notes to points that needs comments, everything else is 
basically agreed to.

> > 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.
>
>  
It's deceiving taken out of context, try it.

I am not sure about the "$reload_conv_objs" part
(not to mention understanding **at all** what such a complicated line of 
code really does in plain English in the first place),
but the point of the change related to $libobjs --> $non_pic_objects is 
to have a partial link like this :

$LIBTOOL --mode=link $CC -o all.lo a.lo b.lo

produce non-PIC objects by linking non-PIC objects (which as I 
understand is the correct result)

ld -r -o all.o a.o b.o

as opposed produce an advertised "non-PIC" object made from PICs (which 
looks wrong to me)

ld -r -o all.o .libs/a.o .libs/b.o

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

Ok, you really, really confused me by quoting the whole paragraph and 
saying it looks wrong,
because it made me wonder if we need to generate a script or not.

I am reading it as follows then (correct me where needed) :

1)
we do need to generate a script for libtool objects (*.lo), which the 
patch does.

2)
for libtool objects, the **content** of the generated script looks wrong,
because (assuming what you really meant is this):

"In general, you cannot assume that **both** PIC objects **and** non-PIC 
objects are built".

I am reading "both" since the proposed patch did assume "both".

3)
So, with that in mind and reading the code again, it's making sense and 
I found that :

- Oops, there is a path where the generated script should contain 
pic_object=none, I will fix that.

- I have found a case with "non_pic_object=none" in compile mode,
but for the link mode, the current code always (i.e., un-conditionally) 
generates a non_pic_object as per the following :

      # Create the old-style object.
      reload_objs= [snip rated for language] ### testsuite: skip nested quoting 
test

      output="$obj"
      cmds=$reload_cmds

Not sure if it should stay that way or be protected by a "if static is 
not disabled" or something,
so that might be an existing bug in the code surfacing now.

Please confirm what the expected result should be, and i'll fix it.

> > +$libtool -n --mode=link $CC -o all.o a.o b.o > linkresult
> >    
>
> You can't assume object names are `*.o'.  Use $OBJEXT instead.
>
>  
Doh, and it's not the first time.
Somehow I still can't mentally picture that someone would want to pay 
for a broken system,
instead of using a free one that works, so I unconsciously (and 
repeatedly) discard the notion of $(OBJEXT).

For the benefit of :
g++ -c wizard.cpp -o wizard.oz
the next patch will account for that (if I don't mind blank again).

Now the real question : where do you get the value of $OBJEXT from in 
the test scripts,
and in the 1.5 branch in particular ? I am expecting to run a test stand 
alone when debugging ...


> > +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.
>
>  
I agree it's only a surface test, but hey, it still does more than what 
was there for partial linking :)

> 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).
>
>  
In theory, I totally agree with that approach.

In practice, I have some concerns with the feasability.

My very limited shell scripting abilities,
which boils down to writing commands at complicated as :
wc -l `which libtool`
tells me when looking at the result that writing portable shell script 
is a nightmare.

Looking at both branch-1-5 and CVS HEAD, in the tests directory,
I don't see any README or obvious template showing how to write a test,
so any pointers to a good one are welcome :)

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

I will try in CVS HEAD first, just for the sake of learning what 
autotest can do,
but it might take me some time to figure it out.

Also, note that more elaborated tests will be written, but as part of 
this obscure proposal
dealing with _RELOCATABLES in Automake :))

Cheers,
-- Marc.