Re: mdemo ltdl failure
Hi Charles,
* Charles Wilson wrote on Thu, Apr 19, 2007 at 03:02:08AM CEST:
>
> This is because the test is just too ugly for words, not to mention
> brittle. Trying to tease out malloc issues outside of a dedicated malloc
> testsuite is just plain silly.
I think the biggest problem with the previous patch was that it was
relying so much on undefined behavior, that is not only undefined by any
relevant standard, but also likely to be explicitly treated in very
unpredictable ways by different systems/compilers.
>> TESTING:
[...]
> Re-ran all of these tests under the specified conditions. Same results as
> the original patch.
Thanks.
>> (4) linux (whose system argz is OK)
>> --detects success, builds using system argz, works.
>> (5) linux, but with $lt_cv_sys_argz_works=no.
>> --doesn't run the test, and builds using libltdl argz. works.
>> (6) mingw, which doesn't have any system argz facility at all
>> --the test is not run
>> --builds successfully with libltdl's argz
>
> Did not run these tests. configury and sourcecode paths unchanged from
> original path so these should obviously still work. But I'll retest if
> desired.
It's a good choice of testing, and should be done again with the final
patch. Plus one test on Solaris with its /bin/sh. (Just noting this,
I can probably do these tests then.)
Some comments to the patch:
> --- libltdl/m4/argz.m425 Mar 2006 11:05:02 - 1.3
> +++ libltdl/m4/argz.m417 Mar 2007 06:09:50 -
[...]
> +os_ver=$(uname -r | $SED -e 's,^\([[0123456789\.]]*\).*,\1,')
> +os_major=${os_ver%%\.*}
> +os_micro=${os_ver##*\.}
> +os_minor=${os_ver%\.*}
> +os_minor=${os_minor##*\.}
You can not use the $(cmd) and the ${var%%pattern} constructs, Solaris'
shell will barf when parsing the code (i.e., even if it's inside a
conditional branch that is not executed). Use `cmd` and sed or expr
instead, see the Autoconf manual's chapter on portable shell.
> +AS_IF([test "$os_major" -gt "1"],[lt_cv_sys_argz_works=yes],dnl
> + [test "$os_minor" -gt "5"],[lt_cv_sys_argz_works=yes],dnl
> + [test "$os_micro" -gt "24"],[lt_cv_sys_argz_works=yes],dnl
No need for double quoting the literal integers. And if it can happen
that $os_micro is the empty string (which I'll consider quite likely),
then double quoting isn't enough precaution against error output, as
"" isn't interpreted by test as a number.
BTW, what are all the dnl's here for? If they're necessary in some way,
that would indicate that AS_IF had a bug. Ahh, that reminds me, AS_IF
has only been made n-ary in 2.60, and we intend to support 2.59. Please
use plain `if..else' or nested AS_IF instead, thanks.
Cheers, and thank you for your work,
Ralf
Re: [cygwin, libtool] use shell function to emit wrapper scripts and wrapper.exe source
* Charles Wilson wrote on Thu, Apr 19, 2007 at 06:13:19AM CEST: > > Test results -- new tests. Unexpected failures: > 14: Java convenience archives FAILED (convenience.at:273) > 16: Link order of deplibs. FAILED (link-order2.at:129) > 49: Run tests with low max_cmd_len FAILED (cmdline_wrap.at:42) > are actually not unexpected on cygwin. And certainly have nothing to do > with this patch. > > Actually, I think this is may be the best test conformance I've ever seen > on cygwin... I think I saw the new testsuite passing on Cygwin at one point. Could you post tests/testsuite.log (gzipped if it's large)? Thanks, Ralf
Re: [cygwin, libtool] use shell function to emit wrapper scripts and wrapper.exe source
* Charles Wilson wrote on Thu, Apr 19, 2007 at 03:55:20AM CEST: > [Added libtool-patches to CC list. Discussion of this patch > should probably drop libtool and cygwin] Done. > Okay, here's the first bit. It's pretty simple. Testing is in progress (and > in conjuction with the new argz fix I just posted to libtool-patches), but > looks good so far: the "new" wrapper scripts are identical to "old" ones > generated without this patch. Thanks. Please resubmit with the new functions moved to right before func_mode_link (or func_mode_install/func_mode_execute, in case you intend to use them from inside them in a followup patch); there's a real performance benefit to doing this for compile mode, as it saves the shell from having to parse the functions in that case. It may be lost in the fork/exec overhead on w32, but on GNU/Linux, libtool compile mode overhead is dominated by shell parsing (see the 2007-02-11 changes for a 30% execution time reduction this way). Also, please use round parentheses as per GCS for referring to functions in the ChangeLog entry. I'll apply the reposted patch then. Cheers, and thanks again, Ralf > 2007-04-18 Charles Wilson <[EMAIL PROTECTED]> > > * libltdl/config/ltmain.m4sh [func_mode_link]: move wrapper > script writing from here... > [func_emit_libtool_wrapper_script]: to this new function, and > write to stdout > [func_mode_link]: move cwrapper source code writing from here... > [func_emit_libtool_cwrapperexe_source]: to this new function, > and write to stdout > [func_mode_link]: call the two new functions and redirect to > appropriate file. >
Re: mdemo ltdl failure
Ralf Wildenhues wrote:
* Charles Wilson wrote:
This is because the test is just too ugly for words, not to mention
brittle. Trying to tease out malloc issues outside of a dedicated malloc
testsuite is just plain silly.
I think the biggest problem with the previous patch was that it was
relying so much on undefined behavior, that is not only undefined by any
relevant standard, but also likely to be explicitly treated in very
unpredictable ways by different systems/compilers.
Yes, I knew it was, which was why all "error" conditions in the test
resulted in the test reporting *success*. I was hoping during
discussion that somebody would have a better suggestion than "punt and
use version numbers and explicit OS hacks, even tho that is not the
Autoconf Way". But nobody commented on the patch until I raised it in
private correspondence with BobF. Now, I just don't think there IS any
way of actually detecting the error that isn't horrifyingly ugly.
And I felt really dirty after posting the original patch. :-)
(4) linux (whose system argz is OK)
--detects success, builds using system argz, works.
(5) linux, but with $lt_cv_sys_argz_works=no.
--doesn't run the test, and builds using libltdl argz. works.
(6) mingw, which doesn't have any system argz facility at all
--the test is not run
--builds successfully with libltdl's argz
Did not run these tests. configury and sourcecode paths unchanged from
original path so these should obviously still work. But I'll retest if
desired.
It's a good choice of testing, and should be done again with the final
patch. Plus one test on Solaris with its /bin/sh. (Just noting this,
I can probably do these tests then.)
will do for linux & mingw, but have no unencumbered access to solaris at
present.
Some comments to the patch:
--- libltdl/m4/argz.m4 25 Mar 2006 11:05:02 - 1.3
+++ libltdl/m4/argz.m4 17 Mar 2007 06:09:50 -
[...]
+os_ver=$(uname -r | $SED -e 's,^\([[0123456789\.]]*\).*,\1,')
+os_major=${os_ver%%\.*}
+os_micro=${os_ver##*\.}
+os_minor=${os_ver%\.*}
+os_minor=${os_minor##*\.}
You can not use the $(cmd) and the ${var%%pattern} constructs,
[EMAIL PROTECTED] and I checked to make sure those were fully supported by Posix
Shell and weren't just convenient bashisms.
Solaris'
shell will barf when parsing the code (i.e., even if it's inside a
conditional branch that is not executed). Use `cmd` and sed or expr
instead, see the Autoconf manual's chapter on portable shell.
Geez, solaris sux. You think they might consider shipping a posix
compliant /bin/sh sometime this century?
+AS_IF([test "$os_major" -gt "1"],[lt_cv_sys_argz_works=yes],dnl
+ [test "$os_minor" -gt "5"],[lt_cv_sys_argz_works=yes],dnl
+ [test "$os_micro" -gt "24"],[lt_cv_sys_argz_works=yes],dnl
No need for double quoting the literal integers. And if it can happen
that $os_micro is the empty string (which I'll consider quite likely),
then double quoting isn't enough precaution against error output, as
"" isn't interpreted by test as a number.
Okay, but actually, all cygwin releases are guaranteed to have non-empty
major, minor, and micro numbers -- and we're inside a $host_os == cygwin
block, here. If I update the os_* variable assignments with protection
against empty (default to "0"?), I'll move it outside the case block so
that if any other "bad" systems are discovered they can be added more
easily -- although I doubt that there are any.
BTW, what are all the dnl's here for? If they're necessary in some way,
that would indicate that AS_IF had a bug.
Habit. I'll remove 'em all and see what breaks.
Ahh, that reminds me, AS_IF
has only been made n-ary in 2.60, and we intend to support 2.59. Please
use plain `if..else' or nested AS_IF instead, thanks.
That'll teach me to read the manual. I didn't realize it was n-ary until
I did so, and thought: Cool! Never realizing I was reading the 2.61
manual...
--
Chuck
Re: [cygwin, libtool] use shell function to emit wrapper scripts and wrapper.exe source
Ralf Wildenhues wrote: * Charles Wilson wrote: Test results -- new tests. Unexpected failures: 14: Java convenience archives FAILED (convenience.at:273) 16: Link order of deplibs. FAILED (link-order2.at:129) 49: Run tests with low max_cmd_len FAILED (cmdline_wrap.at:42) are actually not unexpected on cygwin. And certainly have nothing to do with this patch. Actually, I think this is may be the best test conformance I've ever seen on cygwin... I think I saw the new testsuite passing on Cygwin at one point. ??? that would surprise me. Could you post tests/testsuite.log (gzipped if it's large)? attached. -- Chuck testsuite.log.gz Description: GNU Zip compressed data
Re: mdemo ltdl failure
* Charles Wilson wrote on Thu, Apr 19, 2007 at 08:08:41PM CEST: > Ralf Wildenhues wrote: > > > >It's a good choice of testing, and should be done again with the final > >patch. Plus one test on Solaris with its /bin/sh. (Just noting this, > >I can probably do these tests then.) > > will do for linux & mingw, but have no unencumbered access to solaris at > present. I'll do that then. > >>+AS_IF([test "$os_major" -gt > >>"1"],[lt_cv_sys_argz_works=yes],dnl > >>+ [test "$os_minor" -gt > >>"5"],[lt_cv_sys_argz_works=yes],dnl > >>+ [test "$os_micro" -gt > >>"24"],[lt_cv_sys_argz_works=yes],dnl > > > >No need for double quoting the literal integers. And if it can happen > >that $os_micro is the empty string (which I'll consider quite likely), > >then double quoting isn't enough precaution against error output, as > >"" isn't interpreted by test as a number. > > Okay, but actually, all cygwin releases are guaranteed to have non-empty > major, minor, and micro numbers -- and we're inside a $host_os == cygwin > block, here. Oh, I overlooked this fact in my review. Ignore that comment then. Cheers, Ralf
Re: mdemo ltdl failure
* Charles Wilson wrote on Thu, Apr 19, 2007 at 08:08:41PM CEST:
> >
> >>--- libltdl/m4/argz.m4 25 Mar 2006 11:05:02 - 1.3
> >>+++ libltdl/m4/argz.m4 17 Mar 2007 06:09:50 -
> >[...]
> >>+os_ver=$(uname -r | $SED -e 's,^\([[0123456789\.]]*\).*,\1,')
> >>+os_major=${os_ver%%\.*}
> >>+os_micro=${os_ver##*\.}
> >>+os_minor=${os_ver%\.*}
> >>+os_minor=${os_minor##*\.}
One more thing: please use variables prefixed with lt_, or with gl_.
Also I think the range [0-9] is portable sed.
Cheers,
Ralf
Re: [cygwin, libtool] use shell function to emit wrapper scripts and wrapper.exe source
Ralf Wildenhues wrote:
Thanks. Please resubmit with the new functions moved to right before
func_mode_link (or func_mode_install/func_mode_execute, in case you
intend to use them from inside them in a followup patch); there's a real
performance benefit to doing this for compile mode, as it saves the
shell from having to parse the functions in that case. It may be lost
in the fork/exec overhead on w32, but on GNU/Linux, libtool compile mode
overhead is dominated by shell parsing (see the 2007-02-11 changes for a
30% execution time reduction this way).
Done. Testing is still in progress (again, in conjunction with updated
argz.m4 patch). However, mdemo works (on all six test variants
described in the argz.m4 messages), and the entire "old" test suite
passes on cygwin(broken argz kernel: 1.5.24-2).
Also, please use round parentheses as per GCS for referring to functions
in the ChangeLog entry. I'll apply the reposted patch then.
Done.
--
Chuck
2007-04-19 Charles Wilson <[EMAIL PROTECTED]>
* libltdl/config/ltmain.m4sh (func_mode_link): move wrapper
script generation from here...
(func_emit_libtool_wrapper_script): to this new function, and
write to stdout
(func_mode_link): move cwrapper source code generation from
here...
(func_emit_libtool_cwrapperexe_source): to this new function,
and write to stdout
(func_mode_link): call the two new functions and redirect
output to appropriate file.
Index: libltdl/config/ltmain.m4sh
===
RCS file: /cvsroot/libtool/libtool/libltdl/config/ltmain.m4sh,v
retrieving revision 1.72
diff -u -r1.72 ltmain.m4sh
--- libltdl/config/ltmain.m4sh 2007-04-19 14:51:24.109375000 -0400
+++ libltdl/config/ltmain.m4sh 2007-04-19 14:51:00.96875 -0400
@@ -2209,6 +2209,555 @@
test "$mode" = install && func_mode_install ${1+"$@"}
+# func_emit_libtool_wrapper_script
+# emit a libtool wrapper script on stdout
+# don't directly open a file because we may want to
+# incorporate the script contents within a cygwin/mingw
+# wrapper executable. Must ONLY be called from within
+# func_mode_link because it depends on a number of variable
+# set therein.
+func_emit_libtool_wrapper_script ()
+{
+ $ECHO "\
+#! $SHELL
+
+# $output - temporary wrapper script for $objdir/$outputname
+# Generated by $PROGRAM (GNU $PACKAGE$TIMESTAMP) $VERSION
+#
+# The $output program cannot be directly executed until all the libtool
+# libraries that it depends on are installed.
+#
+# This wrapper script should never be moved out of the build directory.
+# If it is, it will not operate correctly.
+
+# Sed substitution that helps us do robust quoting. It backslashifies
+# metacharacters that are still active within double-quoted strings.
+Xsed='${SED} -e 1s/^X//'
+sed_quote_subst='$sed_quote_subst'
+
+# Be Bourne compatible
+if test -n \"\${ZSH_VERSION+set}\" && (emulate sh) >/dev/null 2>&1; then
+ emulate sh
+ NULLCMD=:
+ # Zsh 3.x and 4.x performs word splitting on \${1+\"[EMAIL PROTECTED]"},
which
+ # is contrary to our usage. Disable this feature.
+ alias -g '\${1+\"[EMAIL PROTECTED]"}'='\"[EMAIL PROTECTED]"'
+ setopt NO_GLOB_SUBST
+else
+ case \`(set -o) 2>/dev/null\` in *posix*) set -o posix;; esac
+fi
+BIN_SH=xpg4; export BIN_SH # for Tru64
+DUALCASE=1; export DUALCASE # for MKS sh
+
+# The HP-UX ksh and POSIX shell print the target directory to stdout
+# if CDPATH is set.
+(unset CDPATH) >/dev/null 2>&1 && unset CDPATH
+
+relink_command=\"$relink_command\"
+
+# This environment variable determines our operation mode.
+if test \"\$libtool_install_magic\" = \"$magic\"; then
+ # install mode needs the following variables:
+ generated_by_libtool_version='$macro_version'
+ notinst_deplibs='$notinst_deplibs'
+else
+ # When we are sourced in execute mode, \$file and \$ECHO are already set.
+ if test \"\$libtool_execute_magic\" != \"$magic\"; then
+ECHO=\"$qecho\"
+file=\"\$0\"
+# Make sure echo works.
+if test \"X\$1\" = X--no-reexec; then
+ # Discard the --no-reexec flag, and continue.
+ shift
+elif test \"X\`{ \$ECHO '\t'; } 2>/dev/null\`\" = 'X\t'; then
+ # Yippee, \$ECHO works!
+ :
+else
+ # Restart under the correct shell, and then maybe \$ECHO will work.
+ exec $SHELL \"\$0\" --no-reexec \${1+\"[EMAIL PROTECTED]"}
+fi
+ fi\
+"
+ $ECHO "\
+
+ # Find the directory that this script lives in.
+ thisdir=\`\$ECHO \"X\$file\" | \$Xsed -e 's%/[^/]*$%%'\`
+ test \"x\$thisdir\" = \"x\$file\" && thisdir=.
+
+ # Follow symbolic links until we get to the real thisdir.
+ file=\`ls -ld \"\$file\" | ${SED} -n 's/.*-> //p'\`
+ while test -n \"\$file\"; do
+destdir=\`\$ECHO \"X\$file\" | \$Xsed -e 's%/[^/]*\$%%'\`
+
+# If there was a directory component, then change thisdir.
+if test \"x\$destdir\" != \"x\$file\"; then
+ case \"\$destdir\" in
+ [/]* |
Re: mdemo ltdl failure
Hopefully the attached patch addresses all comments...Recapping: The argz functions (specifically, argz_insert) supplied by cygwin are buggy, in wierd use-dependent malloc-related ways. I've already submitted a patch to newlib to fix that error which has been accepted http://sourceware.org/ml/newlib/2007/msg00271.html but (a) there's no telling when a new cygwin kernel with that fix will be released, and (b) libtool ought to work with any relatively recent cygwin kernel. Testing for actual broken argz behavior, as I did in my first patch http://lists.gnu.org/archive/html/libtool-patches/2007-03/msg00030.html is horrendously ugly, brittle, and all-around bad -- even if that is the "recommended" Way Of Autoconf. This patch is a refinement of an alternative, first proposed here: http://lists.gnu.org/archive/html/libtool-patches/2007-04/msg00035.html where we instead check the platform and OS version for the few (only?) known-bad systems which both declare and provide the desired argz functions, but whose argz implementation is broken. The basic idea of this patch is: (1) if argz is found on the system, check to see if the $host_os and os version are such that the system argz is "known bad" (2) if not, everything is fine (3) if so (currently: cygwin, 1.5.24 or older), then we need to force use of the libltdl-provided argz (a) define a new symbol SYSTEM_ARGZ_IS_BROKEN (b) set ARGZ_H and AC_LIBOBJ, just as if we didn't find argz on the system (c) use SYSTEM_ARGZ_IS_BROKEN in lt__glibc.h along with HAVE_ARGZ_H to determine whether to #define argz* lt__argz* === One other change: you can now force the use of libltdl's argz on any system, which was not possible before when HAVE_ARGZ_H was true. BUT, doing this with HAVE_ARGZ_H true carries some risk: we've already decided whether to #define error_t ourselves, and/or set __error_t_defined, based on what we detected AFTER #including the system's argz.h -- but by forcing the use of libltdl's argz, we won't use the system's argz.h, so at compile-time we might not "see" what the error_t test "saw". Therefore, libltdl's argz_.h needs to include most of the common places where error_t may have been "picked up" by the system argz.h. On newlib systems, this is . On glibc systems, this is also , but you must #define __need_error_t first (which glibc's argz.h does). So, I've modified libltdl's argz_.h to do that, too. This is really a minor issue IMO. If the system argz is found, it should be used except in rare circumstances (like, it's broken). At present, the only known system where this applies is cygwin, and cygwin doesn't need the extra stuff specified in this section. But, trying to be thorough... I've tested argzfix-3.patch (in conjunction with the functionalize wrapper generation patch) under the following conditions: (NOTES: "works" means that mdemo-conf/mdemo-make/mdemo-exec passes. Also, I checked every libltdl ahd mdemo/mlib shared library explicitly using objdump or nm to determine whether system argz functions were imported, or whether it exported its own replacement argz functions. In each case, the results "track" with what should be expected, for each of the six test cases below) (1) broken cygwin kernel (1.5.24-2 used, but any older would do) --reports that system argz is broken, builds successfully using libltdl's argz --resultant libraries and mdemo tests also work after dropping in a fixed cygwin kernel. (2) fixed cygwin kernel (official snapshot from 20070330) --reports that system argz works, builds successfully using system argz --resultant libraries coredump if you drop in a broken cygwin kernel after the fact. This is expected: broken cygwin is "broken" precisely because its argz facility coredumps on argz_insert(). (3) fixed cygwin kernel, but with 'export $lt_cv_sys_argz_works=no'. --reports that system argz does not work (cached), and builds successfully using libltdl's argz --resultant libraries works fine even after dropping in a broken cygwin kernel. (4) linux (whose system argz is OK) --reports that system argz works, builds successfully using system argz, works. (5) linux, but with 'export $lt_cv_sys_argz_works=no'. --reports that system argz does not work (cached), and builds successfully using libltdl's argz (6) mingw, which doesn't have any system argz facility at all --because the argz functions are not declared, the section of the configure script that reports whether system argz works is not even run, nor is the cache checked. configure has already decided to use the libltdl-supplied argz. --and, in fact, build is completed successfully using libltdl supplied argz, and the result works. Also, under case (1), ran the entire testsuite. All old-style tests pass:
Re: mdemo ltdl failure
Charles Wilson wrote: Under case (1), currently running the new-style testsuite. Will report that later in a follow-up message. I expect the following: 14: Java convenience archives FAILED (convenience.at:273) 16: Link order of deplibs. FAILED (link-order2.at:129) 49: Run tests with low max_cmd_len FAILED (cmdline_wrap.at:42) Confirmed. which IMO are longstanding on cygwin, and certainly have nothing to do with either this patch or the 'functionalize wrapper generation' patch. -- Chuck
