autoconf-patches
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: patch for problems with echo '-...' and echo '...\...'


From: Ralf Wildenhues
Subject: Re: patch for problems with echo '-...' and echo '...\...'
Date: Fri, 1 Dec 2006 07:24:44 +0100
User-agent: Mutt/1.5.13 (2006-08-11)

Hello Paul,

* Paul Eggert wrote on Sat, Nov 25, 2006 at 10:59:28AM CET:
> 
> OK, thanks, I installed the patch appended at the end of this message.
> It should address the problem, but I have no Solaris 7 host to test it
> on.  Can you please try it?

Yes, see below.

> The patch doesn't use plain /usr/ucb/echo to implement AS_ECHO, since
> that would mishandle AS_ECHO([-n]) (which we want to output '-n'
> followed by a newline).

Oh, ok.  It'd probably be good to document the semantics of AS_ECHO.
(Yes, I should be propoposing a patch...)

> >> -  $(edit) `test -f ./address@hidden || echo $(srcdir)/address@hidden 
> >> >address@hidden
> >> +  srcdir=''; \
> >> +    test -f ./address@hidden || srcdir=$(srcdir)/; \
> >> +    $(edit) address@hidden >address@hidden

> Ralf Wildenhues <address@hidden> writes:
> > This doesn't help backslashes, which I assume was the major problem spot
> > here -- white space in $(srcdir) isn't possible with portable make.  But
> > Automake anyway doesn't cope well with backslashes either.  OTOH, your
> > change doesn't look like it would hurt much; maybe it even saves an exec.
> 
> It also saves when srcdir begins with '-', admittedly low probability.

I thought file names beginning with a hyphen were explicitly discouraged
in POSIX, but I can't find a statement to this extent in SUSv3 now.
Anyway, people doing that need to be slapped even harder that those
putting spaces in file names, IMVHO.

> >> +  AS_ECHO(["$as_me: option \`$[1]'\'' requires an argument"]) >&2
> >> +  AS_ECHO(["$help"]) >&2
> >> +  exit 1

> > Why do these need AS_ECHO?  More generally, what was the rationale for
> > changing some but not all instances?
> 
> These use AS_ECHO since as_me and help might contain backslashes.  In
> general, the idea is to use AS_ECHO when the argument might start with
> "-" or might contain backslashes, and to use plain 'echo' otherwise.

Hmm.  Backslashes in file names are really unlikely though.
(All shells with echo that interpret backslashes on w32
platforms have been slapped to sanity, last I remember.)

> >> -       echo "$usage"; exit ;;
> >> +       AS_ECHO(["$usage"]); exit ;;
> >
> > This one will break on Solaris 2.7, when a few (3 or 4) more options
> > are added to autoconf.  OTOH I don't see the necessity for it (no
> > dangerous backslashes, no leading hyphen in $usage).
> 
> $usage might contain backslashes.

Not really.  I've yet to see a 'program --help' output that contained
backslashes.  But oh well, I guess it doesn't matter now.

> I hope this explains the other AS_ECHO instances that you thought
> were superfluous and/or dangerous.

Yes, I guess.

> >> -# FIXME: This mishandles values that end in newlines, or have backslashes,
> >> -# or are '-n'.  Fixing this will require changing the API.
> >> +# FIXME: This mishandles values that end in newlines.
> >> +# Fixing this will require changing the API.
> 
> > This should have exposure in the testsuite, so the claim is a founded
> > one.
> 
> I suppose so, but I would rather spend my time fixing the API.

Oh, sure.  I should have added "(note to self)".

> I proposed something along those lines a while ago, but haven't
> had a chance to finish it up.

Do you know a use case that needs the newlines-at-end differences?


Now to the patch.  Besides all the technicalities (see inline comments)
this change causes quite some slowdown on script execution.  Can we at
least do some optimization, say, by using plain echo if AS_IF_LITERAL
matches and no backslashes and no hyphen at the beginning?  I mean,
the new $as_echo is used veery many times during a typical configure
execution now, and this patch reacts upon the very first bug report
I've seen to this extent.

The other possibility for optimization would be to take this test into
account in our search for a better shell: prefer a shell with an echo
that does what we want.  This is getting closer and closer to what
Libtool actually tries to do; I can only guess you don't like the way
it's done but I think it's not bad to at least consider it.  It would
also allow the optimization of doing the expensive part of the
_AS_ECHO_PREPARE test only twice per configure invocation, not twice
per candidate shell (the factor two comes from us doing it in configure
and in config.status; another instance comes in when the $x.lineno
scripts are used, which is the case on Solaris).  I see that there are
a couple of AS_ECHO invocations (in error paths) before the expansion of
_AS_DETECT_BETTER_SHELL, but sacrificing them would not bother me.

(Thinking out loud, not requesting a patch.)


>       * lib/m4sugar/m4sh.m4 (_AS_ECHO_PREPARE): Port to Solaris 7,
>       where "/usr/bin/printf '%s\n' S" dumps core if S is long.
>       This is Sun bug 4206210.  Problem reportd by Ralf Wildenhues.

> --- lib/m4sugar/m4sh.m4       17 Nov 2006 21:04:54 -0000      1.202
> +++ lib/m4sugar/m4sh.m4       25 Nov 2006 09:54:32 -0000
> @@ -808,25 +808,34 @@ m4_defun([_AS_ECHO_PREPARE],
>  [[as_nl='
>  '
>  export as_nl
> -case `(printf %s foo) 2>/dev/null` in
> -foo)
> +# Printing a 2060-byte string crashes Solaris 7 /usr/bin/printf.
> +as_echo='\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\'
> +as_echo=$as_echo$as_echo$as_echo$as_echo$as_echo
> +as_echo=$as_echo$as_echo$as_echo$as_echo

This is apparently too short to trigger the failure in the scripted
case.  I don't know why that is, it may have to do with the size of
the environment, or whatnot else.  (How would I know?)
The patch below makes the string a bit longer.

> +if test "X`(printf %s $as_echo) 2>/dev/null`" = "X$as_echo"; then

With this, the 
| ./config.status[49]: 19502 Segmentation Fault(coredump)

message still escapes; maybe due to the shell optimizing out the second
fork?  Wrapping the whole `test' command in a `( ) 2>/dev/null' works
better.

>    as_echo='printf %s\n'
> +  as_echo_n='printf %s'
> +else
> +  if test "X`PATH=/usr/ucb; echo -n -n $as_echo`" = "X-n $as_echo"; then

This setting of PATH seems to work; i.e., the following echo calls
/usr/ucb/echo.  But I don't understand why.  Shouldn't the builtin
echo still precede?
(I've changed this in the patch below, but read further.)

> +    as_echo_body='PATH=/usr/ucb; eval echo -n "$1$as_nl"'

This doesn't work.  It leads to
  sh -c '$as_echo_body'

so only the inner sh will expand the parameter `as_echo_body' (which is
intended).  But then, it will see the semicolon only after parameter
expansion.  But parameter expansoin may not cause a command to be split
in two:
| X: PATH=/usr/ucb;: No such file or directory

The other issue (several instances) is that `$1' must not be expanded
by m4 here.

> +    as_echo_n='sh -c $as_echo_n_body X'
> +  fi
> +  export as_echo_body
>    as_echo='sh -c $as_echo_body X'
> -  as_echo_n='sh -c $as_echo_n_body X';;

Hmm.  That `X' will be the zeroth argument of the resulting script, see
the error message above.  Wouldn't a more descriptive one actually be
better?  Like, say, as_echo, so that users stumbling over an as-yet
unknown bug have a chance to find the culprit faster?

Last but not least, do you think using plain `sh' rather than, say,
$CONFIG_SHELL if that is set, is the right way to go?  Wasn't there
some issue with that on some of the rarer w32 systems?  We can't use
$SHELL here yet: that has not been initialized.
(I haven't changed this yet.  It may be yet another reason to move
this into the better shell search.)

OK to apply the patch below?  With it, Solaris 2.6, 7, 8, 9, and 10
seem to cope.

Cheers,
Ralf

2006-12-01  Ralf Wildenhues  <address@hidden>

        * lib/m4sugar/m4sh.m4 (_AS_ECHO_PREPARE): Use a longer test
        string for more reliable failure.  Wrap the entire test that
        causes the broken Solaris printf to dump core, in a subshell,
        so the segmentation fault message is reliably suppressed.
        Fix shell expansion errors by using /usr/ucb/echo always;
        avoid an error on systems without it by another subshell.
        Avoid m4 expansion of `$1'.  Set the zeroth argument of the
        subshell-$as_echo to `as_echo', for better error message.

Index: lib/m4sugar/m4sh.m4
===================================================================
RCS file: /cvsroot/autoconf/autoconf/lib/m4sugar/m4sh.m4,v
retrieving revision 1.203
diff -u -r1.203 m4sh.m4
--- lib/m4sugar/m4sh.m4 25 Nov 2006 09:57:34 -0000      1.203
+++ lib/m4sugar/m4sh.m4 1 Dec 2006 06:19:23 -0000
@@ -808,21 +808,21 @@
 [[as_nl='
 '
 export as_nl
-# Printing a 2060-byte string crashes Solaris 7 /usr/bin/printf.
+# Printing a long string crashes Solaris 7 /usr/bin/printf.
 
as_echo='\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\'
 as_echo=$as_echo$as_echo$as_echo$as_echo$as_echo
-as_echo=$as_echo$as_echo$as_echo$as_echo
-if test "X`(printf %s $as_echo) 2>/dev/null`" = "X$as_echo"; then
+as_echo=$as_echo$as_echo$as_echo$as_echo$as_echo$as_echo
+if (test "X`printf %s $as_echo`" = "X$as_echo") 2>/dev/null; then
   as_echo='printf %s\n'
   as_echo_n='printf %s'
 else
-  if test "X`PATH=/usr/ucb; echo -n -n $as_echo`" = "X-n $as_echo"; then
-    as_echo_body='PATH=/usr/ucb; eval echo -n "$1$as_nl"'
+  if test "X`(/usr/ucb/echo -n -n $as_echo) 2>/dev/null`" = "X-n $as_echo"; 
then
+    as_echo_body='eval /usr/ucb/echo -n "$][1$as_nl"'
     as_echo_n='/usr/ucb/echo -n'
   else
-    as_echo_body='eval expr "X$1" : "X\\(.*\\)"'
+    as_echo_body='eval expr "X$][1" : "X\\(.*\\)"'
     as_echo_n_body='eval
-      arg=$1;
+      arg=$][1;
       case $arg in
       *"$as_nl"*)
        expr "X$arg" : "X\\(.*\\)$as_nl";
@@ -831,10 +831,10 @@
       expr "X$arg" : "X\\(.*\\)" | tr -d "$as_nl"
     '
     export as_echo_n_body
-    as_echo_n='sh -c $as_echo_n_body X'
+    as_echo_n='sh -c $as_echo_n_body as_echo'
   fi
   export as_echo_body
-  as_echo='sh -c $as_echo_body X'
+  as_echo='sh -c $as_echo_body as_echo'
 fi
 ]])# _AS_ECHO_PREPARE
 




reply via email to

[Prev in Thread] Current Thread [Next in Thread]