Re: MSVC: Support for response files with $NM.

[Peter Rosin]

Hi Ralf,

Den 2010-07-06 08:07 skrev Ralf Wildenhues:
> Hi Peter,
>
> * Peter Rosin wrote on Fri, Jul 02, 2010 at 02:45:47PM CEST:
> > The MSVC linker can't do relinking (-r -o), and this patch avoids
> > a usage of that in libtool when it digs for symbols in large
> > numbers of object files. With the patch I get identical behavior
> > inside "Run tests with low max_cmd_len" as I do outside, without
> > the patch lots of stuff go belly-up with low max_cmd_len. I found
> > no regressions on neither cygwin/gcc nor debian/gcc (admittedly
> > both with "recent" binutils nm, supporting @FILE).
>
> Did you confirm that the debian/gcc case actually uses the @FILE code
> path in the testsuite (should be in the low_cmd_len tests)?

No I didn't, but now I have and nm @FILE is used by at least export.at,
deplib-in-subdir.at and stresstest.at. And it works just fine for
me.

> > 2010-07-01  Ralf Wildenhues<address@hidden>
> >         Peter Rosin<address@hidden>
> >
> >         Support for response files with $NM.
> >         * libltdl/m4/libtool.m4 (_LT_CMD_GLOBAL_SYMBOLS)
> >         <nm_file_list_spec>: New tag variable. Set it to '@' if input
> >         files can be passed to $NM in a file named with the '@' option.
> >         * libltdl/config/ltmain.m4sh (func_mode_link): When
> >         nm_file_list_spec is nonempty, use it to avoid skipped_export.
> >         * doc/libtool.texi (libtool script contents): Document
> >         new variable.
>
> You've pushed this already, but I still have nits:
>
> > --- a/libltdl/config/ltmain.m4sh
> > +++ b/libltdl/config/ltmain.m4sh
> > @@ -6719,14 +6719,30 @@ EOF
> >             $opt_dry_run || $RM $export_symbols
> >             cmds=$export_symbols_cmds
> >             save_ifs="$IFS"; IFS='~'
> > -           for cmd in $cmds; do
> > +           for cmd1 in $cmds; do
> >               IFS="$save_ifs"
> > -             eval cmd=\"$cmd\"
> > +             eval cmd=\"$cmd1\"
> >               func_len " $cmd"
> >               len=$func_len_result
> >               if test "$len" -lt "$max_cmd_len" || test "$max_cmd_len" -le -1; 
> > then
> >                 func_show_eval "$cmd" 'exit $?'
> >                 skipped_export=false
> > +             elif test -n "$nm_file_list_spec"; then
>
> Actually, why even do the expensive test for long command lines, and not
> just go for the spec file all the time?  I'm not sure we really want to
> do this, as it hampers debugging a bit (the developer doesn't readily
> see the contents of the response file), but it surely would be a bit
> more efficient.

I wanted to change as little as possible. The expensive test was not
added by the patch, so I don't feel too bad about that. I didn't
even realize it was expensive, but if that's the case it is
certainly possible to first test for nm_file_list_spec and only do
the max_cmd_len test otherwise.

> > +               output_la=`$ECHO "X$output" | $Xsed -e "$basename"`
>
> save 2 fork&exec:
>
> func_basename "$output"
> output_la=$func_basename_result

Yes, good point. I'll send a patch later if noone beats me to it, since
it might easily take me a couple of days to get to it. Life is interfering.

> > +               save_libobjs=$libobjs
> > +               save_output=$output
> > +               output=${output_objdir}/${output_la}.nm
> > +               libobjs=$nm_file_list_spec$output
> > +               func_append delfiles " $output"
> > +               func_verbose "creating $NM input file list: $output"
> > +               for obj in $save_libobjs; do
> > +                 $ECHO "$obj"
> > +               done>  "$output"
> > +               eval cmd=\"$cmd1\"
>
> This eval is wrong and shouldn't be necessary, func_show_eval below
> already does an evaluation.  Please check that it is not needed.

The double eval was there before, so the patch just copied the
existing style. Or didn't it? The code was:

            cmds=$export_symbols_cmds
            save_ifs="$IFS"; IFS='~'
            for cmd in $cmds; do
              IFS="$save_ifs"
              eval cmd=\"$cmd\"
              func_len " $cmd"
              len=$func_len_result
              if test "$len" -lt "$max_cmd_len" || test "$max_cmd_len" -le -1; 
then
                func_show_eval "$cmd" 'exit $?'


> > +               func_show_eval "$cmd" 'exit $?'
> > +               output=$save_output
> > +               libobjs=$save_libobjs
> > +               skipped_export=false
> >               else
> >                 # The command line is too long to execute in one step.
> >                 func_verbose "using reloadable object file for export list..."

Thanks for the review!

Cheers,
Peter