Ralf Wildenhues wrote:
>> +# func_tr_sh
>> +# turn $1 into a string suitable for a shell variable name
>> +# result is stored in $func_tr_sh_result
>> +func_tr_sh ()
>> +{
>> + func_tr_sh_result=`echo "$1" | $SED -e 's/[^A-Za-z0-9_]/_/g'`
>> + # ensure result begins with non-digit
>> + case "$func_tr_sh_result" in
>> + [A-Za-z_][A-Za-z0-9_] ) ;;
>> + * ) func_tr_sh_result=_$func_tr_sh_result ;;
>> + esac
>> +}
>> ]])
>
> Let's not waste processes when we don't have to, with something like
> this untested bit:
>
> func_tr_sh ()
> {
> case $1 in
> [!a-zA-Z_]* | *[!a-zA-Z_0-9]*)
> func_tr_sh_result=`$ECHO "$1" | $SED 's/^[^a-zA-Z]/_/;
> s/[^a-zA-Z0-9]/_/g'`
> ;;
> *)
> func_tr_sh_result=$1
> ;;
> esac
> }
Your version will confuse '1dumblibraryname.a' and '2dumblibraryname.a'
by turning both into '_dumblibraryname_a'. How about this:
# func_tr_sh
# turn $1 into a string suitable for a shell variable name
# result is stored in $func_tr_sh_result. All characters
# not in the set a-zA-z0-9_ are replaced with '_'. Further,
# if $1 begins with a digit, a '_' is prepended as well.
func_tr_sh ()
{
case "$1" in
[0-9]* | *[!a-zA-Z_0-9]*)
func_tr_sh_result=`$ECHO "$1" | $SED 's/^\([0-9]\)/_\1/;
s/[^A-Za-z0-9]/_/g'`
;;
* )
func_tr_sh_result=$1
;;
esac
}
Which makes it clear exactly what we're trying to do:
1) replace all "bad" chars with _
2) prepend _ if $1 begins with a digit
There is still a slight inefficiency in the above: IF $1 has all valid
characters, but happens to begin with a digit, then we fork a sed simply
to prepend the '_'. I doubt this will occur much. or ever -- and if it
does, the penalty is an extra fork, not wrong behavior.
>> --- a/libltdl/config/ltmain.m4sh
>> +++ b/libltdl/config/ltmain.m4sh
>
>> @@ -1988,7 +1988,7 @@ extern \"C\" {
>> eval '$GREP -f "$output_objdir/$outputname.exp" < "$nlist" >
>> "$nlist"T'
>> eval '$MV "$nlist"T "$nlist"'
>> case $host in
>> - *cygwin | *mingw* | *cegcc* )
>> + *cygwin* | *mingw* | *cegcc* )
>> eval "echo EXPORTS "'> "$output_objdir/$outputname.def"'
>> eval 'cat "$nlist" >> "$output_objdir/$outputname.def"'
>> ;;
>
> Is this fixing a bug? If yes, then it should be a separate patch,
> documented in the ChangeLog entry, done likely in all other such
> instances of missing '*' (I haven't found any), and would be obviously
> correct and ok to push. Please, please don't mix heavy patches with
> such cleanups. It only leads to cleanups being delayed.
It is not a bug fix, exactly. I just noticed the lack of symmetry, AND
that '*cygwin' never appears anywhere else, and figured it was a typo.
The actual cygwin $host patterns we see AFAIK all match *cygwin -- but
by convention we (libtool) allow "extensions" on triples for variant
$hosts. This violated that convention, but wasn't /exactly/ a bug.
I can prepare a separate patch and push, if you prefer.
> In your "take 3" of this patch series, you have this hunk:
>
> | @@ -2217,7 +2217,7 @@ func_win32_libid ()
> | ;;
> | *ar\ archive*) # could be an import, or static
> | if eval $OBJDUMP -f $1 | $SED -e '10q' 2>/dev/null |
> | - $EGREP 'file format (pe-i386(.*architecture:
> i386)?|pe-arm-wince|pe-x86-64)' >/dev/null; then
> | + $EGREP 'file format (pei?-i386(.*architecture:
> i386)?|pe-arm-wince|pe-x86-64)' >/dev/null; then
> | win32_nmres=`eval $NM -f posix -A $1 |
> | $SED -n -e '
> | 1,100{
>
> Now, my memory is really bad about win32 semantics, but wasn't it
> exactly pei-i386 libraries that we wanted to not match here?
Originally (before the introduction of [func_]win32_libid()), we had
pei*-i386:
cygwin* | mingw* | pw32*)
lt_cv_deplibs_check_method='file_magic file format
pei*-i386(.*architecture: i386)?'
When win32_libid() was first introduced, we moved the pei*-i386
incantation as-is into win32_libid():
grep -E 'file format pei*-i386(.*architecture: i386)?' >/dev/null ; then
in 6da15e03aa1127eb42652a1f7e15ee42633dbfdf Thu Oct 31 00:52:39 2002
This was changed to
grep -E 'file format pe-i386(.*architecture: i386)?' >/dev/null ; then
in 709bbb17317c67d28cf7ec8f0baaef16c4137ad0 Mon Feb 17 18:55:45 2003
Supposedly, this was part of a "rewrite to improve speed". Looking at
the mailing list history from that era:
http://lists.gnu.org/archive/html/libtool-patches/2003-02/msg00048.html
No explanation was given for that particular change (my fault).
Looking at the original version of win32_libid() -- after 6da15e03 but
before 709bbb17 -- it originally did this:
if eval $OBJDUMP -f $1 2>/dev/null | \
grep -E 'file format pei+-i386(.*architecture: i386)?' >/dev/null ;
then
win32_libid_type="x86 DLL"
else
if eval $OBJDUMP -f $1 2>/dev/null | \
grep -E 'file format pei*-i386(.*architecture: i386)?' >/dev/null
then
win32_libid_type="x86"
if eval file $1 2>/dev/null | \
grep -E 'ar archive' >/dev/null; then
win32_libid_type="$win32_libid_type archive"
if eval $NM -f posix -A $1 | awk '{print $3}' | grep "I"
>/dev/null ;
That is, objdump was the "primary" filter, and if it reported pei-i386,
then we assumed x86 DLL (even if it were actually an EXE -- or, as I now
discover, an MS SDK import library). However, if it reported pe[i]-i386
-- which is to say, pe-i386, because pei-i386 was already "taken" by the
other branch of the if statement -- then delegate to file and nm for
further analysis to determine if it is a ar archive, and if so, if it is
static or import.
This was clearly wrong. And slow.
The new version (after 709bbb17) used file as the "primary" filter. But,
observing the issue outlined above, when analyzing non-DLL, I removed
the 'i*' part from the objdump filter, now that it was the secondary
filter. This was (apparently) wrong, because static and imp libs can
contain pei-i386 or pe-i386 code.
Now, my original understanding of the difference between the two was
(from binutils/bfd/peicode.h):
The *sole* difference between the pe format and the pei format is
that the
latter has an MSDOS 2.0 .exe header on the front that prints the message
"This app must be run under Windows." (or some such).
(FIXME: Whether that statement is *really* true or not is unknown.
Are there more subtle differences between pe and pei formats?
For now assume there aren't. If you find one, then for God sakes
document it here!)
Now, that told me (back in the 6da15e03 era) that only a linked object
-- e.g. an EXE or a DLL -- could really be "pei"; after all, how could a
.o (or .a) contain "an MSDOS 2.0 .exe header"?. However, the MS Windows
SDK import libraries, when analyzed by objdump, do report pei, not pe.
And they are certainly import libs, and not DLLs or EXEs. So...the
original (pre-6da15e03) version of the objdump filter was correct:
pei*-i386 (or, equivalently, pei?-i386).
> More generally, I have a feeling that this function is badly
> conditioned: it needs adjustment fairly often, it is unclear to me which
> cases exactly it tries to exclude (for starters: why is the file format
> test needed at all?), and when things fail here, they do so very
> unobviously for the libtool user.
Originally, the objdump filter was used to exclude any archives that
were not "win32" -- we wouldn't want somebody compiling for $host
mingw/cygwin/etc to think it was OK to link against a linux static lib
or some other archive that wasn't a *windows* (cygwin/mingw/...) static
or import.
> These issues could IMVHO be
> ameliorated by having some test cases that show the fine line between
> import libraries that are acceptable, and static ones that are not.
Err, but the one line we're worried about here, is NOT trying to
distinguish between implib and static lib. It's trying to distinguish
between an ar archive that holds code for cygwin/mingw/*, vs. an ar
archive that holds some other kind of code. The identification of static
vs. implib is done by the $NM/sed magic, not the $OBJDUMP/grep stuff
we're discussing here. (Don't confuse func_win32_libid with the sed
magic gobbledygook in this patch's new func_cygming_dll_for_implib_core
function; in the latter case we already know we have a cygming import
library, and are trying to parse its contents)
Maybe -- MAYBE -- it's enough to assume that "if $file_magic_cmd is
func_win32_libid" (that is, we're inside the func_win32_libid function),
then any ar archive we see will contain win32 (mingw/cygwin/...) code,
and not sparc or mips or linux-i386 code. But isn't it better to be sure?
> So that when we port this to the next w32 system, we just have to run
> the test suite and fix it until it passes. What do you think?
So you want to include binary files in the test suite? Here are various
"good" dlls, exes, implibs, and static libs, here are various "bad"
ones? Multiply times mingw, cygwin, msvc, pw32, interix, then multiply
that by 64bit? (FYI: the SDK implibs that I'm trying to work with here
are not redistributable, but are freely available from MS. So we can't
include them in any such test, anyway).
I think you're overstating how often this function is tweaked: it gets
modified in two cases: 1) when we bodge in support for new platforms --
which is to be expected when you overload the same function for all
PE-related platforms. Maybe those other $hosts should have their OWN
func_mingw64_libid() rather than everybody glomming on to the misnamed
func_win32_libid() [*]? and 2) when we screw up. Test cases would help
with that -- but we never tried to allow cygwin/mingw to "work with"
ms-provided implibs before. That grew out of discussions around the
msvc-native branch, and talks with Danny on and off the binutils list.
So, we wouldn't have had a test case for this anyway.
[*] That is, the pe-arm-wince and pe-x86-64 stuff
$EGREP 'file format (pei?-i386(.*architecture:
i386)?|pe-arm-wince|pe-x86-64)'
was a mistake, and THAT was the time we should have renamed
func_win32_libid to func_cygming_libid, and created new func_wince_libid
and func_mingw64_libid functions -- that way, func_cygming_libid would
properly return "unknown" for libraries belonging to the other variants,
which it really doesn't know how to deal with anyway. But any of that
stuff is beyond the scope of this patch, and consideration of this patch
should not depend on resolving that issue.
--
Chuck
