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

[Ralf Wildenhues]

Hello Charles,

* Charles Wilson wrote on Mon, Jun 28, 2010 at 12:05:40AM CEST:
> On 6/27/2010 4:43 PM, Ralf Wildenhues wrote:
> > * Charles Wilson wrote on Sun, Jun 27, 2010 at 02:51:21PM CEST:
> >> 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.

I wasn't aware of that.  Sorry about the sloppy review.

>  It obviously isn't
> SUPPOSED to be dead -- or it wouldn't be there.

Well, I wouldn't put my money on that reasoning.

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

I'd say the part that is easier for you, so I guess that would be
committing all the code, including presumably-dead.  And maybe in a
future patch adding a testsuite test that exercises the code.

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

I think you can measure parse time in script length plus number of
here-documents (for old shells).

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

I would like to apologize for this comment making you do this extra
work.  Again, that review of mine was more sloppy than it should have
been.

> 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

Of course code maintenance aspects and long-term slowdown of the code
are a part of code quality.

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

Again, sorry for making you do extra work.

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

Yep, I guess.

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

Yes, the literal you tried is already split into words, so word
splitting of the shell would not make any difference.

And yes, it may happen that I spend a large amount of time trying to
review patches well.  I didn't spend so much time this time, i.e., I
did glance at the 6 takes and discussion threads of this patch
beforehand, but not in large detail.  You can't have both, good review
and timely review.  I'm sorry if review is painful to accept, and I
don't on purpose try to review in a way making you do double work.
That that has happened now is bad, sorry about that.

I think one fundamental reason why the w32 stuff languishes so long is
that neither of the maintainers are highly interested in w32.  That
tends to get it a hit on priority wrt. other things, and since there are
always many other things to do, that can be a problem.  Is that fair?
No, it isn't.  But it happens nonetheless.  Does that mean we might be
overly critical on w32 patches?  Maybe, for me I can't always reject
that notion (backed by the GCS btw), esp. during times when I'm
essentially the only person doing review.  That's nothing personal, but
it turns out to be a problem nonetheless.  I reject the notion of being
the only Libtool maintainer, and I'd rather step down than just open the
floodgate for code that hasn't been properly reviewed or isn't in good
shape.  Somebody else needs to take responsibility for that.

On the brighter side, haven't we made at least some good progress in the
last few weeks?

Cheers,
Ralf