Re: Libtool stresstest.at segfault on Cygwin/MinGW

[Ralf Wildenhues]

Hi Peter,

* Peter Ekberg wrote on Mon, Sep 19, 2005 at 04:17:56PM CEST:
> Ralf Wildenhues wrote on Monday, September 19, 2005 15:19 CEST:
> > * Peter Ekberg wrote on Mon, Sep 19, 2005 at 12:11:23PM CEST:
> > > 
> > > 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.

OK.  I assume it's not a linker bug then.

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

OK.

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

Surely non-constant data are more important.  But you do realize that
exporting data objects should be avoided if possible?  ;-)

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

Yes.

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

But then include_expsyms is appended to export_symbols.  That in turn is
used to build the filter.  The filter is in turn used on
orig_export_symbols, which does not contain the include_expsyms.  Right?
Looks wrong to me.  I believe you want to add the include_expsyms to
orig_export_symbols instead of export_symbols.

Please note all of above babble is from reading alone, I will eventually
test this, but it might take a while.

Also, while thinking about it, please provide an empty default value
for orig_export_symbols right before the first hunk of your patch; that
way it's clear this is a locally-used variable.

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

Somewhere, a helpful comment would be good.  This section of the code
contains loads of intertwined magic (combined with the settings of
export_symbols_cmds in libtool.m4).  If only to keep this from being
broken by the next casual patch, stresstest needs to be extended to
cover all (un)likely cases.

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

I noted that, too.  But thanks for reminding, I added them to the TODO
list.  Erm, did I mention I take patches for all of these issues.  ;-)

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

Your patch is better than what we had before.  We should fix the
piecewise linking ones separately.  But if you want to improve on
the patch first, go right ahead.

Note also there was once a plan to kill the quadratic scaling behavior
in the piecewise linking case..

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

OK.

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

Something like this.  I'd start less drastic, though, by only replacing
SED in the created `libtool', or you might be finding more issues than
you like.  OTOH, comments from mingw people indicate this should not
happen in a good setup, so maybe it's not important after all.  We
really don't need to support people who break their setups too much.

Cheers,
Ralf