Re: mdemo ltdl failure

[Ralf Wildenhues]

Hi Charles, Bruno,

* Charles Wilson wrote on Thu, Apr 26, 2007 at 07:34:56AM CEST:

 Attached.  Re-ran *all* of the tests described here:
 http://lists.gnu.org/archive/html/libtool-patches/2007-04/msg00073.html
 with identical results.

Thank you for patching and testing, and thanks to Bruno for the fine
review.

 I did not bump the argz.m4 serial again (I'm not sure what the rules are: 
 bump on EVERY change, or only on big changes?).  So, Ralf, please do that 
 manually if it is necessary.

It can't hurt, so I did that, and applied (libtool and gnulib).

 +  if test $lt_os_major -gt 1 \
 + || { test $lt_os_major -eq 1 \
 +{ test $lt_os_minor -gt 5 \
 + || { test $lt_os_minor -eq 5 \
 +test $lt_os_micro -gt 24; }; }; }; then

Bruno asked for this GCS compliant formatting.  I have some (very minor)
issue with it: it looks uglier in shell, due to the necessary
backslashes.  Also, the GCS specify this for C code only, to be strict.
But I'm not all that strong about it, so I left it in your patch.

Cheers,
Ralf

Re: mdemo ltdl failure

[Ralf Wildenhues]

[ http://thread.gmane.org/gmane.comp.gnu.libtool.patches/7314/focus=7498 ]

Thanks Charles for all your work on this.  I installed this path into
Libtool HEAD, and pulled the changes over to gnulib.  Here's what the
gnulib patch looks like.

Cheers,
Ralf

2007-04-25  Charles Wilson  [EMAIL PROTECTED]
Ralf Wildenhues  [EMAIL PROTECTED]

* lib/argz_.h: ensure error_t definition is obtained in same
mechanism system argz.h would have.
* m4/argz.m4 (gl_FUNC_ARGZ): add new test to check if $host's
argz facilities are known bad.  Err on the side of caution if
cross-compiling.

Index: lib/argz_.h
===
RCS file: /cvsroot/gnulib/gnulib/lib/argz_.h,v
retrieving revision 1.5
diff -u -r1.5 argz_.h
--- lib/argz_.h 29 Mar 2007 15:02:55 -  1.5
+++ lib/argz_.h 25 Apr 2007 21:16:56 -
@@ -1,6 +1,6 @@
 /* lt__argz.h -- internal argz interface for non-glibc systems
 
-   Copyright (C) 2004 Free Software Foundation, Inc.
+   Copyright (C) 2004, 2007 Free Software Foundation, Inc.
Written by Gary V. Vaughan, 2004
 
NOTE: The canonical source of this file is maintained with the
@@ -32,6 +32,8 @@
 #define LT__ARGZ_H 1
 
 #include stdlib.h
+#define __need_error_t
+#include errno.h
 #include sys/types.h
 
 #if defined(LTDL)
Index: m4/argz.m4
===
RCS file: /cvsroot/gnulib/gnulib/m4/argz.m4,v
retrieving revision 1.5
diff -u -r1.5 argz.m4
--- m4/argz.m4  29 Mar 2007 15:02:55 -  1.5
+++ m4/argz.m4  25 Apr 2007 21:16:57 -
@@ -1,13 +1,13 @@
 # Portability macros for glibc argz.-*- Autoconf -*-
 #
-#   Copyright (C) 2004, 2005, 2006 Free Software Foundation, Inc.
+#   Copyright (C) 2004, 2005, 2006, 2007 Free Software Foundation, Inc.
 #   Written by Gary V. Vaughan [EMAIL PROTECTED]
 #
 # This file is free software; the Free Software Foundation gives
 # unlimited permission to copy and/or distribute it, with or without
 # modifications, as long as this notice is preserved.
 
-# serial 4 argz.m4
+# serial 5 argz.m4
 
 AC_DEFUN([gl_FUNC_ARGZ],
 [gl_PREREQ_ARGZ
@@ -27,6 +27,50 @@
 ARGZ_H=
 AC_CHECK_FUNCS([argz_append argz_create_sep argz_insert argz_next \
argz_stringify], [], [ARGZ_H=argz.h; AC_LIBOBJ([argz])])
+
+dnl if have system argz functions, allow forced use of 
+dnl libltdl-supplied implementation (and default to do so
+dnl on known bad systems). Could use a runtime check, but
+dnl (a) detecting malloc issues is notoriously unreliable
+dnl (b) only known system that declares argz functions,
+dnl provides them, yet they are broken, is cygwin
+dnl releases prior to 16-Mar-2007 (1.5.24 and earlier)
+dnl So, it's more straightforward simply to special case 
+dnl this for known bad systems.
+AS_IF([test -z $ARGZ_H],
+[AC_CACHE_CHECK(
+[if argz actually works],
+[lt_cv_sys_argz_works],
+[case $host_os in #(
+*cygwin*)
+  lt_cv_sys_argz_works=no
+  if test $cross_compiling != no; then
+lt_cv_sys_argz_works=guessing no
+  else
+save_IFS=$IFS
+IFS=-.
+set x `uname -r | $SED -e 's/^\([[0-9\.]]*\).*/\1/'`
+IFS=$save_IFS
+lt_os_major=$[]{2-0}
+lt_os_minor=$[]{3-0}
+lt_os_micro=$[]{4-0}
+if test $lt_os_major -gt 1 ||
+   { test $lt_os_major -eq 1 
+ { test $lt_os_minor -gt 5 ||
+   { test $lt_os_minor -eq 5 
+ test $lt_os_micro -gt 24; }; }; }; then
+  lt_cv_sys_argz_works=yes
+fi
+  fi
+  ;; #(
+*) lt_cv_sys_argz_works=yes ;;
+esac])
+ AS_IF([test $lt_cv_sys_argz_works != yes],
+[AC_DEFINE([SYSTEM_ARGZ_IS_BROKEN], 1,
+   [This value is set to 1 to indicate that the system argz 
facility does not work])
+ARGZ_H=argz.h
+AC_LIBOBJ([argz])])])
+
 AC_SUBST([ARGZ_H])
 ])
 

Re: mdemo ltdl failure

[Charles Wilson]

Charles Wilson wrote:

I'll generate and
test an additional patch addressing Bruno's concerns.


Attached.  Re-ran *all* of the tests described here:
http://lists.gnu.org/archive/html/libtool-patches/2007-04/msg00073.html
with identical results.

I did not bump the argz.m4 serial again (I'm not sure what the rules 
are: bump on EVERY change, or only on big changes?).  So, Ralf, please 
do that manually if it is necessary.


--
Chuck

2007-04-26  Charles Wilson  [EMAIL PROTECTED]

* libltdl/libltdl/lt__glibc.h: use !HAVE_WORKING_ARGZ
instead of SYSTEM_ARGZ_IS_BROKEN
* libltdl/m4/argz.m4 (gl_FUNC_ARGZ): ditto.  Also, minor
stylistic improvements
Reported by: Bruno Haible [EMAIL PROTECTED]

Index: libltdl/libltdl/lt__glibc.h
===
RCS file: /cvsroot/libtool/libtool/libltdl/libltdl/lt__glibc.h,v
retrieving revision 1.9
diff -u -r1.9 lt__glibc.h
--- libltdl/libltdl/lt__glibc.h 25 Apr 2007 21:17:59 -  1.9
+++ libltdl/libltdl/lt__glibc.h 26 Apr 2007 00:33:34 -
@@ -37,7 +37,7 @@
 #  include config.h
 #endif
 
-#if !defined(HAVE_ARGZ_H) || defined(SYSTEM_ARGZ_IS_BROKEN)
+#if !defined(HAVE_ARGZ_H) || !defined(HAVE_WORKING_ARGZ)
 /* Redefine any glibc symbols we reimplement to import the
implementations into our lt__ namespace so we don't ever
clash with the system library if our clients use argz_*
Index: libltdl/m4/argz.m4
===
RCS file: /cvsroot/libtool/libtool/libltdl/m4/argz.m4,v
retrieving revision 1.5
diff -u -r1.5 argz.m4
--- libltdl/m4/argz.m4  25 Apr 2007 21:17:59 -  1.5
+++ libltdl/m4/argz.m4  26 Apr 2007 00:33:34 -
@@ -41,34 +41,35 @@
 [AC_CACHE_CHECK(
 [if argz actually works],
 [lt_cv_sys_argz_works],
-[case $host_os in #(
+[[case $host_os in #(
 *cygwin*)
   lt_cv_sys_argz_works=no
   if test $cross_compiling != no; then
 lt_cv_sys_argz_works=guessing no
   else
+lt_sed_extract_leading_digits='s/^\([0-9\.]*\).*/\1/'
 save_IFS=$IFS
 IFS=-.
-set x `uname -r | $SED -e 's/^\([[0-9\.]]*\).*/\1/'`
+set x `uname -r | sed -e $lt_sed_extract_leading_digits`
 IFS=$save_IFS
-lt_os_major=$[]{2-0}
-lt_os_minor=$[]{3-0}
-lt_os_micro=$[]{4-0}
-if test $lt_os_major -gt 1 ||
-   { test $lt_os_major -eq 1 
- { test $lt_os_minor -gt 5 ||
-   { test $lt_os_minor -eq 5 
- test $lt_os_micro -gt 24; }; }; }; then
+lt_os_major=${2-0}
+lt_os_minor=${3-0}
+lt_os_micro=${4-0}
+if test $lt_os_major -gt 1 \
+   || { test $lt_os_major -eq 1 \
+  { test $lt_os_minor -gt 5 \
+   || { test $lt_os_minor -eq 5 \
+  test $lt_os_micro -gt 24; }; }; }; then
   lt_cv_sys_argz_works=yes
 fi
   fi
   ;; #(
 *) lt_cv_sys_argz_works=yes ;;
-esac])
- AS_IF([test $lt_cv_sys_argz_works != yes],
-[AC_DEFINE([SYSTEM_ARGZ_IS_BROKEN], 1,
-   [This value is set to 1 to indicate that the system argz 
facility does not work])
-ARGZ_H=argz.h
+esac]])
+ AS_IF([test $lt_cv_sys_argz_works = yes],
+[AC_DEFINE([HAVE_WORKING_ARGZ], 1,
+   [This value is set to 1 to indicate that the system argz 
facility works])],
+[ARGZ_H=argz.h
 AC_LIBOBJ([argz])])])
 
 AC_SUBST([ARGZ_H])

Re: mdemo ltdl failure

[Ralf Wildenhues]

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: mdemo ltdl failure

[Charles Wilson]

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: mdemo ltdl failure

[Ralf Wildenhues]

* 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

[Ralf Wildenhues]

* 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: mdemo ltdl failure

[Charles Wilson]

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 errno.h.  On glibc systems, this is also errno.h, 
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]

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

Re: mdemo ltdl failure

[Charles Wilson]

Charles Wilson wrote:

Charles Wilson wrote:

Charles Wilson wrote:

I'll whip up a patch and post it to the newlib list.


So, I posted the following:
http://sourceware.org/ml/newlib/2007/msg00271.html

However, there's no telling how long it'll be before a new cygwin 
kernel is released that includes the patch, and besides, libtool ought 
to work on current and reasonably recent cygwin kernels.  So, how 
should libtool address this issue in the interim?  Always use 
libltdl's argz stuff on cygwin, until further notice: version check 
on uname, etc etc?


If so, how do we induce that


Here's my attempt:


[snip long description of ugly runtime test]
See
http://lists.gnu.org/archive/html/libtool-patches/2007-03/msg00030.html

After discussion with Bob F, I've reimplemented this fix without the 
actual runtime test.  Instead, if $host_os is cygwin, and cygwin version 
is 1.5.24 or older, then force use of libltdl builtin argz.  In all 
other cases (including cross), pre-existing detection rules apply.


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'm still using the -export-symbols-regex .* fix for libmlib in 
tests/mdemo -- it's also included in this patch.


Still true.


TESTING:
I've tested this under the following conditions:

(1) broken cygwin kernel (1.5.24-2 used, but any older would do)
--detects failure, builds successfully using libltdl's argz
--resultant libraries also work after dropping in a
  fixed cygwin kernel.
(2) fixed cygwin kernel (official snapshot from 20070330)
--detects success, 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 $lt_cv_sys_argz_works=no.
--does not even try to run the test program, and successfully
  builds using libltdl's argz
--resultant libraries works fine even after dropping in a
  broken cygwin kernel.


Re-ran all of these tests under the specified conditions.  Same results 
as 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.


New Changelog:

2007-03-17  Charles Wilson  [EMAIL PROTECTED]

* libltdl/argz_.h: ensure error_t definition is obtained
in same mechanism system argz.h would have.
* libltdl/libltdl/lt__glibc.h: also detect if
SYSTEM_ARGZ_IS_BROKEN when determining whether to re#def
argz* functions.
* libltdl/m4/argz.m4: add new test to check if $host's
argz facilities are known bad.
* tests/mdemo/Makefile.am: -export-symbols-regex fix for
libmlib.

--
Chuck
Index: libltdl/argz_.h
===
RCS file: /cvsroot/libtool/libtool/libltdl/argz_.h,v
retrieving revision 1.6
diff -u -r1.6 argz_.h
--- libltdl/argz_.h 4 Sep 2006 17:23:30 -   1.6
+++ libltdl/argz_.h 17 Mar 2007 06:09:46 -
@@ -32,6 +32,8 @@
 #define LT__ARGZ_H 1
 
 #include stdlib.h
+#define __need_error_t
+#include errno.h
 #include sys/types.h
 
 #if defined(LTDL)
Index: libltdl/libltdl/lt__glibc.h
===
RCS file: /cvsroot/libtool/libtool/libltdl/libltdl/lt__glibc.h,v
retrieving revision 1.7
diff -u -r1.7 lt__glibc.h
--- libltdl/libltdl/lt__glibc.h 26 Oct 2006 20:39:04 -  1.7
+++ libltdl/libltdl/lt__glibc.h 17 Mar 2007 06:09:49 -
@@ -37,7 +37,7 @@
 #  include config.h
 #endif
 
-#if !defined(HAVE_ARGZ_H)
+#if !defined(HAVE_ARGZ_H) || defined(SYSTEM_ARGZ_IS_BROKEN)
 /* Redefine any glibc symbols we reimplement to import the
implementations into our lt__ namespace so we don't ever
clash with the system library if our clients use argz_*
Index: libltdl/m4/argz.m4
===
RCS file: /cvsroot/libtool/libtool/libltdl/m4/argz.m4,v
retrieving revision 1.3
diff -u -r1.3 argz.m4
--- libltdl/m4/argz.m4  25 Mar 2006 11:05:02 -  1.3
+++ libltdl/m4/argz.m4  17 Mar 2007 06:09:50 -
@@ -27,6 +27,38 @@
 ARGZ_H=
 AC_CHECK_FUNCS([argz_append argz_create_sep argz_insert argz_next \
argz_stringify], [], [ARGZ_H=argz.h; AC_LIBOBJ([argz])])
+
+dnl if have system argz functions, allow forced use of 
+dnl libltdl-supplied implementation (and default to 

Re: mdemo ltdl failure

[Bob Friesenhahn]

On Wed, 18 Apr 2007, Charles Wilson wrote:


[snip long description of ugly runtime test]
See
http://lists.gnu.org/archive/html/libtool-patches/2007-03/msg00030.html

After discussion with Bob F, I've reimplemented this fix without the actual 
runtime test.  Instead, if $host_os is cygwin, and cygwin version is 1.5.24 
or older, then force use of libltdl builtin argz.  In all other cases 
(including cross), pre-existing detection rules apply.


This new version is certainly more attractive than before.  Presumably 
this special case can be safely removed after enough time has elapsed 
for the majority of Cygwin installs to be updated.


Is the:

  libmlib_la_LDFLAGS = -no-undefined -export-symbols-regex .*

statement portable?

Bob
==
Bob Friesenhahn
[EMAIL PROTECTED], http://www.simplesystems.org/users/bfriesen/
GraphicsMagick Maintainer,http://www.GraphicsMagick.org/

Re: mdemo ltdl failure

[Charles Wilson]

Bob Friesenhahn wrote:

On Wed, 18 Apr 2007, Charles Wilson wrote:


[snip long description of ugly runtime test]
See
http://lists.gnu.org/archive/html/libtool-patches/2007-03/msg00030.html

After discussion with Bob F, I've reimplemented this fix without the 
actual runtime test.  Instead, if $host_os is cygwin, and cygwin 
version is 1.5.24 or older, then force use of libltdl builtin argz.  
In all other cases (including cross), pre-existing detection rules apply.


This new version is certainly more attractive than before.  Presumably 
this special case can be safely removed after enough time has elapsed 
for the majority of Cygwin installs to be updated.


Right, give or take a year. 8-P


Is the:

  libmlib_la_LDFLAGS = -no-undefined -export-symbols-regex .*

statement portable?


(1) Yes

(2) and it shouldn't be in this patch anyway. Ralf already applied that 
bit to HEAD:

http://www.mail-archive.com/libtool-patches@gnu.org/msg02848.html

My bigger worries with this patch were that it could regress on
some other, unrelated platform.  I've checked the systems I have access
to now, and all seem to do fine.  So I have applied the change as below.

This is what happens when you try some home-grown version of 
patch-quilt...long story.


--
Chuck

RE: mdemo ltdl failure

[Dave Korn]

On 16 March 2007 15:35, Charles Wilson wrote:

 Well, it's failing all the time for me, but I'm not sure it's a 
 segfault. What does Hangup mean, when reported by the shell after 
 executing the app: Good question, I don't know.

  It means SIGHUP.

 I've (almost) tracked down the error: it is caused by yet another bug in
 newlib's argz_insert() (or possibly realloc()! ), as called by
 lt_argz_insert:

  This thread should probably be on the newlib list then.

 What's odd is that this bug in argz_insert() is very ticklish: it
 triggers on tests/mdemo/Makefile, but not when argz_insert is called
 with ./tests/mdemo/Makefile.

  Isn't that just exactly what you would expect, given that you're talking
about sorting things in ascii order?  The period collates very early in ascii
sort order, whereas a lower-case t comes much later; hence if you specify the
'.' you get the makefile at the start of the list instead of the end.

 I need to verify this using a debug-built cygwin kernel, but it looks
 like within newlib's argz_insert(), the call to realloc() is not
 operating correctly in this instance.

  Sounds like it should be quite easy to PPAST then.


cheers,
  DaveK
-- 
Can't think of a witty .sigline today

Re: mdemo ltdl failure

[Charles Wilson]

Well, once I got the cygwin1.dbg stuff worked out, it was pretty easy to 
track down: it is a bug in newlib's argz_insert:


Charles Wilson wrote:

Here's the code from newlib's argz_insert:

error_t
_DEFUN (argz_insert, (argz, argz_len, before, entry),
   char **argz _AND
   size_t *argz_len _AND
   char *before _AND
   const char *entry)
{
  int len = 0;

  if (before == NULL)
return argz_add(argz, argz_len, entry);

  if (before  *argz || before = *argz + *argz_len)
return EINVAL;



Note that before is always either NULL or points to some location within 
the existing *argz buffer.



  while (before != *argz  before[-1])
before--;


Because *argz contains NULL-delimited strings one after the other, if 
the user calls this function with a before that points into the middle 
of one of those strings, the preceeding two lines just back up to the 
beginning of that string (or to the beginning of the current argz, 
whichever comes first).



  len = strlen(entry) + 1;


In the failing call, we actually do a realloc...


  if(!(*argz = (char *)realloc(*argz, *argz_len + len)))
return ENOMEM;


But if we realloc the *argz buffer, then a non-NULL 'before' pointer 
will be pointing into the old, freed, *argz buffer.  So the following is 
clearly wrong, because we are copying stuff _from_ the new *argz to a 
modified location in the old (shorter) *argz -- which will overrun the 
end of the old buffer by exactly strlen(entry), and clobber stuff. 
Depending on the actual allocated locations of malloced data, this could 
include (1) like some of the the memory held by entry, or (2) some of 
the memory held by the new *argz buffer.  In this case, it is (1).



  memmove(before + len, before, *argz + *argz_len - before);


Then, we copy this clobbered entry data into the front of (the freed, 
old *argz buffer)



  memcpy(before, entry, len);

  *argz_len += len;


meanwhile, the actual (newly realloc'ed) *argz buffer just contains 
whatever was in *argz prior to the call to this function.  Worse, with 
upward malloc movement, the too-large memove above might also have 
clobbered the first several bytes of the new *argz buffer, as well.


And that's what's happening in this case.

The eventual FREE(buf) error is because the first few bytes in the 
malloc-managed memory for buf (e.g. just below *buf) which contain 
malloc bookkeeping info, are also clobbered.



  return 0;
}


I'll whip up a patch and post it to the newlib list.

--
Chuck