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