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

[Charles Wilson]

On 6/27/2010 4:43 PM, Ralf Wildenhues wrote:
> * 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.

So...I *don't* need to worry about -dlpreopen not-an-.la?  The issue is
that I can't figure out how *current* libtool EVER gets here:

(current master ltmain.m4sh:1984:func_generate_dlsyms)

  for dlprefile in $dlprefiles; do
    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'"
    }
  done

in that situation, with anything IN $dlprefiles -- because in
ltmain.m4sh, we have:

5764     if test "$linkmode" = prog; then
5765       dlfiles="$newdlfiles"
5766     fi
5767     if test "$linkmode" = prog || test "$linkmode" = lib; then
5768       dlprefiles="$newdlprefiles"
5769     fi

and at this point, both newdlfiles and newdlfiles are empty, when the
argument to libtool's -dlpreopen option is not a libtool .la library.

So...we APPEAR to have a bunch of dead code.  It obviously isn't
SUPPOSED to be dead -- or it wouldn't be there.  So, I can either keep
(that is, commit) all of my new stuff in this patch, some of which will
also be dead code, in anticipation of somebody tracking down WHY it and
these existing snippets are (currently) dead, and brings them back to life.

Or I can NOT commit any new (dead) code and commit only those bits that
are presently "live", and wait until after the existing dead code is
resurrected, and THEN add those particular bits that I'd've held back.

Which?  (Note that I didn't realize it was actually dead until I really
started trying hard to exercise the dll-to-implib stuff /in situ/ in
current libtool tip. This dll-to-implib stuff was necessary in the early
days of this patch development, but it SEEMS like intervening changes
have made it dead, along with other existing parts of libtool.)

The code we're talking about is, specifically:

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

and the one location that calls it (via eval $shared_to_implib_cmd) --
plus the .m4 code that sets the value of shared_to_implib_cmd.

Naturally I'd rerun all the tests on multiple platforms if I made such a
major change.

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

Yeah, that's the problem. You complained that these functions added a
lot of parse time to all the other platforms that would never use them,
presumably because they were BIG functions and there were several of
them.  Presumably, the parse-time cost of small functions is low, unless
there are a TON.

Worst case, of course, is a TON of big functions that nobody but
$myplatform uses.

So...I tried to move the big ones to libtool.m4 (leaving only small
stubs in ltmain.m4sh), because (a) they're the worst culprits in
parse-time cost, and (b) they can't fit in _LT_DECLs anyway.

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

That's fine.  Private branches are easily deleted.

But please, in the future, don't complain so strongly ([your patch]
"makes me cringe") about architectural issues if you don't actually want
to see them fixed: "system-specific code in ltmain...rather than in
libtool.m4 where it belongs."

I feel (more) discouraged now (than usual), having wasted so much time
addressing a criticism of a patch that wasn't meant to be taken seriously.

The reason I'm reacting so strongly here is because you've stated this
same criticism many times over the years, in relation to win32 patches
(mine and others) -- especially the cross compile patch which is next on
the list.

In fact, I have often wondered if the reason many of my patches -- and
Peter's -- tend to languish so long is because of these aesthetic
objections to the system-specific code that cyg/mingw seems to
require...  I keep wondering if folks think "gee, there must be a way to
do that in m4...let me think about that a while".  And a while becomes a
month, and a month becomes three, and three becomes twelve...and the
vapor perfect defeats the concrete good.

Anyway, this patch AND that upcoming cross-compile patch both add
several large system-specific new functions to ltmain.sh.  Since you
objected to them now, I figured I'd hear it again THEN, so...I took this
opportunity to TRY and create the appropriate infrastructure to handle
LARGE system-specific functions from m4.  (Few if any of these functions
are suitable candidates for single-line $foo_cmd _LT_DECL-style
treatment, or even just few-line $foo_cmd)


I won't bother to do that in the future.


I'll let somebody else devise the blessed way to address the issue, and
then I'll be glad to assist in adapting (hopefully soon-to-be-committed)
sections of code that could exploit the new mechanisms. Whatever those
mechanisms are, and whenever that actually happens.

Unless, of course, the blessed way is to try to shoe-horn a 40-line sed
expression into a $foo_cmd.  That's just...crazy talk.  At some point
you've got to consider the difficulties in *debugging* these things, if
they're stuffed into a blah~blah~blah~blah nightmare variable.  It's
hard enough putting complex function bodies into m4, given the
re-autotooling you have to do between tests: VERY long
edit-"compile"-test cycles.  Debugging the eval of a giant $foo_cmd,
whose CONTENTS are specified by the .m4?  That's doubly bad. <shudder>.

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

Sure it is.  But some of these considerations are hard to accommodate on
win32 if the .la file is not available at runtime, AND the caller
doesn't explicitly specify the actual DLL name when calling lt_dlopen.
There isn't enough information otherwise for libltdl to know how to turn
"module" into "cygmodule-27.dll".  Accommodations for dlpreopen such as
Pierre wants, appear to me to work against the normal dlopen needs...
unless we subvert normal linker behavior on win32, and always turn
-dlpreopen foo into -Wl,-Bstatic foo -Wl,-Bdynamic (implying -dlpreopen
+ -disable-static is verboten, which is precisely what this patch is
trying to fix!)

Anyway, if we're going to try and nail down these aspects of the API, I
think that's a good thing to do for libltdl2 (whether Gary's or some
other brainstorm).

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

I did try it.  But I didn't use a variable, I did

   case john smith in
   john smith ) echo yes ;;
   esac

and got a syntax error. I didn't realize the behavior was different with
$vars vs. literals (or perhaps the difference is between interactive and
non-interactive shells).

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

Ack.

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

I'll just pass a regex, and update the documentation accordingly.

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

OK, I'll return to (take 7), subject to possibly ripping out a lot of it
depending on the answer to the Q above.

--
Chuck