Re: [PATCH] [cygwin|mingw] fix dlpreopen with --disable-static (take 8?)

[Charles Wilson]

On 6/26/2010 2:51 PM, Ralf Wildenhues wrote:
> * Charles Wilson wrote on Fri, Jun 25, 2010 at 04:57:15AM CEST:
>> However, with this patch, helldl.exeS.c has:
> 
> IOW, all the spurious declarations are gone?  That'd be cool.

Correct.

>> (*) Note that you only need to determine the dll name for an import
>> lib using dlltool --identify or the fallback, IF and ONLY IF you are
>> linking to a library WITHOUT a corresponding .la file.

After trying unsuccesfully for hours to convince modern libtool to
actually allow me to DO this -- for testing purposes -- I wonder if
   -dlpreopen /usr/lib/somelib
where somelib is an .so or .a or .dll.a is ALLOWED at all.  I'm pretty
sure it USED to work -- like five or six years ago.  But it doesn't
appear to be documented to work that way, and I certainly can't figure
out how to get the libtool to even let me do it. It simply adds the
specified library to the link command, but never extracts a symbol table
for it.

If this is NOT actually supported, then I can simplify this patch quite
a bit I think. We'd no longer need ANY of the "figure out the .dll name
from the .dll.a" functions.

OTOH, if this mode IS supposed to be supported...then I think I've found
yet another bug, not covered by the test suite.  There's also this hint
that 1.5 years ago, I didn't explore this modality (-dlpreopen
not-an-la-file) very much:

http://lists.gnu.org/archive/html/libtool-patches/2009-01/msg00056.html
> Limitation: although I have beat this sed-fu

sed-fu: refers to the giant sed script in
func_cygming_dll_for_implib_fallback_core

> to death *outside*
> of libtool, and am pretty confident it works well, there is no
> actual test of that code in the testsuite. This is because well-
> behaved libtool clients -- and our tests are actually well-behaved
> in this regard -- will only -dlpreopen *libtool*-built libraries.

e.g. ".la" files

> In that case, Ralf's suggested libfile_$(transliterated implib name)
> is used, because we have the .la file available which allows that
> shortcut.  The only time we need `dlltool --identify' is when
> dlpreopening a non-libtool implib, where we have no .la file.
> And the sed-fu is a fallback to THAT fallback.


So...can I get a verdict?  Is -dlpreopen not-an-la-file supposed to work?

>> If you DO have
>> a .la file for the library you're linking to, then the "save the .la
>> file using a magic shell variable" approach is used.
>
> OK.  Here's my take on this: if you fix all nits I noted inline below,
> post the updated and tested patch (you decide what testing is needed),
> then you are OK to commit after 72 hours of waiting.  FWIW, I'm likely
> not available most of next week; if we find issues later, then I guess
> they'll just have to be fixed afterwards.

OK.

> I'll note though that this patch makes me cringe: it introduces more
> system-specific code in ltmain that is just bloat on other systems,
> rather than in libtool.m4 where it belongs. 

Well, I did manage to come up with a mechanism to move most of

func_cygming_dll_for_implib
func_cygming_dll_for_implib_fallback
func_cygming_dll_for_implib_fallback_core
func_cygming_gnu_implib_p
func_cygming_ms_implib_p

into libtool.m4 (attached; only lightly tested pending Q above).
However, I can't see how to move the other mods in func_generate_dlsyms
and func_mode_link there.

I tried to use Gary's _LT_PROG_XSI_REPLACE function, but using a sed
script to create a sed script and all the quoting nightmares just made
my head hurt.  So, I have _LT_PROG_FUNCTION_REPLACE that uses the old
'copy half the script, emit the new function content, copy the rest of
the script' algorithm.

> It has the potential to
> regress non-cygwin non-mingw w32 systems, and since we effectively have
> no cegcc maintainer that is a problem for cegcc (dunno if for us).
> The patch doesn't introduce a new test, so I assume it either fixes a
> current testsuite failure in either the default setup or in the
>   ./configure --disable-static

It fixed an OLD failure in demo-shared/demo-make/demo-exec.  However,
this patch originally addressed 4 separate issues that were clustered
together -- but the demo-shared test failure actually exposed only one
of them.  In the interim, that particular issue was been, well, not
fixed I don't think, but rather avoided.  So, NOW, there is no actual
failure that this patch fixes.  The symbol list in demo-shared's .exe is
really ugly, but the test doesn't FAIL because of that.

> setup of Libtool.  If not, or if only the latter, then a testsuite
> addition in a followup patch would be nice.

OK, I'll look into that for a followup.

> Does this patch have any relation to Pierre Ossman's "Preloading without
> .la" patch?  http://thread.gmane.org/gmane.comp.gnu.libtool.general/7071
> His copyright papers are through now, so we can look at that patch.

Well, I *think* Pierre's patch would break cygwin -- but that'd be true
with or without this patch.  (The following statement in libltdl is not
true, for cygwin when -dlpreload and --disable-static):

/* Preloaded modules are always named according to their old
   archive name. */

because at least currently, the second entry in the
LTX_preloaded_symbols array is "cygfoo-N.dll" in those circumstances,
not "libfoo.a".

The title of Pierre's thread is a bit confusing (at least to me). What
he is talking about is using libltdl at runtime, with a variety of names
refering to the same library (module, module.la, module.a, etc).  That's
why HE means by "Preloading without .la". My Question above is about
*build* time, when you're trying to specify -dlpreopen not-a-.la.

>> --- a/libltdl/config/general.m4sh
>> +++ b/libltdl/config/general.m4sh
>> @@ -559,4 +559,21 @@ func_show_eval_locale ()
>>        fi
>>      fi
>>  }
>> +
>> +# func_tr_sh
>> +# Turn $1 into a string suitable for a shell variable name.
>> +# Result is stored in $func_tr_sh_result.  All characters
>> +# not in the set a-zA-Z0-9_ are replaced with '_'. Further,
>> +# if $1 begins with a digit, a '_' is prepended as well.
>> +func_tr_sh ()
>> +{
>> +  case "$1" in
> 
> No double-quotes needed here.

Even if $1 might have spaces?   It's a pathname:

  func_tr_sh "$dlprefile"

But, I guess, since dlprefile obtained as a member of a space-separated
list, it BETTER not have any spaces in it. OK.

>> +                eval '$sharedlib_from_linklib "$dlprefile"'
> 
> redundant eval: remove eval and single quotes.

OBE in the new version.

>> +                eval '$ECHO ": $dlprefile_dlbasename" >> "$nlist"'
>> +                eval '$ECHO ": $name " >> "$nlist"'
> 
> Likewise.

Those are (copies, adaptations of) pre-existing code:

-         $opt_dry_run || {
-           eval '$ECHO ": $name " >> "$nlist"'
-           eval "$NM $dlprefile 2>/dev/null | $global_symbol_pipe >>
'$nlist'"

I don't feel comfortable folding in a change like that as part of this
patch, but I'd be happy to supply a separate follow-on patch to change
them all at once.

>> +              fi
>> +              eval "$NM $dlprefile 2>/dev/null | $global_symbol_pipe |
>> +                $SED -e '/I __imp/d' -e 's/I __nm_/D /;s/_nm__//' >> 
>> '$nlist'"
> 
> This can probably have the outer double quotes removed, eval moved to
> $global_symbol_pipe, and quotes around $nlist removed.  Actually, don't
> change this, because it might be the eval isn't needed at all; I will
> check this and change all uses in libtool then.

Again, this is a copy of pre-existing code, so...I'll leave this to you.

>> +              eval '$ECHO ": $name " >> "$nlist"'
> 
> Likewise eval and single quotes redundant.
>> +            eval '$ECHO ": $name " >> "$nlist"'
> 
> Likewise.

See above.

>> +# func_cygming_dll_for_implib_fallback_core SECTION_NAME LIBNAMEs
>> +#
>> +# The is the core of a fallback implementation of a
>> +# platform-specific function to extract the name of the
>> +# DLL associated with the specified import library LIBNAME.
>> +#
>> +# SECTION_NAME is either .idata$6 or .idata$7, depending
>> +# on the platform and compiler that created the implib.
>> +#
>> +# Echos the name of the DLL associated with the
>> +# specified import library.
> 
> You'd save a fork if this function stores its result in a variable.
> I'd just use sharedlib_from_linklib_result for that.

I can't actually do this.  The problem is that the function is simply a
big pipeline:

  objdump $1 | sed | sed

So, the caller (func_cygming_dll_for_implib_fallback) invokes as

sharedlib_from_linklib_result=`func_cygming_dll_for_implib_fallback_core
...`

which is a fork, it is true.  But for
func_cygming_dll_for_implib_fallback_core to put the result in a
variable, IT would have to do:

   sharedlib_from_linklib_result=`objdump $1 | sed | sed`

which is...an extra fork.  So it's six of one, a half-dozen of the
other.  (And, I found that I had difficulty trying to express it as
`objdump $1 | sed |sed` with such a complicated sed expression. sh was
not happy.

That's actually the REASON I split this operation into a main function
and a _core function.

>> +func_cygming_dll_for_implib_fallback_core ()
>> +{
>> +  $opt_debug
>> +  $OBJDUMP -s --section "$1" "$2" 2>/dev/null |
>> +    sed '/^Contents of section '"$1"':/{
> 
> Can $1 contain slash, dollar, ^, characters?

Yes.  In fact, it only ever has one of two values: '.idata$6' and
'.idata$7'.

>> +      # Place marker at beginning of archive member dllname section
>> +      s/.*/====MARK====/
>> +      p
>> +      d
>> +    }
>> +    # These lines can sometimes be longer than 43 characters, but
>> +    # are always uninteresting
>> +    /:[ \t]*file format pe[i]\{,1\}-i386$/d
> 
> What about x86-64, cegcc?  Look at func_win32_libid.
> \t is not portable sed, please use literal TAB then space.

Changed to /:[ \t]*file format pe[i]\{,1\}-$/d

which should match all of those variants.  I can't reuse the
func_win32_libid egrep expression, because this is sed.

>> +    /^In archive [^:]*:/d
>> +    # Ensure marker is printed
>> +    /^====MARK====/p
>> +    # Remove all lines with less than 43 characters
>> +    /^.\{43\}/!d
>> +    # From remoaining lines, remove first 43 characters
> 
> typo remaining

Fixed.

>> +    # Of those that remain, print the first one.
>> +    sed -e '/^\./d' -e '/^.\./d' | sed -n -e '1p'
> 
> Can't you join the last three sed scripts?  Replace the last commmand /./p
> of the third-to-last script with
>   /^\./d
>   /^.\./d
>   p
>   q
> 
> and be done with it.  Right?

No, it doesn't work. The best I can do is consolidate them into a single
separate expression:

sed -e '/^\./d;/^.\./d;q'

>> +}
>> +
>> +# func_cygming_gnu_implib_p ARG
>> +# This predicate returns with zero status (TRUE) if
>> +# ARG is a GNU/binutils-style import library. Returns
>> +# with nonzero status (FALSE) otherwise.
>> +func_cygming_gnu_implib_p ()
>> +{
>> +  $opt_debug
>> +  func_cygming_gnu_implib_tmp=`eval "\$NM \$1 | \$global_symbol_pipe | 
>> \$EGREP ' (_head_[A-Za-z0-9_]+_dll|[A-Za-z0-9_]+_dll_iname)\\\$'"`
> 
> What is the eval for here?  If it is needed, it should probably be
> before the assignment, a la
>   eval "func_cygming_gnu_implib_tmp=..."
> 
> or, if just for the symbol pipe, then just inside there, but the
> backslash before $global_symbol_pipe really makes no sense to me.
...
>> +  func_cygming_ms_implib_tmp=`eval "\$NM \$1 | \$global_symbol_pipe | grep 
>> '_NULL_IMPORT_DESCRIPTOR'"`
> 
> Likewise.
> 

It's only needed for $global_symbol_pipe.


>> +# _LT_DECL_DLLTOOL
>> +# --------------
> 
> Please align underline with title.

Ack.

>> +# If we don't have a new enough Autoconf to choose the best dlltool
>> +# available, choose the one first in the user's PATH.
> 
> What does this comment mean?  Copy and pasto garbage?

Yep. That's what _LT_DECL_OBJDUMP and _LT_DECL_EGREP say. I've removed
it from _LT_DECL_DLLTOOL, but I've left the other two alone.

> I'll note that _LT_DECL_DLLTOOL and _LT_DECL_OBJDUMP duplicate code from
> the win32-dll option setting in ltoptions.m4.  That is wrong.  I'll let
> it go through now because the problem isn't new.

Ack.

No changelog or in-depth testing on this version of the patch yet; if we
can dispense with all the dllname-from-implib stuff it'll be simplified
considerably, so it's not worth the effort until my Q is answered.

I did test it minimally on cygwin, mingw, and linux and it does work; I
just didn't do the whole regression suite.

--
Chuck

0009-cygwin-mingw-fix-dlpreopen-with-disable-static--take-8.patch
Description: Source code patch