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

[Ralf Wildenhues]

Hi Marc, 

* Marc Alff wrote on Sat, Apr 08, 2006 at 02:17:21PM CEST:
> Ralf Wildenhues wrote:
> >
> >>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.

Probably, depending upon impact.  But producing a patch against
branch-1-5 is fine, we can port it. 

> Ok, I am looking at HEAD now ... it took me a while to find WHERE the 
> code was for libtool,

Sorry about that; it's libltdl/config/ltmain.m4sh.
And an Autotest addition (tests/*.at) would be good.
But again, we can do that if you don't. 

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

(Still pending) 

> I am not sure about the "$reload_conv_objs" part

Yes, I think they are necessary.  To find out, do this in your test:
create a convenience archive and link that into the reloadable object.
Like so:
echo 'int conv() { return 0;}' > conv.c
$libtool --mode=compile $CC $CPPFLAGS $CFLAGS -c conv.c
$libtool --mode=link $CC $CFLAGS $LDFLAGS -o libconv.la conv.lo 

$libtool --mode=link $CC $CFLAGS $LDFLAGS -o reloaded.lo libconv.la
(you can add more objects here). 

> (not to mention understanding **at all** what such a complicated line of 
> code really does in plain English in the first place),

| reload_objs= 

assign to the shell variable reload_objs the concatenation of 

| "$objs$old_deplibs " 

the variables objs and old_deplibs, and a space, and 

| `$echo "X$libobjs" | $SP2NL | $Xsed -e '/\.'${libext}$'/d' -e '/\.lib$/d' 
-e "$lo2o" | $NL2SP` 

some weird expression to be explained below, and 

| " $reload_conv_objs" 

another space, and the value of reload_conv_objs. 

Weird expression:  `command` is replaced by the output of command
(this is called command substitution; see a shell manual).  The command is: 

| $echo "X$libobjs" | 

Output X and the value of libobjs, then transform it 

| $SP2NL | 

by changing spaces to newlines, then 

| $Xsed -e '/\.'${libext}$'/d' -e '/\.lib$/d' -e "$lo2o" | 

killing the X in the first line, and removing lines matching the
basic regular expressions
\.a$
\.lib$ 

and then changing all .lo into .o, and then 

| $NL2SP 

transforming newlines back to spaces. 

> 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

Correct.  (Except that if you create a non-PIC object as output, and no
non-PIC objects as input are available, then you need to pick the PIC
ones.) 

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

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

Correct. 

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

Wrong.  I meant `either'.  You cannot be sure that PIC objects will be
built.  You cannot be sure that non-PIC objects will be built.  Only
thing you can be sure of is that at least one of them will be built. 

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

Correct.  But that's why the patch is wrong. 

> 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 show how to reproduce this (and I'll be able to say whether that's
wrong or not). 

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

Oh, when compiling (-c), you can't even be sure the compiler accepts -o
at all.  And don't forget $EXEEXT :) 

> Now the real question : where do you get the value of $OBJEXT from in 
> the test scripts,

It should be exported from tests/Makefile.am. 

> and in the 1.5 branch in particular ? I am expecting to run a test stand 
> alone when debugging ...

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

:)  Then it's fine if you describe what you want, and we'll write a test. 

> 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 :)

Look at existing tests.. sorry, there isn't much better right now. 

Cheers,
Ralf