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

[Ralf Wildenhues]

Hi Charles,

* Charles Wilson wrote on Thu, Nov 13, 2008 at 06:09:20AM CET:
> Ralf Wildenhues wrote:
> > 
> > Well, --verbose is documented to be a reversal of --silent, and
> > documented to be the default.  The fact that opt_verbose is never set is
> > a limitation.  If fixed, that should better happen in a separate patch.
> 
> Well, if --verbose is really the negation of --silent, then (unless the
> functionality is extended as you describe) the effect of --verbose
> should be to only unset opt_silent (which it does), and there should not
> exist any 'opt_verbose' variable.

Yes, I agree there is some inconsistency currently.

> In this case, to avoid backwards compatibility issues, the new "I want
> really talkative output, but not --debug" option should be something
> other than '--verbose' -- because that already has a specific meaning:
> negate--silent.
> 
> --chatty?  (I know, it's idiomatic and that's bad. It's more a
> tongue-in-cheek suggestion anyway).
> 
> Regardless, I think the current (2? 3?) usages of opt_verbose should be
> changed to !opt_silent.

20 uses of func_verbose, actually.  A bit much to undo, methinks.
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.

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

> >> When configuring with --disable-static, dlpreopen is very confused.
> >> First, libtool tries to extract the symbols -- using $NM and
> >> $global_symbols_pipe -- from the DLL. That works...poorly:
[...]
> > OK, I see that this is problematic.  What I don't understand yet is:
> > is there a way to extract only the interesting symbols from the DLL?
> > I don't yet understand why we have to move to the import library.
> 
> Because it's extremely tricky. You have to use objdump (not nm), and
> then search for the exported symbols which is non-trivial because you
> need a stateful parser -- maybe an awk program...

Hmm, yes, agreed.  Thanks for all your detailed explanations.  I'm glad
to not have had to analyze this myself.

> The ugliest part of my patch is the fallback code for deducing the
> dllname from an import library.  But that's *pretty* compared to mucking
> with objdump output.

OK.  :-)

> Alternatively, libtool used to embed an "impgen" program which could be
> ressurected to generate the symbol lists we need (rather than a .def
> file, as it used to do).  A better solution would be to push that
> functionality into dlltool ("Hmm. I need to generate a symbol list or
> def file for a DLL." but 'dlltool --output-def=my.def my.dll' doesn't do
> what you expect).  Even so, the core functionality of impgen would need
> some improvements (especially so as to indicate DATA items, which it
> doesn't do at present).
> http://loreley.ath.cx/cygwin/impgen/impgen.c
> 
> But then, you're back to requiring a very recent binutils, or providing
> a fallback -- see "objdump ugliness" above.

ACK.

> > Once I understand that, I can better judge the rest of the patch.
> 
> 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?

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

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

> > for example mentioning in the mail
> > whether you considered cygwin or cegcc or so would be helpful for
> > review).
> 
> cygwin and mingw yes. I know nothing about cegcc.

OK, that's what I figured.

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:

> --- a/libltdl/config/ltmain.m4sh
> +++ b/libltdl/config/ltmain.m4sh

> @@ -1991,10 +1992,36 @@ extern \"C\" {
>         func_verbose "extracting global C symbols from \`$dlprefile'"
>         func_basename "$dlprefile"
>         name="$func_basename_result"
> -       $opt_dry_run || {
> -         eval '$ECHO ": $name " >> "$nlist"'
> -         eval "$NM $dlprefile 2>/dev/null | $global_symbol_pipe >> '$nlist'"
> -       }
> +       case $host in
> +         *cygwin | *mingw* )
> +           # if an import library, we need to obtain dlname
> +           if eval $file_magic_cmd \"\$dlprefile\" 2>/dev/null |
> +               $SED -e 10q | $EGREP "import" >/dev/null; then

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.

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

> @@ -2198,6 +2230,91 @@ func_win32_libid ()
>  }
>  
>  
> +# func_win32_dllname_for_implib implib
> +# Obtain the name of the DLL associated with the
> +# specified import library.

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

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

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

The long names are not only awkward to read, they also don't help in
that they still require you to take care not to use recursive functions.
(This is, of course, not a criticism of your patch, but of the way we
write these functions.)

> +  if test "$f_win32_d_for_i_can_use_dlltool" = "yes"; then
> +    f_win32_d_for_i_dllname=`$DLLTOOL --identify "$f_win32_d_for_i_implib" 
> 2>/dev/null`
> +  fi
> +
> +  # 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'.

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

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

> +      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/[   ]*$//'`

Inside the sed script, please put tab before space, that avoids
whitespace warnings (from the git commit hook, for example).

Using a semicolon after ':' or 'b' command is not portable according to
autoconf.info, so please use either newlines or -e to separate them.
The 'b' command should be followed by one space.

The second sed script slurps in all of the text before doing further
processing, then removes all newlines, and then applies some more
regexes.  My testing of this script with GNU sed and glibc 2.7 showed
that these tools operate in linear time, but other combinations may not.
In any case, can't we restart when the input contains
  ^[^    ]*\.o:

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?

Something like this (untested):

  $SED '/^[^     ]*\.o:/{
          s/.*//
          p
          d
        }
        /^.\{43\}/!d
        s/^.\{43\}//' |
  $SED -n '
        :more
        N
        /\n$/b work
        $!b more
        :work
        s/\n//g
        s/\.\.\.*//g
        s/[      ][      ][      ]*//g; s/^[     ]*//; s/[       ]*$//
        /./{
          p
          q
        }
        '

>  # func_extract_an_archive dir oldlib
>  func_extract_an_archive ()
> @@ -5052,11 +5169,24 @@ func_mode_link ()
>           # 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"
> +       # Otherwise, use the dlname, so that lt_dlopen finds it --
> +       # except on mingw|cygwin, where we must use the import library
> +       # for symbol extraction. Therefore, on those platforms, the name
> +       # needed by lt_dlopen is extracted from the import library via
> +       # func_win32_dllname_for_implib.
>         else
> -         newdlprefiles="$newdlprefiles $dir/$linklib"
> +         case "$host" in
> +           *cygwin* | *mingw* )
> +             newdlprefiles="$newdlprefiles $dir/$linklib"
> +                ;;
> +           * )
> +             if test -n "$dlname"; then
> +               newdlprefiles="$newdlprefiles $dir/$dlname"
> +             else
> +               newdlprefiles="$newdlprefiles $dir/$linklib"
> +             fi
> +                ;;
> +         esac
>         fi
>       fi # $pass = dlpreopen
>  

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

>    ;;
>    *)
>      _LT_TAGVAR(export_symbols_cmds, $1)='$NM $libobjs $convenience | 
> $global_symbol_pipe | $SED '\''s/.* //'\'' | sort | uniq > $export_symbols'
>    ;;
>    esac
> -  _LT_TAGVAR(exclude_expsyms, $1)=['_GLOBAL_OFFSET_TABLE_|_GLOBAL__F[ID]_.*']
>  ], [
>    runpath_var=
>    _LT_TAGVAR(allow_undefined_flag, $1)=
> @@ -4329,7 +4330,8 @@ _LT_EOF
>        _LT_TAGVAR(allow_undefined_flag, $1)=unsupported
>        _LT_TAGVAR(always_export_symbols, $1)=no
>        _LT_TAGVAR(enable_shared_with_static_runtimes, $1)=yes
> -      _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?

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

Thanks,
Ralf