Re: [PATCH] use m4_require to implement AS_REQUIRE
> The point is that text goes into a diversion only when _you are not doing > argument collection_ - in other words, the macro call must be expanded at > the outermost level. Any macro that potentially changes the current > diversion must be quoted, rather than expanded in advance, when handed as > an argument to another macro; otherwise, the text output while the > diversion change is in effect doesn't go to the changed diversion, but to > the argument collection. Yeah, that's the similar testcase I got: m4_init m4_define([FROBIT], [m4_divert_text([6], [in diversion _m4_divert_diversion])$1]) m4_define([SHOWIT], [$[]1 is $1]) m4_define([TEXT], [FROBIT(text)]) m4_divert_pop([KILL]) m4_divert_push([1000])dnl SHOWIT([TEXT]) and that's also the reason why the original testcase was failing. Paolo
Re: [PATCH] use m4_require to implement AS_REQUIRE
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 According to Paolo Bonzini on 10/17/2008 6:41 AM: > I still cannot understand why the diversion ends up like this though. > > Here is an even smaller testcase: > > AS_INIT > m4_defun([AS_FROBIT_PREPARE], [frob=1]) > m4_defun([AS_FROBIT], [AS_REQUIRE([AS_FROBIT_PREPARE])$1]) > m4_defun([AS_DOIT], [: $1]) > m4_define([XXFOO], [AS_FROBIT(exit 1)]) > AS_DOIT(XXFOO) > > which gives > > : frob=1 > exit 1 > > as is, and > > frob=1 > : exit 1 > > when XXFOO is quoted. > > So, if this is a bug (and it looks like one, though very minor because > proper quoting works around it) we also have a testcase. :-) This is a limitation of m4 diversions. Something even simpler: $ echo 'm4_define([foo],[m4_divert(1)hi m4_divert(0)])m4_divert[]foo:' | m4 -Ilib m4sugar/m4sugar.m4 - : hi $ echo 'm4_define([foo],[m4_divert(1)hi m4_divert(0)])m4_divert[]m4_echo(foo):' | m4 -Ilib m4sugar/m4sugar.m4 - hi : The point is that text goes into a diversion only when _you are not doing argument collection_ - in other words, the macro call must be expanded at the outermost level. Any macro that potentially changes the current diversion must be quoted, rather than expanded in advance, when handed as an argument to another macro; otherwise, the text output while the diversion change is in effect doesn't go to the changed diversion, but to the argument collection. M4 1.6 will add an optional argument to the divert builtin, such that divert(1,text) places text in diversion 1, regardless of whether it is being expanded during argument collection. But unfortunately, we cannot rely on that. Alas, m4 lacks the ability to inform you when a macro is being expanded during argument collection, so we can't even protect against these sorts of subtle diversion bugs (maybe I should add a macro that expands to the current nesting depth?). - -- Don't work too hard, make some time for fun as well! Eric Blake [EMAIL PROTECTED] -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.9 (Cygwin) Comment: Public key at home.comcast.net/~ericblake/eblake.gpg Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org iEYEARECAAYFAkj4jB8ACgkQ84KuGfSFAYCjMwCgnBIjug/gOs5Dczqp7Mk8Dm2L 0qYAnixI6hsUVLiwDDkbLtqNKD2to7d3 =aaR6 -END PGP SIGNATURE-
Re: [PATCH] use m4_require to implement AS_REQUIRE
Eric Blake wrote: > According to Paolo Bonzini on 10/17/2008 6:19 AM: >> That's related to tmp being underquoted, see the following even more >> reduced testcase > >> AS_INIT >> m4_defun([AS_DOIT], [: $1]) >> m4_define([tmp], [AS_TR_SH(foo)]) >> AS_DOIT(tmp) > >> => : # Sed expression to map a string onto a valid variable name. >>as_tr_sh="eval sed 'y%*+%pp%;s%^_$as_cr_alnum%_%g'" > >>foo > > Thanks; that was what I was missing. I'm trying to document and test > AS_VAR, and didn't realize that the temporary variable still needed > quoting until the last minute. It turns out that the m4 temporary > variable created by AS_VAR_PUSHDEF must be expanded only at the outermost > level (diversions only work for outermost expansions; otherwise the text > is picked up by argument collection). AS_VAR patch coming up soon, and > nothing more to see in this thread. I still cannot understand why the diversion ends up like this though. Here is an even smaller testcase: AS_INIT m4_defun([AS_FROBIT_PREPARE], [frob=1]) m4_defun([AS_FROBIT], [AS_REQUIRE([AS_FROBIT_PREPARE])$1]) m4_defun([AS_DOIT], [: $1]) m4_define([XXFOO], [AS_FROBIT(exit 1)]) AS_DOIT(XXFOO) which gives : frob=1 exit 1 as is, and frob=1 : exit 1 when XXFOO is quoted. So, if this is a bug (and it looks like one, though very minor because proper quoting works around it) we also have a testcase. :-) Paolo
Re: [PATCH] use m4_require to implement AS_REQUIRE
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 According to Paolo Bonzini on 10/17/2008 6:19 AM: > That's related to tmp being underquoted, see the following even more > reduced testcase > > AS_INIT > m4_defun([AS_DOIT], [: $1]) > m4_define([tmp], [AS_TR_SH(foo)]) > AS_DOIT(tmp) > > => : # Sed expression to map a string onto a valid variable name. >as_tr_sh="eval sed 'y%*+%pp%;s%^_$as_cr_alnum%_%g'" > >foo Thanks; that was what I was missing. I'm trying to document and test AS_VAR, and didn't realize that the temporary variable still needed quoting until the last minute. It turns out that the m4 temporary variable created by AS_VAR_PUSHDEF must be expanded only at the outermost level (diversions only work for outermost expansions; otherwise the text is picked up by argument collection). AS_VAR patch coming up soon, and nothing more to see in this thread. - -- Don't work too hard, make some time for fun as well! Eric Blake [EMAIL PROTECTED] -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.9 (Cygwin) Comment: Public key at home.comcast.net/~ericblake/eblake.gpg Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org iEYEARECAAYFAkj4hNUACgkQ84KuGfSFAYDXPQCeJ0tMUJ6RDJwUpMy1qnYVrOXJ 0EwAn1Kj8qiG03C64tRqqTUS+eZvvION =IA2p -END PGP SIGNATURE-
Re: [PATCH] use m4_require to implement AS_REQUIRE
> It's still not perfect. This file is failing: > > AS_INIT > foo=bar > AS_VAR_PUSHDEF([tmp], [foo]) > AS_VAR_IF(tmp, [bar], [echo yes], [echo no]) > AS_VAR_POPDEF([tmp]) > > because it is expanding _AS_TR_SH_PREPARE in the middle of AS_VAR_IF > rather than hoisting it into M4SH-INIT right after _AS_CR_PREPARE: That's related to tmp being underquoted, see the following even more reduced testcase AS_INIT m4_defun([AS_DOIT], [: $1]) m4_define([tmp], [AS_TR_SH(foo)]) AS_DOIT(tmp) => : # Sed expression to map a string onto a valid variable name. as_tr_sh="eval sed 'y%*+%pp%;s%^_$as_cr_alnum%_%g'" foo AS_INIT m4_defun([AS_DOIT], [: $1]) m4_define([tmp], [AS_TR_SH(foo)]) AS_DOIT([tmp]) => # Sed expression to map a string onto a valid variable name. as_tr_sh="eval sed 'y%*+%pp%;s%[^_$as_cr_alnum]%_%g'" : foo Paolo
Re: [PATCH] use m4_require to implement AS_REQUIRE
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 According to Paolo Bonzini on 10/12/2008 10:20 AM: > The implementation is easy, though I had to try several times before > getting it right. Anyway, the attached patch works and adds a testcase > for the first behavior. The second bug is already tested by the existing > AS_REQUIRE_SHELL_FN test, which however needs to be modified in order > to really test this behavior. It's still not perfect. This file is failing: AS_INIT foo=bar AS_VAR_PUSHDEF([tmp], [foo]) AS_VAR_IF(tmp, [bar], [echo yes], [echo no]) AS_VAR_POPDEF([tmp]) because it is expanding _AS_TR_SH_PREPARE in the middle of AS_VAR_IF rather than hoisting it into M4SH-INIT right after _AS_CR_PREPARE: eval as_val=\$# Sed expression to map a string onto a valid variable name. as_tr_sh="eval sed 'y%*+%pp%;s%^_$as_cr_alnum%_%g'" foo I'm trying to figure out the correct approach, but it seems like we can't dump anything into the final diversion until all of the dependency ordering has been collected into the growth area. - -- Don't work too hard, make some time for fun as well! Eric Blake [EMAIL PROTECTED] -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.9 (Cygwin) Comment: Public key at home.comcast.net/~ericblake/eblake.gpg Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org iEYEARECAAYFAkj4dqoACgkQ84KuGfSFAYCQ9wCfUkSHyfiYmIVfQTvLFB+nFsVV 9sEAoI1e53Ise/EXQa+i2U/k+BtGhOp9 =sQ6V -END PGP SIGNATURE-
Re: [PATCH] use m4_require to implement AS_REQUIRE
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 According to Paolo Bonzini on 10/12/2008 10:20 AM: > Hi, I know I should be writing testcases for AS_ME_PREPARE. :-) But > while I was looking at adding shell functions to Autoconf proper, I > decided that the current basic implementation of AS_REQUIRE is not > scalable. OK, I've reviewed this. I think the following quoting change is needed, but then the patch can be applied. > +m4_defun([AS_REQUIRE], > +[m4_define([_m4_divert_desired], m4_default_quoted([$3], [M4SH-INIT]))dnl > +m4_if(m4_eval(_m4_divert(_m4_divert_dump) <= > _m4_divert(_m4_divert_desired)), 1, These two lines do the wrong thing if INIT is m4_define'd at this point, since you are defining _m4_divert_desired to an unquoted name (the quoting provided by m4_default_quoted is stripped by the argument parsing of m4_define). One way to work around this is to make sure _m4_divert_desired expands to a quoted name, by changing the first line: m4_define([_m4_divert_desired], [m4_default_quoted([$3], [M4SH-INIT])])dnl > + [m4_require([$1], [$2])], > + [m4_divert_require([_m4_divert_desired], [$1], [$2])])]) Then, as you observed, if m4_divert_require expands its first argument, this will do the right thing. > +# m4_divert_require(DIVERSION, NAME-TO-CHECK, [BODY-TO-EXPAND]) > +# -- > +# Same as m4_require, but BODY-TO-EXPAND goes into the named diversion; > +# requirements still go in the current diversion though. > +# > +m4_define([m4_divert_require], > +m4_do([[m4_ifdef([_m4_expanding($2)], > + [m4_fatal([$0: circular dependency of $2])])]], > + [[m4_ifdef([_m4_divert_dump], [], > + [m4_fatal([$0($2): cannot be used outside of an m4_defun'd > macro])])]], > + [[m4_provide_if([$2], > + [], > + [_m4_require_call([$2], [$3], [$1])])]])) m4_do is a bit of an overkill (yes, I know I've used it in places, but not recently). If you want, you could rewrite this as: m4_define([m4_divert_require], [m4_ifdef([_m4_expanding($2)], [m4_fatal([$0: circular dependency of $2])])]dnl [m4_ifdef([_m4_divert_dump], [], [m4_fatal([$0($2): cannot be used outside of an m4_defun'd macro])])]dnl [m4_provide_if([$2], [], [_m4_require_call([$2], [$3], [$1])])]) > + > # m4_defun(NAME, EXPANSION) > # - > # Define a macro which automatically provides itself. Add machinery > @@ -1681,12 +1695,13 @@ m4_do([[m4_ifdef([_m4_expanding($1)], > m4_bmatch([$0], [^AC_], [[AC_DEFUN]], [[m4_defun]])['d macro])])]], >[[m4_provide_if([$1], > [], > - [_m4_require_call([$1], [$2])])]])) > + [_m4_require_call([$1], [$2], > [_m4_defn([_m4_divert_dump])])])]])) With the quoting change to AS_REQUIRE, this means that all clients of _m4_require_call are passing text that still needs expansion as the third argument. > - [[m4_divert(_m4_defn([_m4_divert_dump]))]], > + [[m4_divert($3)]], so I agree with your analysis that you don't need to quote $3 here. [This is not the only solution; you could also quote $3 here and change all callers of _m4_require_call to pass a diversion name, rather than macro text that expands to a diversion name, but as you noted in your follow-up, that adjusts more lines of your original patch.] > ## ## > ## AS_REQUIRE_SHELL_FN and m4_require. ## > ## ## > > # Hypothesis: M4sh expands the requirements of AS_REQUIRE_SHELL_FN > -# in the main diversion, and not in M4SH-INIT. > +# in M4SH-INIT-FN. This changed after Autoconf 2.63. AS_REQUIRE_SHELL_FN didn't even exist in 2.63, so I'd just strike the last sentence of this comment. And to prove the point of quoting, I'd add an m4_define([INIT], [oops]) prior to the AS_INIT in this test. - -- Don't work too hard, make some time for fun as well! Eric Blake [EMAIL PROTECTED] -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.9 (Cygwin) Comment: Public key at home.comcast.net/~ericblake/eblake.gpg Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org iEYEARECAAYFAkj0l7IACgkQ84KuGfSFAYAvSgCfeDIMQxQzbT1BIQiGJLb2OQVc 5mcAoJarILT046IZF+YpZ1dNlU04g0bn =HS0U -END PGP SIGNATURE-
Re: [PATCH] use m4_require to implement AS_REQUIRE
> I still don't quite understand (but at this point have to admit that I
> haven't looked at the implementation _at all_). If AC_CHECK_FUNC
> requires the preparation of AC_LINK_IFELSE, then the latter should be
> expanded outside and before the body of the former. No?
After this patch, yes. :-)
Without it, AS_REQUIRE is a hack that does not guarantee "atomic"
expansion of macros; it just slapped text into the appropriate
diversion, causing intermixed expansion like this one:
ac_c_check_func() {
if test $ac_cv...; then
...
else
ac_c_link_ifelse() {
}
fi
}
and requiring commits like 34b79a8 and 926ed6a9 which could even be
reverted after this patch goes in (though I'd rather not do it to avoid
excessive churn -- there is more than enough already). Look at those
seemingly innocent patches and you'll realize that, while tolerable, the
situation was anyway all but optimal.
> Ralf (and yes, just ignore me unless you have extra time to explain)
No problem. Explaining things twice makes sure you have really
understood them. Also, since I submitted the first shell function
patches 4-5 years ago, and so many prerequisites jump up now years
later, I'd better explain things as much as possible to make sure I'm
doing them right.
Paolo
Re: [PATCH] use m4_require to implement AS_REQUIRE
* Paolo Bonzini wrote on Sun, Oct 12, 2008 at 07:40:58PM CEST:
>
> >> ac_c_check_func() {
> >> if test $ac_cv...; then
> >> ...
> >> else
> >> ac_c_link_ifelse() {
> >> }
> >> fi
> >> }
> >>
> >> which does not work of course.
> >
> > For me mere mortal, who doesn't wade in the problems you've studied in
> > detail, what exactly is problematic here?
>
> Suppose you have:
>
> ac_c_check_func function_that_is_in_cached
> ... some other test that does uses AC_LINK_IFELSE ...
>
> ac_c_link_ifelse won't be defined.
I still don't quite understand (but at this point have to admit that I
haven't looked at the implementation _at all_). If AC_CHECK_FUNC
requires the preparation of AC_LINK_IFELSE, then the latter should be
expanded outside and before the body of the former. No?
Cheers,
Ralf (and yes, just ignore me unless you have extra time to explain)
Re: [PATCH] use m4_require to implement AS_REQUIRE
> Assuming that the existing code was not underquoted, then either I > underquote here, or I do that in m4_require. I chose to underquote > here. I can try the other version, underquoting the _m4_defn in > m4_require and using [$3] here. I tried it, and I had to underquote not only the _m4_defn in m4_require (which might be okay, since defn overquotes), but also the call to m4_divert_require: - [m4_divert_require([_m4_divert_desired], [$1], [$2])])]) + [m4_divert_require(_m4_divert_desired, [$1], [$2])])]) So I'm inclined to leave it as in the already-posted patch. Paolo
Re: [PATCH] use m4_require to implement AS_REQUIRE
>>ver usertime size >>2.61 18.154s1037578 >>2.63 13.192s1055693 >>+funcs1 13.907s886574 >>+funcs2 11.467s780238 > > I take it these are speedups in 'autoconf' time, and not in 'configure' time? Yes, of course. >> # BODY-TO-EXPAND is some initialization which must be expanded in the >> -# given diversion when expanded (required or not). This is very >> -# different from m4_require. For instance: >> +# given diversion when expanded (required or not). The expansion >> +# goes in the named diversion or an earlier one. > > Hmmm. M4SH-INIT is at diversion 6. If this is identical to m4_require, > then it doesn't take very many nested AS_REQUIRES before we have dropped > to diversion 0, or even worse to diversion -1. No, the required macros are built into the _m4_divert_grow diversions, that count down from 1, and dumped "atomically" into the _m4_divert_dump diversion (for m4_require) or into the specified diversion (for m4_divert_require). >> +m4_defun([AS_REQUIRE], >> +[m4_define([_m4_divert_desired], m4_default_quoted([$3], [M4SH-INIT]))dnl >> +m4_if(m4_eval(_m4_divert(_m4_divert_dump) <= >> _m4_divert(_m4_divert_desired)), 1, > > Or maybe this eval is trying to guarantee just that? Again, I'm typing > this without a thorough review; it is just first impressions. The eval is basically subsuming 926ed6a97561e0a77de18011b29c6bcaaff4c365 (place _AS_UNSET_PREPARE in the right diversion): if the outermost macro is expanded before the requested diversion, we do not move its shell requirements to M4SH-INIT or M4SH-INIT-FN. >> @@ -1698,7 +1713,7 @@ m4_provide_if([$1], >>[], >>[m4_warn([syntax], >> [$1 is m4_require'd but not m4_defun'd])])]], >> - [[m4_divert(_m4_defn([_m4_divert_dump]))]], >> + [[m4_divert($3)]], > > Underquoted? Assuming that the existing code was not underquoted, then either I underquote here, or I do that in m4_require. I chose to underquote here. I can try the other version, underquoting the _m4_defn in m4_require and using [$3] here. Paolo
Re: [PATCH] use m4_require to implement AS_REQUIRE
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 According to Paolo Bonzini on 10/12/2008 10:20 AM: > Hi, I know I should be writing testcases for AS_ME_PREPARE. :-) But > while I was looking at adding shell functions to Autoconf proper, I > decided that the current basic implementation of AS_REQUIRE is not > scalable. True on both points. ;-) > I chose not to add a third argument to m4_require, as I did for > AS_REQUIRE, because m4_require is on an expensive path. Should > m4_divert_require be documented? We can leave it undocumented for a release, until we're happy with it. For that matter, a lot of m4sugar diversion magic is (intentionally) underdocumented, since we should generally be providing sane wrapper macros that take care of the diversions under the hood, rather than forcing the user to directly think about diversions. > > I have not run the entire testsuite (I will before committing) but I > have run the m4sugar and m4sh testsuites, as well as tests related to > the shell functions I already added. While testing, I noticed the effect > of the speedups introduced after 2.61; on GNU Smalltalk I have: > >ver usertime size >2.61 18.154s1037578 >2.63 13.192s1055693 > >+funcs1 13.907s886574 >+funcs2 11.467s780238 I take it these are speedups in 'autoconf' time, and not in 'configure' time? > In the meanwhile, ok to apply after proper testing? I haven't looked closely at contents yet; please give me a bit more time to properly review before applying. But here's a preliminary nits review: > # BODY-TO-EXPAND is some initialization which must be expanded in the > -# given diversion when expanded (required or not). This is very > -# different from m4_require. For instance: > +# given diversion when expanded (required or not). The expansion > +# goes in the named diversion or an earlier one. Hmmm. M4SH-INIT is at diversion 6. If this is identical to m4_require, then it doesn't take very many nested AS_REQUIRES before we have dropped to diversion 0, or even worse to diversion -1. Even a shorter chain would get confusing if we start impinging on HEADER_COPYRIGHT. Do we need to see the named diversions in m4sh renumbered to leave some reserved diversion numbers, to guarantee that AS_REQUIRE prerequisites appear cleanly after M4SH-SANITIZE. > +m4_defun([AS_REQUIRE], > +[m4_define([_m4_divert_desired], m4_default_quoted([$3], [M4SH-INIT]))dnl > +m4_if(m4_eval(_m4_divert(_m4_divert_dump) <= > _m4_divert(_m4_divert_desired)), 1, Or maybe this eval is trying to guarantee just that? Again, I'm typing this without a thorough review; it is just first impressions. > @@ -1698,7 +1713,7 @@ m4_provide_if([$1], > [], > [m4_warn([syntax], > [$1 is m4_require'd but not m4_defun'd])])]], > - [[m4_divert(_m4_defn([_m4_divert_dump]))]], > + [[m4_divert($3)]], Underquoted? - -- Don't work too hard, make some time for fun as well! Eric Blake [EMAIL PROTECTED] -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.9 (Cygwin) Comment: Public key at home.comcast.net/~ericblake/eblake.gpg Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org iEYEARECAAYFAkjzQDgACgkQ84KuGfSFAYC+wQCeJBXOYvXkA6eIpWmXo/W9BV1M hssAnj6K12sk6yXbRoSpWiIfgBkw+dfg =tIjE -END PGP SIGNATURE-
Re: [PATCH] use m4_require to implement AS_REQUIRE
>> ac_c_check_func() {
>> if test $ac_cv...; then
>> ...
>> else
>> ac_c_link_ifelse() {
>> }
>> fi
>> }
>>
>> which does not work of course.
>
> For me mere mortal, who doesn't wade in the problems you've studied in
> detail, what exactly is problematic here?
Suppose you have:
ac_c_check_func function_that_is_in_cached
... some other test that does uses AC_LINK_IFELSE ...
ac_c_link_ifelse won't be defined.
Re: [PATCH] use m4_require to implement AS_REQUIRE
Hi Paolo,
* Paolo Bonzini wrote on Sun, Oct 12, 2008 at 06:20:53PM CEST:
> Hi, I know I should be writing testcases for AS_ME_PREPARE. :-) But
> while I was looking at adding shell functions to Autoconf proper, I
> decided that the current basic implementation of AS_REQUIRE is not
> scalable.
>
> In particular, AC_CHECK_FUNC will need its own shell function, plus
> the one for AC_LINK_IFELSE. If AC_LINK_IFELSE has not been expanded yet,
> then something like this comes out of m4:
>
> ac_c_check_func() {
> if test $ac_cv...; then
> ...
> else
> ac_c_link_ifelse() {
> }
> fi
> }
>
> which does not work of course.
For me mere mortal, who doesn't wade in the problems you've studied in
detail, what exactly is problematic here?
(Not that I expect to review your patch, but I kind of hope the question
would be easy and quick for you anyway)
Thanks!
Ralf
