RE: Libtool stresstest.at segfault on Cygwin/MinGW

[Peter Ekberg]

Ralf Wildenhues wrote on Monday, September 19, 2005 15:19 CEST:
> Hi Peter,

Hi!

> * Peter Ekberg wrote on Mon, Sep 19, 2005 at 12:11:23PM CEST:
> > * Peter Ekberg wrote on Friday, September 16, 2005 11:11 CEST:
> > > 
> > > In short, when you specify the symbols to export with
> > > "-export-symbols foo.sym", the file foo.sym is simply prepended
> > > with an EXPORTS line (if missing) and used as a .def input file
> > > for the linker. But when a .def file is specified, the auto-export
> > > magic is shorted out of the loop and the .def file is trusted
> > > blindly. So, the correct fix is that when the symfile given to
> > > libtool does not start with an EXPORTS line, the symbols in it has
> > > to be filtered and " DATA" should be appended to those symbols
> > > that are data symbols, probably similar to what is done when the
> > > -export-symbols-regex option is used.
> > 
> > And here's a patch to fix it. It doesn't work 100% on MinGW as
> > MinGW seems to put most const symbols in the .text segment, so
> > the nm output has T for some symbols where the nm output on
> > Cygwin has R.
> 
> Is this a linker bug?  Should we mention this on the mingw list?
> OTOH, if, for the symbols that mingw puts in .text, it also works
> fine without the DATA, I don't think this is an issue at all.
> Is that maybe the case?  (Note I don't *really* know what I'm talking
> about here, just stabbing in the dark..)

Well, the test segfaults on MinGW with the patch, and if I add
DATA to all symbols manually in asyms the (reordered) test goes
on until the same problem is triggered when export_symbols_cmds
is invoked because of the -export-symbols-regex option, so I
assume it is not good enough for MinGW. I think the import lib
gets screwed up if data symbols are not correctly tagged.

I also suspect that the variables are in .text by design, but
that could have changed. I'm not using the latest MinGW
tools. The question should be asked though, as it seems like a
rather silly thing to do, at least for a casual observer.

The patch makes exporting non-const data work though, which is
what I need...

> > But it seems to work OK on Cygwin so I think the
> > patch should still be considered. I have no clue as to how to
> > detect all symbols that are data symbols on MinGW. The symbols
> > in stresstest.at that are placed in the .text segment are v5,
> > v6, v8 and v12 (apart from the obvious function, v9). These
> > symbols are all const, The last const symbol (v7) isn't in the
> > .text segment on MinGW, that's probably for relocation issues.
> 
> I see that I need to hurry to extend stresstest so you don't
> run out of bugs to fix.  ;-)

:-)

> >     * libltdl/config/ltmain.m4sh (func_mode_link): Filter
> >     user supplied symfile to tag relevant symbols as data
> >     symbols. Fixes segfault in stresstest.at on Cygwin and
> >     makes the test pass.
> 
> Very clever idea, by the way, to reuse the already-present
> export_symbols_cmds machinery!

Thanks.

> > The patch should perhaps be backported to 1.5 as the bug is
> > most likely present there as well, but I don't use 1.5...
> 
> Yes, I agree.  Same thing with your other stresstest-induced fix.

I suppose you mean the one with exporting from self?

> Some comments to your patch (untested so far):
> 
> *snip first hunk*
> | @@ -4967,6 +4983,14 @@
> |  
> |     if test -n "$export_symbols" && test -n "$include_expsyms"; then
> |       $opt_dry_run || eval '$ECHO "X$include_expsyms" | 
> $Xsed | $SP2NL >> "$export_symbols"'
> | +   fi
> 
> Hmm. You set export_symbols to empty before, so in fact you kill the
> (little known, probably unused) include_expsyms feature here. 
>  Can't we
> have both?  By the way, your bug report also shows that 
> include_expsyms
> needs a similar treatment as your filter, otherwise one would 
> have to add
>   symbol,DATA
> to the include_expsyms list on cygwin, right?  Not a pressing issue
> though, this feature looks little-used.

Actually, I had that in mind when I wrote the patch. So,
include_expsyms isn't killed as I set up the variables so that
the big if statement is entered and in there somewhere there's
a new assignment of export_symbols. Perhaps I should add a test
for $orig_export_symbols in the big if instead? Whatever suits
you best, it's all the same to me...

> Good thing you bring include_expsyms up here: stresstest does 
> not cover
> this line.  Another thing to check. :)

Make sure to check it when exports are skipped due to command
line length too (there's a bug to fix). And also check
-export-symbols-regex when exports are skipped due to command
line length (oops, another one). I better shut up... :-)

But thinking further, my patch is also less than perfect if
exports are skipped due to command line length. It would
export all global symbols, instead of only those specified,
and that would be a regression for those only exporting
functions from a *large* number of objects. Crap.

> | +
> | +   if test "X$skipped_export" != "X:" && test -n 
> "$orig_export_symbols"; then
> | +     # The given exports_symbols file has to be filtered, 
> so filter it.
> | +     func_echo "filter symbol list for \`$libname.la' to 
> tag DATA exports"
> | +     $opt_dry_run || $SED -e '/[[ ,]]DATA/!d;s,\(.*\)\([[ 
> ,]].*\),s/^\1$/\1\2/,' < $export_symbols > 
> $output_objdir/$libname.filter
> 
> Please escape the separator (in this case, the bracketed comma) within
> patterns (this is a POSIX necessity), or use a separator not 
> present in
> the pattern; also, in light of the other recent bug report, it may be
> safer to use slashes as little as possible (shown without m4 quoting):
>   /[ ,]DATA/!d; s,\(.*\)\([ \,].*\),s|^\1$|\1\2|,

Ok, update attached (w/o fix for above mentioned regression).

> This surely needs to be tested for possible mingw path-translation or
> other issues (try by calling it with SED referring to a path component
> starting outside the msys tree).

Should I:

$ mkdir /c/foo
$ cp /bin/sed /c/foo
$ ./configure SED=/c/foo/sed

or what do you mean?

> | +     export_symbols=$output_objdir/$libname.def
> | +     $opt_dry_run || $SED -f 
> $output_objdir/$libname.filter < $orig_export_symbols > 
> $export_symbols
> |     fi
> |  
> |     tmp_deplibs=
> 
> Thank you for working on this!

You're welcome, thanks for the review!

Cheers,
Peter

head-filter-data-symbols-2.patch
Description: head-filter-data-symbols-2.patch