Re: [PATCH] [cygwin|mingw] fix dlpreopen with --disable-static

[libtool]

On Thu, 13 Nov 2008 22:09:07 +0100, "Ralf Wildenhues" said:
> OK, how about this.  It is a slight backward incompatibility, but
> not a large one:
> - --verbose undoes --silent *and* enables verbose output (that one with
>   func_verbose),
> - --no-silent *only* undoes --silent,
> 
> It should still be bearable for the user, in the sense that if you
> use --verbose rather than --no-silent, it's not a big problem.
> And we don't have to think about what
>   --verbose --verbose --silent
> 
> causes, we can just make the last one win.
> 
> If you agree, then let's proceed this way.  I don't mind who writes the
> patch.

That sounds good to me. The help output would need a little re-wording:

#       --quiet, --silent    don't print informational messages
#   -v, --verbose            print informational messages (default)
#       --no-silent          ???

I'll let you do that. <g>

> > >> B) func_win32_libid() gives some confusing errors to users
> > >>    when (a) using recursive make, and (b) MAKEFLAGS does not
> > >>    contain $OBJDUMP. Add a diagnostic error message, rather
> > >>    than allowing $SED to die a horrible death.
> [...]
> > Actually, this may no longer be necessary given the _LT_DECL_OBJDUMP
> > changes (I /said/ this was an old patch).  Here's the thread:
> > http://cygwin.com/ml/cygwin/2008-09/msg00552.html
> 
> Ah, ok, thanks.

I'll remove any of these bits from the revised patch(es).

> > Well, one reason I sat on this for so long was the 'fallback' mechanism
> > for deducing the dll name from the import library was just so...hideous.
> > And it wasn't a fallback -- it was the only mechanism since I hadn't yet
> > enhanced dlltool.
> 
> Do you steer dlltool development, BTW?

No. I've contributed a few patches over the years to dlltool and
binutils, but that's it.

> > The only reason to allow it is because (hopefully) that ugly fallback
> > code can get flagged with a warning, and maybe in a year or so get
> > removed.
> 
> Sounds like a good idea.

Of course, first I need to revise the dlltool patch and get it accepted;
there have been some comments on the binutils list.

> > Well, that, and it fixes a test that currently fails.
> 
> Which one, and can you post output for failure as well as success with
> the patch, please?

demo-exec after demo-shared, in the old test suite.

I'll post the output(s) tonight or tomorrow.

> Hmm.  I reviewed this whole function, and only when done I asked myself
> this, more radical question: we go great lengths here to find out a
> name.  Iff we have a *.la file to go with the implib, can't we just
> *know* the name?  I mean, we produced that thing, it has the expected
> name, no?  That's what the *.la file was designed for: to not have to
> look into the binary files for information.
> 
> Or is this purely for import libraries not created with libtool (and
> people who throw away *.la files)?

The information (e.g. library to dlpreopen) is passed in $dlprefiles.
But, if that filename is .la:  
func_mode_link():
...
        dlfiles|dlprefiles)
          if test "$preload" = no; then
            # Add the symbol object into the linking commands.
            func_append compile_command " @SYMFILE@"
            func_append finalize_command " @SYMFILE@"
            preload=yes
          fi
          case $arg in
          *.la | *.lo) ;;  # We handle these cases below.
          ...

...much later...
      *.la)
        # A libtool-controlled library.

        if test "$prev" = dlfiles; then
          # This library was specified with -dlopen.
          dlfiles="$dlfiles $arg"
          prev=
        elif test "$prev" = dlprefiles; then
          # The library was specified with -dlpreopen.
          dlprefiles="$dlprefiles $arg"
          prev=
        else
          deplibs="$deplibs $arg"
        fi
        continue
        ;;

So far, so good.  But then we eventually source the .la file, and end up
here (this is, in fact, what's happening in the demo-shared case):

       # This library was specified with -dlpreopen.
        if test "$pass" = dlpreopen; then
          if test -z "$libdir" && test "$linkmode" = prog; then
            func_fatal_error "only libraries may -dlpreopen a
            convenience library: \`$lib'"
          fi
          # Prefer using a static library (so that no silly _DYNAMIC
          symbols
          # are required to link).
          if test -n "$old_library"; then
            newdlprefiles="$newdlprefiles $dir/$old_library"
            # Keep a list of preopened convenience libraries to check
            # that they are being used correctly in the link pass.
            test -z "$libdir" && \
                dlpreconveniencelibs="$dlpreconveniencelibs
                $dir/$old_library"
          # Otherwise, use the dlname, so that lt_dlopen finds it.
          elif test -n "$dlname"; then
            newdlprefiles="$newdlprefiles $dir/$dlname"
          else
            newdlprefiles="$newdlprefiles $dir/$linklib"
          fi
        fi # $pass = dlpreopen

We've stored the DLL name as just ONE of the entries in $newdlprefiles.
But later, after we're done with all the $passes and are ready to
actually perform the link -- and that means generating the symbol lists
-- we scan back through $newdlprefiles (actually $dlprefiles, because
there was a reassignment).  During this scan thru $[new]dlprefiles we
want to recover the $linklib that goes with each entry -- worse, not all
entries in $[new]dlprefiles during that pass are DLLs, and so might not
have a matching $linklib -- and we also no longer know the name of the
.la file from which a particular entry in $newdlprefiles came from.
There might not be one (do we allow -dlpreopen
foo.a|foo.dll.a|foo.dll?).

The point is, we perhaps STARTED with the .la file, but the whole point
of the dlpreopen $pass is to replace each .la file in $dlprefiles with
the name of the object from which the symbols should be extracted, to
build the symbol table. So, pick one: either the DLL, or the import
library (there is no static lib, the failure mode in question occurs
when --disable-static).

If you pick "DLL" -- then it's real hard to get the symbols (objdump
ugliness, plus figuring out which ones are DATA).

If you pick "implib" -- then it's real hard to get the correct DLL name
(but not nearly as hard as extracting the correct symbols from the dll).

But the name of the .la file is no longer available.


I guess we could add another accumulator, $dl_linklib_prefiles [*], to
populate during the dlpreopen $pass, that will have exactly as many
entries as $[new]dlprefiles. These two accumulators would only in that
when $[new]dlprefiles contains a DLL, $newdl_linklib_prefiles would
contain the matching $linklib (or vice versa).  And (on
cygwin/mingw/cegcc?) we'd extract the symbols from the elements of
$newdl_linklib_prefiles -- except when the elements of $newdlprefiles
and $newdl_linklib_prefiles differ, the first entry of the symbol list
would be replaced by the DLL name (e.g. the element from
$newdlprefiles).


[*] or the accumulator could be $dl_la_prefiles, but then it would need
to have "magic" placeholder values corresponding to entries in the
original $dlprefiles that were NOT .la files

Is that really cleaner?


> In summary, it'd be great if you could redo the patch(es) along the
> comments in the previous message (but read below first).
> Couple more nits inline:

Ack.

> After your patch, this idiom occurs three times in ltmain.  Should we
> factorize like this
> 
>   if func_import_lib "$dlprefile"; then ...
> 
> with this?
> 
> func_import_lib ()
> {
>   case `eval $file_magic_cmd \"\$dlprefile\" 2>/dev/null | $SED -e 10q`
>   in
>   *import*) : ;;
>   *) false ;;
>   esac
> }
> 
> If you agree, you can do this as an independent patch if you like
> (preapproved).  Not sure if the name should be func_win32_import_lib
> rather.

Sure. I think the function should be called func_win32_import_lib_p
because it is a predicate.


> > +           dllname=`func_win32_dllname_for_implib "$dlprefile"`
> 
> can you write func_win32_dllname_for_implib so that it stores its
> result in a variable, so the caller doesn't need to fork?

No problem.

> > +func_win32_dllname_for_implib ()
> > +{
> > +  $opt_debug
> > +  f_win32_d_for_i_implib="$1"
> > +  f_win32_d_for_i_can_use_dlltool=no
> > +  f_win32_d_for_i_dllname=
> > +
> > +  # In recursive makes, DLLTOOL is often omitted from the
> > +  # passed-down MAKEFLAGS. As a courtesy, warn when this
> > +  # happens but don't fail; we have a workaround.
> > +  if test -z "${DLLTOOL}"; then
> > +    func_warning "\$DLLTOOL is not defined"
> 
> See my comment on the win32-dll option.

Ack.

> > +  else
> > +    # check for --identify option
> > +    if eval $DLLTOOL --help | $EGREP -- '--identify' >/dev/null ; then
> > +      f_win32_d_for_i_can_use_dlltool=yes
> > +    fi
> > +  fi
> 
> This should be a configure test.  Or, you could at least use a global
> variable name here (and maybe factorize the test out into a separate
> function) like
>   
>   if test -z "$dlltool_identify"; then
>     case `$DLLTOOL --help` in
>     *--identify*) dlltool_identify=: ;;
>     *) dlltool_identify=false ;;
>     esac
>   fi

Either way. The advantage of a runtime test is the "system" libtool
script (/usr/bin/libtool) would benefit without rebuilding if the system
binutils were updated; but of course that slows down runtime operation.
For "normal" usage -- an explicitly libtoolized package -- it makes no
effective difference (other than speed of execution).

On balance, a configure test seems better.

> > +
> > +  # use fallback implementation when dlltool is not available, or
> > +  # does not have the --identify option.
> 
> Or --identify did not work for some reason?  Because otherwise this
> line:
> 
> > +  if test -z "$f_win32_d_for_i_dllname"; then
> 
> can just be an 'else' to the previous 'if'.

Correct. If --identify is present but failed I wanted to go ahead and
try the fallback code. But it's not likely that the gloriously ugly sed
code will succeed where dlltool (with direct C access to libbfd) failed.
So, 'else' it is...

> > +    # make sure argument is actually an import library
> > +    if eval $file_magic_cmd \"\$f_win32_d_for_i_implib\" 2>/dev/null |
> > +      $SED -e 10q | $EGREP "import" >/dev/null; then
> 
> See func_import_lib above.

Ack.

> > +      # In recursive makes, OBJDUMP is often omitted from the
> > +      # passed-down MAKEFLAGS. As a courtesy, flag an error when
> > +      # this happens (it's more humane than allowing the sed
> > +      # expression below to fail).
> > +      test -z "${OBJDUMP}" && func_fatal_error "\$OBJDUMP is not defined"
> 
> See comment on win32-dll option.

Ack.

> > +      f_win32_d_for_i_dllname=`$OBJDUMP -s --section '.idata$7' 
> > $f_win32_d_for_i_implib |
> > +          $SED -e '/^.\{43\}/!d' -e 's/^.\{43\}//' |
> > +          $SED -e ':a;N;$!ba;s/\n//g' -e 's/\.\.\.*//g' \
> > +               -e 's/[     ][      ][      ]*//g' \
> > +               -e 's/^[    ]*//' -e 's/[   ]*$//'`
> 
[snip esoteric sed stuff]
> 
> denoting the start of another object file?  Also, we should be able to
> finish the script as soon as we've found a match, no?

Yes, and no. Stop-on-success was the way I originally coded the dlltool
changes, but the suggestion here:
http://sourceware.org/ml/binutils/2008-11/msg00085.html
is to continue checking...and fail if more than one.  It IS possible to
have an import lib directly reference two (or more) DLLs (not counting
forwarded symbols, which is a different thing) -- but IMO this is not
likely except as some pathological corner case.

Anyway, this would break libtool's symbol extraction anyway -- and the
fix is...ugly.

We'd have to parse the 'U __head_*_dll' symbol in each member of the
.dll.a that contains a "real" symbol, match it up to the member that
contains the 'I __head_cygz_dll' symbol, and take THAT member's 'U
_*_dll_iname' symbol, and THEN locate the member that has the matching
'I _*_dll_iname'.

This member presumbly contains the appropriate .idata$7 section, so now
we have the specific DLL for the original 'real' symbol in question.

Repeat for all symbols, building separate lists for each referenced DLL.
Then, create two (or more) separate
lt_${my_prefix}_LTX_preloaded_symbols arrays for each DLL and its
symbols referenced by this frankenstein .dll.a. And try to do all that
using portable sh code and only $SED and $AWK.

No thanks.

So, YES, the fallback code can certainly stop-on-success.

> 
> > --- a/libltdl/m4/libtool.m4
> > +++ b/libltdl/m4/libtool.m4
> 
> > @@ -4185,12 +4186,12 @@ m4_if([$1], [CXX], [
> >    ;;
> >    cygwin* | mingw* | cegcc*)
> >      _LT_TAGVAR(export_symbols_cmds, $1)='$NM $libobjs $convenience | 
> > $global_symbol_pipe | $SED -e '\''/^[[BCDGRS]][[ ]]/s/.*[[ ]]\([[^ ]]*\)/\1 
> > DATA/;/^.*[[ ]]__nm__/s/^.*[[ ]]__nm__\([[^ ]]*\)[[ ]][[^ ]]*/\1 
> > DATA/;/^I[[ ]]/d;/^[[AITW]][[ ]]/s/.* //'\'' | sort | uniq > 
> > $export_symbols'
> > +    _LT_TAGVAR(exclude_expsyms, 
> > $1)=['_head_[A-Za-z0-9_]+_dll|[A-Za-z0-9_]+_dll_iname']
> 
> Now this change applies to cegcc as well.  Hmm.

True, but unless the format of the import libs emitted by cegcc differs
in that _head_*_dll and *_dll_iname are allowed to be "real" symbols as
opposed to book-keeping indirections for the compiler, it's probably
correct for cegcc as well.

> > -      _LT_TAGVAR(export_symbols_cmds, $1)='$NM $libobjs $convenience | 
> > $global_symbol_pipe | $SED -e '\''/^[[BCDGRS]][[ ]]/s/.*[[ ]]\([[^ ]]*\)/\1 
> > DATA/'\'' | $SED -e '\''/^[[AITW]][[ ]]/s/.*[[ ]]//'\'' | sort | uniq > 
> > $export_symbols'
> > +      _LT_TAGVAR(export_symbols_cmds, $1)='$NM $libobjs $convenience | 
> > $global_symbol_pipe | $SED -e '\''/^[[BCDGRS]][[ ]]/s/.*[[ ]]\([[^ ]]*\)/\1 
> > DATA/;/^.*[[ ]]__nm__/s/^.*[[ ]]__nm__\([[^ ]]*\)[[ ]][[^ ]]*/\1 
> > DATA/;/^I[[ ]]/d;/^[[AITW]][[ ]]/s/.* //'\'' | sort | uniq > 
> > $export_symbols'
> 
> Can't we omit the /^.*[[ ]]__nm__/  before the 's' here?

Cut-n-paste from the existing C++ version. It looks like they both could
be modified as you suggest -- but surely there was a reason that
apparent duplication was put there?  I'll have to check the archives; I
remember it took some doing to get this correct (which makes it a shame
that the C part was lost sometime during the ->2.0 X ->2.2 development
fiasco).  This original change goes back to early 1.5 days, IIRC.

> > +      _LT_TAGVAR(exclude_expsyms, 
> > $1)=['_head_[A-Za-z0-9_]+_dll|[A-Za-z0-9_]+_dll_iname']
> 
> I'm actually not sure whether _GLOBAL__F[ID]_.* can appear on w32.
> Do you know?  They should happen with C++ code using constructors
> and destructors IIRC.

I couldn't find that symbol in cygxerces-c28.dll -- and the Xerces-C
code uses lots of custom constructors and destructors. However, it
doesn't have many (any?) static objects AFAIK; would that make a
difference?

--
Chuck