Re: make libtool faster v1

[Ralf Wildenhues]

* Gary V. Vaughan wrote on Mon, Nov 29, 2004 at 02:11:30PM CET:
> Ralf Wildenhues wrote:
> > So, to get to my point:  Would the others accept speedup patches?
> 
> Except where they obfuscate the code.

Sure.  But Noah's patch abstracting the functionality makes this one
easy to to, IMHO.

> > For now, it has a
> > drawback: it depends on the user not using stdin (e.g., in conjunction
> > with a `-' argument somewhere).  Do we support this mode?
> 
> Nothing is documented, and I can't think why anyone would want to
> feed objects to libtool on stdin.  It's probably worth a NEWS entry
> though, incase someone has found a way to talk to libtool on stdin.

Sure.

> > The patch below is against HEAD, though I'd really like to backport to
> > branch-2-0 and branch-1-5 as well.
> 
> branch-1-5 is stable at the moment, so only bug fixes can go there.

Actually, I'd go as far as calling a 40% speed increase a bugfix, but
that may only be me (and maybe the gcc folks).

> branch-2-0 is not quite stable yet, but I suspect there will be
> shouting if we perturb the code enough to force retesting... well,
> we need to retest everywhere when the outstanding bugs are fixed
> anyway, so backporting to branch-2-0 isn't necessarily out of the
> question.

Good.

> > +func_lalib_unsafe_p ()
> > +{
> > +    lalib_p=no
> > +    if test -r "$1" && exec <"$1"; then
> 
> I'd be amazed if this exec has any portability problems.
> 
> > +   for lalib_p_l in 1 2 3 4
> > +   do
> > +       read lalib_p_line
> > +       case "$lalib_p_line" in
> > +           \#\ Generated\ by\ *$PACKAGE* ) lalib_p=yes; break;;
> > +       esac
> > +   done
> > +   exec <&-
> 
> The old syntax for this was simply 'exec <&', but I'm not sure
> which (if any) might cause problems with some shells.

This syntax is mentioned in Steve Bourne's shell tutorial[1].  I'd be
truly amazed if there are shells having problems with this.

>    However,
> since we are saying that stdin is not used by libtool, then it
> should be safe to drop this line anyway.  Maybe for safety it
> would be best to use exec</dev/null?  Whichever, a big comment
> explaining what the optimisation is, and how it works is definitely
> needed.

Sure.  See the comment before the function implementation in updated
patch at the end of this mail.

> > -   func_lalib_p "$file" \ ||
> > +   func_lalib_unsafe_p "$file" \ ||
> >       func_fatal_help "\`$lib' is not a valid libtool archive"
> 
>         func_lalib_unsafe_p "$file" || \
>            func_fatal_help "\`$lib' is not a valid libtool archive"

Whoops, yes.

> (my comments about linesplitting around || and && notwithstanding)

Also overlooked, sorry.

Err, you do realise there are almost no places in libtool shell code
with your linesplitting and almost hundred the other way round?  I
don't care much which style we choose, just wanted to mention
existing practise to the contrary.

> The rest looks fine to me.

Great.

OK to commit this to HEAD?

Regards,
Ralf

[1] See http://steve-parker.org/sh/bourne.shtml for a HTML transcript.

2004-11-29  Ralf Wildenhues <address@hidden>

        * config/ltmain.m4sh (func_lalib_p): Update function
        description. (func_lalib_unsafe_p): New function with same
        functionality but written without forks; this function is safe
        to use for cases where the argument either does not exist or
        is required to be a lalib for correct operation.
        * NEWS: Mention that stdin is not to be used.
        * (func_mode_execute, func_mode_install, func_mode_link):
        Use func_lalib_unsafe_p where appropriate.
        * (func_mode_execute): For the program wrapper, use
        func_ltwrapper_p instead of func_lalib_p.


Index: NEWS
===================================================================
RCS file: /cvsroot/libtool/libtool/NEWS,v
retrieving revision 1.172
diff -u -r1.172 NEWS
--- NEWS        24 Nov 2004 17:25:38 -0000      1.172
+++ NEWS        29 Nov 2004 16:24:36 -0000
@@ -3,6 +3,7 @@
 New in 2.1b: 2005-??-??; CVS version 2.1a, Libtool team:
 * New tests for support of Automake subdir-objects.
 * Support for Portland Group compiler on Linux.
+* Shell optimizations which break use of the stdin file descriptor in libtool.
 
 New in 1.9h: 2004-??-??; CVS version 1.9g, Libtool team:
 * Libtool versions can now be parallel installed, except that only one
Index: config/ltmain.m4sh
===================================================================
RCS file: /cvsroot/libtool/libtool/config/ltmain.m4sh,v
retrieving revision 1.27
diff -u -r1.27 ltmain.m4sh
--- config/ltmain.m4sh  28 Nov 2004 19:55:40 -0000      1.27
+++ config/ltmain.m4sh  29 Nov 2004 16:24:36 -0000
@@ -574,7 +574,7 @@
 
 
 # func_lalib_p file
-# True iff FILE is a libtool `.la' library.
+# True iff FILE is a libtool `.la' library or `.lo' object file.
 # This function is only a basic sanity check; it will hardly flush out
 # determined imposters.
 func_lalib_p ()
@@ -583,6 +583,28 @@
       | $GREP "^# Generated by .*$PACKAGE" > /dev/null 2>&1
 }
 
+# func_lalib_unsafe_p file
+# True iff FILE is a libtool `.la' library or `.lo' object file.
+# This function implements the same check as func_lalib_p without
+# resorting to external programs.  To this end, it redirects stdin and
+# closes it afterwards, without saving the original file descriptor.
+# As a safety measure, use it only where a negative result would be
+# fatal anyway.  Works if `file' does not exist.
+func_lalib_unsafe_p ()
+{
+    lalib_p=no
+    if test -r "$1" && exec <"$1"; then
+       for lalib_p_l in 1 2 3 4
+       do
+           read lalib_p_line
+           case "$lalib_p_line" in
+               \#\ Generated\ by\ *$PACKAGE* ) lalib_p=yes; break;;
+           esac
+       done
+       exec <&-
+    fi
+    test "$lalib_p" = yes
+}
 
 # func_ltwrapper_p file
 # True iff FILE is a libtool wrapper script.
@@ -1431,15 +1453,15 @@
 
     # Handle -dlopen flags immediately.
     for file in $execute_dlfiles; do
-      test -f "$file" || \
-       func_fatal_help "\`$file' is not a file"
+      test -f "$file" \
+       || func_fatal_help "\`$file' is not a file"
 
       dir=
       case $file in
       *.la)
        # Check to see that this really is a libtool archive.
-       func_lalib_p "$file" \ ||
-         func_fatal_help "\`$lib' is not a valid libtool archive"
+       func_lalib_unsafe_p "$file" \
+         || func_fatal_help "\`$lib' is not a valid libtool archive"
 
        # Read the libtool library.
        dlname=
@@ -1505,7 +1527,7 @@
       -*) ;;
       *)
        # Do a test to see if this is really a libtool program.
-       if func_lalib_p "$file"; then
+       if func_ltwrapper_p "$file"; then
          # If there is no directory component, then add one.
          case $file in
          */* | *\\*) . $file ;;
@@ -1765,8 +1787,8 @@
 
       *.la)
        # Check to see that this really is a libtool archive.
-       func_lalib_p "$file" || \
-         func_fatal_help "\`$file' is not a valid libtool archive"
+       func_lalib_unsafe_p "$file" \
+         || func_fatal_help "\`$file' is not a valid libtool archive"
 
        library_names=
        old_library=
@@ -2260,8 +2282,8 @@
          ;;
        expsyms)
          export_symbols="$arg"
-         test -f "$arg" || \
-           func_fatal_error "symbol file \`$arg' does not exist"
+         test -f "$arg" \
+           || func_fatal_error "symbol file \`$arg' does not exist"
          prev=
          continue
          ;;
@@ -2299,7 +2321,7 @@
              # A libtool-controlled object.
 
              # Check to see that this really is a libtool object.
-             if func_lalib_p "$arg"; then
+             if func_lalib_unsafe_p "$arg"; then
                pic_object=
                non_pic_object=
 
@@ -2792,7 +2814,7 @@
        # A libtool-controlled object.
 
        # Check to see that this really is a libtool object.
-       if func_lalib_p "$arg"; then
+       if func_lalib_unsafe_p "$arg"; then
          pic_object=
          non_pic_object=
 
@@ -3305,8 +3327,8 @@
        fi
 
        # Check to see that this really is a libtool archive.
-       func_lalib_p "$lib" || \
-         func_fatal_error "\`$lib' is not a valid libtool archive"
+       func_lalib_unsafe_p "$lib" \
+         || func_fatal_error "\`$lib' is not a valid libtool archive"
 
        ladir=`$ECHO "X$lib" | $Xsed -e 's%/[[^/]]*$%%'`
        test "X$ladir" = "X$lib" && ladir="."