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

[Ralf Wildenhues]

* Charles Wilson wrote on Sun, Jun 27, 2010 at 02:51:21PM CEST:
> > 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?

I think Pierre's report was about using -dlpreopen file.la at link time,
but then not wanting to need the .la file at run time.  I think that is
desirable.  At link edit time, having a .la file is a reasonable thing I
think.

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

I don't see this method as the new method of choice.  We already have a
mechanism for years to transport values to the libtool script with
_LT_DECLs and _LT_TAGDECLs, and at least for small code snippets, that
should be used.  I'd probably be more confident with code in ltmain that
you did test rather than a new transplanting method that has not been
tested well, and thus by definition has bugs.

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

Well yeah, this confusion and interface non-well-definedness is bad, no?

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

OK.

> >> +func_tr_sh ()
> >> +{
> >> +  case "$1" in
> > 
> > No double-quotes needed here.
> 
> Even if $1 might have spaces? 

Yes.  The shell does not do word splitting on the right hand side of an
assignment and in the argument to 'case'.  Just try it:

  foo="with space"; case $foo in *space*) echo whoo;; esac

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

Irrelevant.

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

Don't worry.  I'm working on a related change anyway that fixes a lot of
the eval stuff.

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

OK.

> >> +# func_cygming_dll_for_implib_fallback_core SECTION_NAME LIBNAMEs

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

OK, fine like this then.  Thanks for the explanation, and sorry for not
seeing that.

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

Well then the characters which are special in sed regexes need to be
escaped, otherwise a .idata$6 will never match: $ matches end of line,
there is never a 6 after an end of line.  A period OTOH matches any
character.  To generally escape a string so that sed matches it
literally:

  match_literal=`$ECHO "string" | sed 's/[].[^$\\*|]/\\\\&/g'`
  sed "/$match_literal/..."

(untested, from Automake).  Or you just additionally pass a regex.

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

OK thanks.

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

Yeah, that's the bugger I'm fixing currently.

I'll note again that you do have OK in the manner I wrote in my last
review.

Thanks,
Ralf