RE: MSYS+MSVC for libtool branch-2-0, take 7

[Peter Ekberg]

Ralf Wildenhues wrote:
> * Peter Ekberg wrote on Thu, Aug 11, 2005 at 11:38:59AM CEST:
> > 
> > Anyway, here's testsuite.log and I configured with:
> > ../configure CC=cl CFLAGS=-MD CXX=cl CXXFLAGS=-MD STRIP=: RANLIB=:
> > F77=no FC=no NM="dumpbin -symbols" AR=lib LD=link
> 
> OK, how about this suggestion:
> 
> After addressing my comments below, if you feel comfortable, please go
> ahead and check this patch (or split-up patches) in for HEAD. 
>  That way
> it may be easier for other people to test, too.  After the testsuite
> failures have been ironed out, and it has been used to successfully
> build a few real-world packages, we put a backport in 
> branch-2-0 (I can
> help with backporting, if you like).  If HEAD turns up severely broken
> with this patch, we can still revert it later.
> 
> Would that be ok with you?  Does one of the other maintainers
> disapprove?

Certainly ok, but my ultimate goal is 2.0. Does the 72 hour rule
apply here? Anyway, I intend to split the patch up and post the
parts one more time with all the bits from this message plus one
or two cleanups.

> Missing bits and noted to the patch:
> 
> A ChangeLog entry.  :)
> (be sure to note all new TAGVARs)
> 
> Please add a NEWS entry stating much improved but still experimental
> support for MSVC.
> 
> Please also note that lt_error_list suffers from the same failure as
> preloaded_symbols.  I have not checked how a possible fix should look
> like (this applies to mingw/gcc as well).
> 
> All over the patch, you sometimes use 'sed' instead of '$SED'.
> In places where you cannot control the contents of stdin, you should
> change that to $SED.

Is it ok to replace sed with $SED everywhere (only for the stuff I
touch obviously)?

> 
> config/ltmain.m4sh:
> 
> | @@ -5009,7 +5079,35 @@
> |           fi
> |         done
> |         IFS="$save_ifs"
> | -       if test -n "$export_symbols_regex"; then
> | +       case $host_os/$with_gcc/$skipped_export in
> | +       mingw*/no/: | mingw*//: | cygwin*/no/: | cygwin*//:)
> | +         # Assume MSVC _and_ lib archiver interface...
> | +         # Build a lib using a command file and export symbols from
> | +         # the lib instead of from the objects individually.
> | +         skipped_export=false
> | +         :>$output_objdir/$libname.symlibcmd
> | +         for obj in $libobjs; do
> | +           $ECHO \""$obj"\" >> $output_objdir/$libname.symlibcmd
> | +         done
> 
> Please use only one redirection here, it should be faster:
> 
> skipped_export=false
> for obj in $libobjs; do
>   $ECHO "\"$obj\""
> done >"$output_objdir/$libname.symlibcmd"
> 
> Similarly for file archive linking.
> (The only thing one has to remember with redirected loops is that very
> old shells might fork and thus variable assignments inside will not
> propagate.)

Ok.

> | +         $RM $output_objdir/$libname.symlib
> | +         func_show_eval '$AR $AR_FLAGS 
> ${AR_OFLAGS}$output_objdir/$libname.symlib 
> @$output_objdir/$libname.symlibcmd'
> | +         save_libobjs="$libobjs"
> | +         libobjs="$output_objdir/$libname.symlib"
> | +         # Rerun export_symbols_cmds
> | +         $run $RM $export_symbols
> 
> This:
> | +         cmds=$export_symbols_cmds
> | +         save_ifs="$IFS"; IFS='~'
> | +         for cmd in $cmds; do
> | +           IFS="$save_ifs"
> | +           eval cmd=\"$cmd\"
> | +           func_show_eval "$cmd" 'exit $?'
> | +         done
> | +         IFS="$save_ifs"
> 
> Can be reduced to
>   func_execute_cmds "$export_symbols_cmds" 'exit $?'

Ok, that one was hard to get right at the time of my post though :-)

> | +         libobjs="$save_libobjs"
> | +         ;;
> | +       esac
> | +       # No use to run $export_symbols_regex if exports 
> are skipped...
> | +       if test "X$skipped_export" != "X:" && test -n 
> "$export_symbols_regex"; then
> |           func_show_eval '$EGREP -e "$export_symbols_regex" 
> "$export_symbols" > "${export_symbols}T"'
> |           func_show_eval '$MV "${export_symbols}T" 
> "$export_symbols"'
> |         fi
> | 
> 
> 
> 
> | @@ -6074,7 +6204,11 @@
> |       # are only useful if you want to execute the "real" binary.
> |       # Since the "real" binary is built for $host, then this
> |       # wrapper might as well be built for $host, too.
> | -     $opt_dry_run || $LTCC $LTCFLAGS -s -o $cwrapper 
> $cwrappersource
> | +     if test "$with_gcc" = "yes"; then
> | +       $opt_dry_run || $LTCC $LTCFLAGS -s -o $cwrapper 
> $cwrappersource
> | +     else
> | +       $opt_dry_run || $LTCC $LTCFLAGS -o $cwrapper $cwrappersource
> | +     fi
> |       ;;
> |     esac
> |     $RM $output
> 
> Does the following work too?  Should we not use it then?
> (Or is there a severe performance penalty involved for gcc?)
> 
> $opt_dry_run || {
>    $opt_dry_run || $LTCC $LTCFLAGS -o $cwrapper $cwrappersource
>    $STRIP $cwrapper
> }

Yes, works for MSVC, MinGW and Cygwin.

> m4/libtool.m4:
> 
> | @@ -2840,7 +2946,8 @@
> |  m4_require([_LT_DECL_EGREP])dnl
> |  
> |  # Check for command to grab the raw symbol name followed 
> by C symbol from nm.
> | -AC_MSG_CHECKING([command to parse $NM output from 
> $compiler object])
> | +# $compiler is not yet set, fall back to $CC.
> | +AC_MSG_CHECKING([command to parse $NM output from $CC object])
> |  AC_CACHE_VAL([lt_cv_sys_global_symbol_pipe],
> |  [
> |  # These are sane defaults that work on at least a few old systems.
> 
> Why is this change needed?  AFAICS, the output should be identical.

Hmmm, $compiler is empty if I "./configure CC=cl", but not if I
just "./configure" or if I "./configure CC=gcc". Something is
missing in the non-gcc case.
Anyway, I'll drop that change, it is pure cosmetics...

> old test suite changes:
> 
> | --- tests/demo/foo.h        22 Apr 2005 10:10:31 -0000      1.2
> | +++ tests/demo/foo.h        11 Aug 2005 08:50:21 -0000
> | @@ -33,6 +33,12 @@
> |  #  endif
> |  #endif
> |  
> | +#ifdef _MSC_VER
> | +# define EXTERN extern __declspec(dllimport)
> | +#else
> | +# define EXTERN extern
> | +#endif
> | +
> |  /* __BEGIN_DECLS should be used at the beginning of your 
> declarations,
> |     so that C++ compilers don't mangle their names.  Use 
> __END_DECLS at
> |     the end of C declarations. */
> 
> I don't like this.  I also don't think it is correct.
> Look at tests/pdemo/foo.h for other occurrences of EXTERN, which I
> don't like either.  Try to make the definitions match.
> 
> This is something we need to get right once (maybe in combination with
> Automake), and then provide a portable way to users.  Same with the
> mdemo2 failure.  You could think about applying the patch 
> without these
> old testsuite changes for now.

I'll contribute with what I know. The problem is with variable
imports. Importing and exporting functions works as expected, and
exporting variables do not do any magic things to the symbols
as is the case with variable imports.

One problem is that the importing variable magic is different
in gcc and in MSVC.

First the gcc case. When gcc finds
        __declspec(dllimport) int foo;
it will only link if there is a __imp__foo symbol.
So, if some code should link with both the static and the
shared version of a lib providing foo, it will have to be
compiled twice. Once so that the __declspec is defined
to nothing for static linking, otherwise __imp__foo is not
found. And once with the __declspec in place for shared
linking, otherwise the symbol is not imported properly.

Or, as an option, you could do as Howard Chu suggested
in April and rely on the automatic import feature, in
which case you should not have any __declspec and the
same object can by used to both link with a static and
a shared lib providing foo. I.e. with automatic import
it works as you'd like it to work.

For MSVC, there is no magic automatic import feature
that solves the problem. But, if you have the above
__declspec and link with a static lib providing foo,
the linker will still find foo (issuing a warning,
that's all). Also, the compiler will do the right
thing if it finds both a dllimport declaration
and a local definition (again issuing a warning).

Also, in that April thread, Bob Friesenhahn mentioned
that the declarations in all translation units should be
consistent. The key here is that all *declarations*
should be consistent, the definitions should IMHO
never have any of this declspec crap (it's perhaps
useful for C++, but that's a different story).

Now, you *could* add __declspec(dllexport) to all
symbols you'd like to export from a library, but I
would take care to again *only* use the dllexport
on declarations, and to do that you have to use
extern in conjunction with the dllexport, otherwise
it is assumed that the dllexport is a definition.
I.e. "__declspec(dllexport) int foo;" is always a
definition, but "extern __declspec(dllexport) int foo;"
is a declaration. At least this applies to MSVC, but
since changing LT_SCOPE to extern (LT_SCOPE was in
that case defined as __declspec(dllexport)) on an
intended declaration solved the same bug for MinGW
as it did for MSVC, I'm willing to bet that the same
holds true for gcc. However, there are other means to
selectivly export stuff, so the use of dllexport is
optional.

And on a tangent, the use of the DLL_EXPORT macro in the
header of library A is dangerous when you build library B
that depends on library A. When you build library B,
libtool also defines DLL_EXPORT and you end up with the
wrong __declspec from library A. Ergo, don't use DLL_EXPORT
To select what declspec you should apply. The right thing
to do it is to define something like BUILDING_LIB_A during
build of library A and use that to get the right declspec
(or rely on gcc to autoimport and MSVC to do the fixup of
imported data that should not be imported when linking
statically, as discussed above).

For compilers other that gcc and MSVC, I'm in the dark.

Note, I'm not dead certain that this is all 100% correct,
but it fully matches my experience to this date with the
tools in question.

> Notes to the testsuite output:
> stresstest:
> | libtool: link: creating main
> | lt-main.c
> | ./.libs/lt-main.c(98) : warning C4002: too many actual 
> parameters for macro 'DEBUG'
> | ./.libs/lt-main.c(99) : warning C4002: too many actual 
> parameters for macro 'DEBUG'
> | ./.libs/lt-main.c(105) : warning C4002: too many actual 
> parameters for macro 'DEBUG'
> | ./.libs/lt-main.c(115) : warning C4002: too many actual 
> parameters for macro 'DEBUG'
> | ./.libs/lt-main.c(167) : warning C4002: too many actual 
> parameters for macro 'DEBUG'
> | ./.libs/lt-main.c(200) : warning C4002: too many actual 
> parameters for macro 'DEBUG'
> | ./.libs/lt-main.c(252) : warning C4047: '==' : 'int ' 
> differs in levels of indirection from 'void *'
> | ./.libs/lt-main.c(275) : warning C4047: '==' : 'int ' 
> differs in levels of indirection from 'void *'
> 
> This looks like your change to the cwrappersource in ltmain 
> somehow did
> not work out correctly.  Could you look into this?

I'll do as you asked in your follow-up message.

> Independently, you changed some preprocessor constructs there -- could
> you fix the indentation like this?
> #if foo
> # bar
> #else
> # if other
> #  baz
> # endif
> #endif

Consider it done.

> The following, however, is probably the failure which also happens
> on mingw/gcc and cygwin:
> 
> | dlself.def : error LNK2001: unresolved external symbol w1$
> | dlself.def : error LNK2001: unresolved external symbol w10$

Ok, it looked so weird that I didn't even bother to look...

> Cheers, and thanks for all your work,
> Ralf
>