Re: cwrapper generates long strings.

[Peter Rosin]

Den 2010-10-04 07:00 skrev Ralf Wildenhues:
> * Peter Rosin wrote on Sun, Oct 03, 2010 at 08:28:47PM CEST:
>> Den 2010-10-03 09:01 skrev Ralf Wildenhues:
>>>> +for i in 25 50 75 100 125 150 175 200 225 250
>>>> +do
>>>> +  PATH="$PATH$PATH_SEPARATOR/somewhere-that-eksists-not"
> 
>>> Does $LIBTOOL or the autotest machinery ever intentionally try to run
>>> commands that won't exist in $PATH within this shell?
>>
>> I don't think so, and it is a really hard question to answer too.
> 
> Indeed.
> 
> I'm kinda wondering whether we should at least delimit our use of
> nonexistent files and directories to a common subtree, like below
> /nonexistent or so.  I realize we're getting near bike shedding
> issues though, so how about we cross that bridge when we get to it,
> and leave your patch as is for now.
> 
>>>  If so, then we
>>> might get the odd bug report from security-hardened distributions that
>>> complain about read or execute accessses to non-allowed parts of the
>>> file system.
>>
>> Using "$PATH$PATH_SEPARATOR$PATH" seemed like a much quicker way
>> to make the length explode so I didn't like that.
> 
> OK.

I wrote a loop appending the first PATH component until the length
exceeds the limit.  The longest possible PATH should be 499 characters
this way, which seems OK to me, and it should have no "wild" components.

>>> This length doesn't yet trigger the compiler string literal length
>>> limit; not sure whether it should?
>>
>> That's not reliable anyway as most compilers support so long strings
>> that it's hard to tickle it.
> 
> FWIW, that is not necessary.  It would be sufficient if it were tickled
> with the one compiler in question.

Since the compiler limit in this case is as large as 2048 (whoooa), which
is about the same as you quoted for grep, I decided to not do that.

>> On a tangent, another bug is that linking
>> doesn't fail (libtool returns zero) when building the cwrapper fails.
>> I think that's a separate issue from this one, which is why I haven't
>> mixed them up previously.
> 
> OK, good.
> 
>> Another nit in that area is that there are
>> multiple levels of "$opt_dry_run || {" which seems superfluous, but
>> that might be intentional in order to keep the code copy-paste-safe?
> 
> Not sure.  I don't have much sympathy for copy-paste-safety, because
> I dislike the kludge that copy-paste is.  Functions should be used
> instead.  So yes, I guess a cleanup is a good idea, after ensuring that
> the code really works fine with the outer opt_dry_run enclosing.

Ok.

>>> Do we have to cater to the case where the environment is very large
>>> already?  I'm not sure, we might want to deal with it when crossing
>>> that bridge only, if it's hard to know off-hand.
>>
>> Are your three above paragraphs nits and part of what I need to
>> address, or just notes for the future?
> 
> They started out as nits, but I guess they've become notes by now.
> So go ahead with your patch, please.

Holding it up for one more iteration.

>>>> +# try to make sure the test is relevant
>>>> +AT_CHECK([grep ' *fputs' $objdir/lt-usea.c > /dev/null])
>>>
>>> Rather than redirecting stdout, put [ignore] in the third argument.
>>
>> Certainly possible, but I didn't think anyone would be interested in a
>> couple of hundred lines of boilerplate in the log.  I don't really care
>> though, so if you still think [ignore] is a good idea, then ok.
> 
> Ah yes.  Autoconf 2.64 provides stdout-nolog for this, but I guess you
> can keep the code the way it is, for compatibility.

Ok.

Cheers,
Peter

>From 0cdd5a00c698cc47e4c7d1af377b7fb5090a417b Mon Sep 17 00:00:00 2001
From: Peter Rosin <address@hidden>
Date: Mon, 4 Oct 2010 09:40:45 +0200
Subject: [PATCH] cwrapper: split long lines when dumping the wrapper script.

* libltdl/config/ltmain.m4sh (func_emit_cwrapperexe_src): If
the wrapper script contains long lines, split them for
readability and to conform with C standards.
* tests/cwrapper.at (cwrapper string length): New test, making
sure we don't regress.

Signed-off-by: Peter Rosin <address@hidden>
---
 ChangeLog                  |    9 ++++++
 libltdl/config/ltmain.m4sh |   12 ++++++--
 tests/cwrapper.at          |   61 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 79 insertions(+), 3 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index a7aa489..e45bfe8 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,12 @@
+2010-10-04  Peter Rosin  <address@hidden>
+
+       cwrapper: split long lines when dumping the wrapper script.
+       * libltdl/config/ltmain.m4sh (func_emit_cwrapperexe_src): If
+       the wrapper script contains long lines, split them for
+       readability and to conform with C standards.
+       * tests/cwrapper.at (cwrapper string length): New test, making
+       sure we don't regress.
+
 2010-09-27  Peter Rosin  <address@hidden>
 
        tests: check if sys_lib_search_path_spec works on MSVC.
diff --git a/libltdl/config/ltmain.m4sh b/libltdl/config/ltmain.m4sh
index 0418007..1078e75 100644
--- a/libltdl/config/ltmain.m4sh
+++ b/libltdl/config/ltmain.m4sh
@@ -4268,9 +4268,15 @@ void lt_dump_script (FILE* f)
 {
 EOF
            func_emit_wrapper yes |
-              $SED -e 's/\([\\"]\)/\\\1/g' \
-                  -e 's/^/  fputs ("/' -e 's/$/\\n", f);/'
-
+             $SED -n -e '
+s/^\(.\{79\}\)\(..*\)/\1\
+\2/
+h
+s/\([\\"]\)/\\\1/g
+s/$/\\n/
+s/\([^\n]*\).*/  fputs ("\1", f);/p
+g
+D'
             cat <<"EOF"
 }
 EOF
diff --git a/tests/cwrapper.at b/tests/cwrapper.at
index 248c0c0..cd618dc 100644
--- a/tests/cwrapper.at
+++ b/tests/cwrapper.at
@@ -134,3 +134,64 @@ done
 
 AT_CLEANUP
 
+
+AT_SETUP([cwrapper string length])
+
+eval "`$LIBTOOL --config | $EGREP '^(objdir)='`"
+
+AT_DATA([liba.c],
+[[int liba_func1 (int arg)
+{
+  return arg + 1;
+}
+]])
+AT_DATA([usea.c],
+[[extern int liba_func1 (int arg);
+int main (void)
+{
+  int a = 2;
+  int b = liba_func1 (a);
+  if (b == 3) return 0;
+  return 1;
+}
+]])
+
+AT_CHECK([$LIBTOOL --mode=compile $CC $CPPFLAGS $CFLAGS -c liba.c],
+        [], [ignore], [ignore])
+AT_CHECK([$LIBTOOL --mode=link $CC $CFLAGS $LDFLAGS -no-undefined ]dnl
+        [-o liba.la -rpath /foo liba.lo],
+        [], [ignore], [ignore])
+AT_CHECK([$CC $CPPFLAGS $CFLAGS -c usea.c],
+        [], [ignore], [ignore])
+
+
+# Make sure PATH is at least 250 chars, which should force line breaks
+# in lt-usea.c.
+
+dirpath=
+save_IFS=$IFS
+IFS=$PATH_SEPARATOR
+for dirpath in $PATH; do
+  IFS=$save_IFS
+  break
+done
+IFS=$save_IFS
+
+until $ECHO "PATH=$PATH" | grep 'PATH=.\{250\}'; do
+  PATH="$PATH$PATH_SEPARATOR$dirpath"
+done
+export PATH
+
+AT_CHECK([$LIBTOOL --mode=link $CC $CFLAGS $LDFLAGS -no-fast-install ]dnl
+        [-o usea$EXEEXT usea.$OBJEXT liba.la],
+        [], [ignore], [ignore])
+
+# Skip if no cwrapper is generated.
+AT_CHECK([test -f $objdir/lt-usea.c || exit 77])
+
+# Try to make sure the test is relevant.
+AT_CHECK([grep ' *fputs' $objdir/lt-usea.c > /dev/null])
+# Check for no overly long fputs.
+AT_CHECK([grep ' *fputs.\{250\}' $objdir/lt-usea.c], [1])
+
+AT_CLEANUP
-- 
1.7.1