Eric Blake <ebb9 <at> byu.net> writes: > > Both patches look like the correct approach; can you add a couple of > > lines to the documentation of the require mechanism in m4sugar.m4? > > Thanks for the review. I've already written NEWS and autoconf.texi patch, > but had not yet done the m4sugar.m4 doc patch. I'll complete that, post > the updated patch series, and push.
Here's what I'm pushing. I took the liberty in adding a FAQ node to the manual, since I'm sure that we will start getting questions about the new warning. >From cf7d7f2e823c776df17de4aa1e85cec73fd49489 Mon Sep 17 00:00:00 2001 From: Eric Blake <[email protected]> Date: Tue, 20 Jan 2009 14:03:59 -0700 Subject: [PATCH] Warn if macro is provided before indirectly required. * lib/m4sugar/m4sugar.m4 (m4_provide): Track the set of all macros provided since last outermost defun. (_m4_defun_pro_outer): Empty the set. (_m4_require_call): Distinguish between direct and indirect requires, and remove required macros from the set. (m4_require): Check the set, in order to warn. * tests/m4sugar.at (m4@&t...@_require: nested): Remove xfail, and add test case for direct requires. Signed-off-by: Eric Blake <[email protected]> --- ChangeLog | 12 +++++++ lib/m4sugar/m4sugar.m4 | 78 ++++++++++++++++++++++++++++++++++++++++-------- tests/m4sugar.at | 57 ++++++++++++++++++++++++++++++----- 3 files changed, 126 insertions(+), 21 deletions(-) diff --git a/ChangeLog b/ChangeLog index 6023598..f03804c 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,15 @@ +2009-01-21 Eric Blake <[email protected]> + + Warn if macro is provided before indirectly required. + * lib/m4sugar/m4sugar.m4 (m4_provide): Track the set of all macros + provided since last outermost defun. + (_m4_defun_pro_outer): Empty the set. + (_m4_require_call): Distinguish between direct and indirect + requires, and remove required macros from the set. + (m4_require): Check the set, in order to warn. + * tests/m4sugar.at (m4@&t...@_require: nested): Remove xfail, and add + test case for direct requires. + 2009-01-20 Eric Blake <[email protected]> Clean up some bugs caught by preliminary dependency validation. diff --git a/lib/m4sugar/m4sugar.m4 b/lib/m4sugar/m4sugar.m4 index 3b28512..1457aef 100644 --- a/lib/m4sugar/m4sugar.m4 +++ b/lib/m4sugar/m4sugar.m4 @@ -1460,10 +1460,11 @@ m4_define([m4_undivert], # difficult part is the proper expansion of macros when they are # m4_require'd. # -# The implementation is based on two ideas, (i) using diversions to +# The implementation is based on three ideas, (i) using diversions to # prepare the expansion of the macro and its dependencies (by Franc,ois -# Pinard), and (ii) expand the most recently m4_require'd macros _after_ -# the previous macros (by Axel Thimm). +# Pinard), (ii) expand the most recently m4_require'd macros _after_ +# the previous macros (by Axel Thimm), and (iii) track instances of +# provide before require (by Eric Blake). # # # The first idea: why use diversions? @@ -1597,8 +1598,8 @@ m4_define([m4_undivert], # GROW: # BODY: TEST2a; TEST3; TEST2b: TEST1 # -# The idea is simple, but the implementation is a bit evolved. If you -# are like me, you will want to see the actual functioning of this +# The idea is simple, but the implementation is a bit involved. If +# you are like me, you will want to see the actual functioning of this # implementation to be convinced. The next section gives the full # details. # @@ -1649,7 +1650,7 @@ m4_define([m4_undivert], # BODY: empty # GROW - 1: TEST2a # diversions: GROW - 1, GROW, BODY |- -# Than the content of the temporary diversion is moved to DUMP and the +# Then the content of the temporary diversion is moved to DUMP and the # temporary diversion is popped. # DUMP: BODY # BODY: TEST2a @@ -1669,7 +1670,7 @@ m4_define([m4_undivert], # BODY: TEST2a # GROW - 2: TEST3 # diversions: GROW - 2, GROW - 1, GROW, BODY |- -# Than the diversion is appended to DUMP, and popped. +# Then the diversion is appended to DUMP, and popped. # DUMP: BODY # BODY: TEST2a; TEST3 # diversions: GROW - 1, GROW, BODY |- @@ -1697,6 +1698,52 @@ m4_define([m4_undivert], # diversions: BODY |- # # +# The third idea: track macros provided before they were required +# --------------------------------------------------------------- +# +# Using just the first two ideas, Autoconf 2.50 through 2.63 still had +# a subtle bug for more than seven years. Let's consider the +# following example to explain the bug: +# +# | m4_defun([TEST1], [1]) +# | m4_defun([TEST2], [2[]REQUIRE([TEST1])]) +# | m4_defun([TEST3], [3 TEST1 REQUIRE([TEST2])]) +# | TEST3 +# +# After the prologue of TEST3, we are collecting text in GROW with the +# intent of dumping it in BODY during the epilogue. Next, we +# encounter the direct invocation of TEST1, which provides the macro +# in place in GROW. From there, we encounter a requirement for TEST2, +# which must be collected in a new diversion. While expanding TEST2, +# we encounter a requirement for TEST1, but since it has already been +# expanded, the Axel Thimm algorithm states that we can treat it as a +# no-op. But that would lead to an end result of `2 3 1', meaning +# that we have once again output a macro (TEST2) prior to its +# requirements (TEST1). +# +# The problem can only occur if a single defun'd macro first provides, +# then later indirectly requires, the same macro. Note that directly +# expanding then requiring a macro is okay, since the requirement will +# be a no-op; the problem is only present if the requirement is nested +# inside a context that will be hoisted in front of the outermost +# defun'd macro. In other words, we must be careful not to warn on: +# +# | m4_defun([TEST1], [1]) +# | m4_defun([TEST2], [TEST1 REQUIRE([TEST1])]) +# +# The implementation of the warning involves tracking the set of +# macros which have been provided since the start of the outermost +# defun'd macro (the set is named _m4_provide). When starting an +# outermost macro, the set is emptied; when a macro is provided, it is +# added to the set; when require expands the body of a macro, it is +# removed from the set; and when a macro is indirectly required (that +# is, when m4_require detects a nested call), the set is checked. If +# a macro is in the set, then it has been provided before it was +# required, and a warning is issued because the output will be out of +# order and the user must fix her macros to reflect the true +# dependencies. +# +# # 2. Keeping track of the expansion stack # ======================================= # @@ -1772,6 +1819,7 @@ m4_define([_m4_defun_pro], [m4_expansion_stack_push([$1])m4_pushdef([_m4_expanding($1)])]) m4_define([_m4_defun_pro_outer], +[m4_set_delete([_m4_provide])]dnl [m4_pushdef([_m4_divert_dump], m4_divnum)m4_divert_push([GROW])]) # _m4_defun_epi(MACRO-NAME) @@ -1935,8 +1983,10 @@ m4_define([m4_require], [m4_if(_m4_divert_dump, [], [m4_fatal([$0($1): cannot be used outside of an ]dnl m4_if([$0], [m4_require], [[m4_defun]], [[AC_DEFUN]])['d macro])])]dnl -[m4_provide_if([$1], [], - [_m4_require_call([$1], [$2], _m4_divert_dump)])]) +[m4_provide_if([$1], [m4_ifdef([_m4_require], + [m4_set_contains([_m4_provide], [$1], + [m4_warn([syntax], [$0: `$1' was expanded before it was required])])])], + [_m4_require_call([$1], [$2], _m4_divert_dump)])]) # _m4_require_call(NAME-TO-CHECK, [BODY-TO-EXPAND = NAME-TO-CHECK], @@ -1949,12 +1999,13 @@ m4_if([$0], [m4_require], [[m4_defun]], [[AC_DEFUN]]) ['d macro])])]dnl # by avoiding dnl and other overhead on the common path. m4_define([_m4_require_call], [m4_pushdef([_m4_divert_grow], m4_decr(_m4_divert_grow))]dnl +[m4_pushdef([_m4_require])]dnl [m4_divert_push(_m4_divert_grow)]dnl [m4_if([$2], [], [$1], [$2]) -m4_provide_if([$1], [], [m4_warn([syntax], - [$1 is m4_require'd but not m4_defun'd])])]dnl +m4_provide_if([$1], [m4_set_remove([_m4_provide], [$1])], + [m4_warn([syntax], [$1 is m4_require'd but not m4_defun'd])])]dnl [_m4_divert_raw($3)_m4_undivert(_m4_divert_grow)]dnl -[m4_divert_pop(_m4_divert_grow)_m4_popdef([_m4_divert_grow])]) +[m4_divert_pop(_m4_divert_grow)_m4_popdef([_m4_divert_grow], [_m4_require])]) # _m4_divert_grow @@ -1976,7 +2027,8 @@ m4_define([m4_expand_once], # m4_provide(MACRO-NAME) # ---------------------- m4_define([m4_provide], -[m4_define([m4_provide($1)])]) +[m4_ifdef([m4_provide($1)], [], +[m4_set_add([_m4_provide], [$1], [m4_define([m4_provide($1)])])])]) # m4_provide_if(MACRO-NAME, IF-PROVIDED, IF-NOT-PROVIDED) diff --git a/tests/m4sugar.at b/tests/m4sugar.at index e822ffb..a6739cf 100644 --- a/tests/m4sugar.at +++ b/tests/m4sugar.at @@ -507,7 +507,7 @@ d post ]]) -dnl Direct invocation, top level +dnl Direct invocation, nested requires, top level AT_CHECK_M4SUGAR_TEXT([[dnl m4_defun([a], [[a]])dnl m4_defun([b], [[b]m4_require([a])])dnl @@ -528,10 +528,10 @@ c post ]]) -dnl Direct invocation, nested. This is an example of expansion before -dnl requirement, such that b occurs before its prerequisite a. This -dnl indicates a bug in the macros (but not in autoconf), so we should -dnl be emitting a warning. +dnl Direct invocation, nested requires, nested defun. This is an example +dnl of expansion before requirement, such that b occurs before its +dnl prerequisite a. This indicates a bug in the macros (but not in +dnl autoconf), so we should be emitting a warning. AT_CHECK_M4SUGAR_TEXT([[dnl m4_defun([a], [[a]])dnl m4_defun([b], [[b]m4_require([a])])dnl @@ -552,9 +552,50 @@ c a c post -]], [stderr]) -AT_XFAIL_IF([:]) -AT_CHECK([test -s stderr]) +]], +[[script.4s:14: warning: m4@&t...@_require: `a' was expanded before it was required +script.4s:5: b is expanded from... +script.4s:6: c is expanded from... +script.4s:7: outer is expanded from... +script.4s:14: the top level +]]) + +dnl Direct invocation, expand-before-require but no nested require. As this +dnl is common in real life, but does not result in out-of-order expansion, +dnl we silently permit this. +AT_CHECK_M4SUGAR_TEXT([[dnl +m4_defun([a], [[a]])dnl +m4_defun([b], [[b]m4_require([a])])dnl +m4_defun([c], [[c]])dnl +m4_defun([d], [[d]m4_require([c])])dnl +pre1 +a +b +a +b +post1 +m4_defun([outer], +[pre2 +c +d +c +d +post2])dnl +outer +]], +[[pre1 +a +b +a +b +post1 +pre2 +c +d +c +d +post2 +]]) AT_CLEANUP -- 1.6.0.4 >From 46009205a943a5f5b712cc11b72e9b017141ffeb Mon Sep 17 00:00:00 2001 From: Eric Blake <[email protected]> Date: Tue, 20 Jan 2009 14:22:41 -0700 Subject: [PATCH] Fix out-of-order expansion with expand-before-require. * lib/m4sugar/m4sugar.m4 (m4_require): Redundantly expand a required macro when issuing expand-before-require warning. * doc/autoconf.texi (Prerequisite Macros): Adjust documentation. (Expanded Before Required): New node. * tests/m4sugar.at (m4@&t...@_require: nested): Adjust test. * NEWS: Mention this fix. Suggested by Bruno Haible. Signed-off-by: Eric Blake <[email protected]> --- ChangeLog | 9 +++ NEWS | 9 +++ doc/autoconf.texi | 128 +++++++++++++++++++++++++++++++++++++----------- lib/m4sugar/m4sugar.m4 | 20 +++++--- tests/m4sugar.at | 3 +- 5 files changed, 131 insertions(+), 38 deletions(-) diff --git a/ChangeLog b/ChangeLog index f03804c..4ac83f5 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,14 @@ 2009-01-21 Eric Blake <[email protected]> + Fix out-of-order expansion with expand-before-require. + * lib/m4sugar/m4sugar.m4 (m4_require): Redundantly expand a + required macro when issuing expand-before-require warning. + * doc/autoconf.texi (Prerequisite Macros): Adjust documentation. + (Expanded Before Required): New node. + * tests/m4sugar.at (m4@&t...@_require: nested): Adjust test. + * NEWS: Mention this fix. + Suggested by Bruno Haible. + Warn if macro is provided before indirectly required. * lib/m4sugar/m4sugar.m4 (m4_provide): Track the set of all macros provided since last outermost defun. diff --git a/NEWS b/NEWS index 7eff140..5e43abc 100644 --- a/NEWS +++ b/NEWS @@ -5,6 +5,15 @@ GNU Autoconf NEWS - User visible changes. ** The manual is now shipped under the terms of the GNU FDL 1.3. +** AC_REQUIRE now detects the case of an outer macro which first expands + then later indirectly requires the same inner macro. Previously, + this case led to silent out-of-order expansion (bug present since + 2.50); it now issues a syntax warning, and duplicates the expansion + of the inner macro to guarantee dependencies have been met. See + the manual for advice on how to refactor macros in order to avoid + the bug in earlier autoconf versions and avoid increased script + size in the current version. + ** AC_LANG_ERLANG works once again (regression introduced in 2.61a). ** AC_HEADER_ASSERT is fixed so that './configure --enable-assert' no diff --git a/doc/autoconf.texi b/doc/autoconf.texi index 69d5085..b7e63fc 100644 --- a/doc/autoconf.texi +++ b/doc/autoconf.texi @@ -617,6 +617,7 @@ Top * Defining Directories:: Passing @code{datadir} to program * Autom4te Cache:: What is it? Can I remove it? * Present But Cannot Be Compiled:: Compiler and Preprocessor Disagree +* Expanded Before Required:: Expanded Before Required History of Autoconf @@ -13119,61 +13120,87 @@ Prerequisite Macros @end example However, this implementation can lead to another class of problems. -Since dependencies are hoisted prior to the outermost @code{AC_DEFUN} -body, but only if they have not been previously seen, it is still -possible to encounter out-of-order expansion. Given this example: +Consider the case where an outer macro first expands, then indirectly +requires, an inner macro: @example -AC_DEFUN([A], [[in A]]) -AC_DEFUN([B], [AC_REQUIRE([A])[in B]]) -AC_DEFUN([C], [AC_REQUIRE([B])[in C]]) -AC_DEFUN([OUTER], [[in OUTER] +AC_DEFUN([TESTA], [[echo in A +if test -n "$SEEN_A" ; then echo duplicate ; fi +SEEN_A=:]]) +AC_DEFUN([TESTB], [AC_REQUIRE([TESTA])[echo in B +if test -z "$SEEN_A" ; then echo bug ; fi]]) +AC_DEFUN([TESTC], [AC_REQUIRE([TESTB])[echo in C]]) +AC_DEFUN([OUTER], [[echo in OUTER] A C]) OUTER @end example @noindent -observe that the expansion of @code{B} occurred prior to the expansion -of @code{OUTER}, but because the requirement for @code{B} was not -encountered until after @code{A} had already been directly expanded, -there was no reason to hoist @code{A}. Or, put another way, the fact -that @code{B} was hoisted but not @code{A} means that @code{B} occurs -before its prerequisite: +Prior to Autoconf 2.64, the implementation of @code{AC_REQUIRE} +recognized that @code{TESTB} needed to be hoisted prior to the expansion +of @code{OUTER}, but because @code{TESTA} had already been directly +expanded, it failed to hoist @code{TESTA}. Therefore, the expansion of +...@code{testb} occurs prior to its prerequisites, leading to the following +output: @example in B +bug in OUTER in A in C @end example +...@noindent +Newer Autoconf is smart enough to recognize this situation, and hoists +...@code{testa} even though it has already been expanded, but issues a +syntax warning in the process. This is because the hoisted expansion of +...@code{testa} defeats the purpose of using @code{AC_REQUIRE} to avoid +redundant code, and causes its own set of problems if the hoisted macro +is not idempotent: + +...@example +in A +in B +in OUTER +in A +duplicate +in C +...@end example + The bug is not in Autoconf, but in the macro definitions. If you ever pass a particular macro name to @code{AC_REQUIRE}, then you are implying that the macro only needs to be expanded once. But to enforce this, all uses of that macro should be through @code{AC_REQUIRE}; directly expanding the macro defeats the point of using @code{AC_REQUIRE} to eliminate redundant expansion. In the example, this rule of thumb was -violated because @code{B} requires @code{A} while @code{OUTER} directly -expands it. One way of fixing the bug is to factor @code{A} into two -macros, the portion designed for direct and repeated use (here, named -...@code{a}), and the portion designed for one-shot output and used only -inside @code{AC_REQUIRE} (here, named @code{A_PREREQ}). Then, by fixing -all clients to use the correct macro according to their needs: - -...@example -AC_DEFUN([A], [AC_REQUIRE([A_PREREQ])[in A]]) -AC_DEFUN([A_PREREQ], [[in A_PREREQ]]) -AC_DEFUN([B], [AC_REQUIRE([A_PREREQ])[in B]]) -AC_DEFUN([C], [AC_REQUIRE([B])[in C]]) -AC_DEFUN([OUTER], [[in OUTER] -A -C]) +violated because @code{TESTB} requires @code{TESTA} while @code{OUTER} +directly expands it. One way of fixing the bug is to factor +...@code{testa} into two macros, the portion designed for direct and +repeated use (here, named @code{TESTA}), and the portion designed for +one-shot output and used only inside @code{AC_REQUIRE} (here, named +...@code{testa_prereq}). Then, by fixing all clients to use the correct +calling convention according to their needs: + +...@example +AC_DEFUN([TESTA], [AC_REQUIRE([TESTA_PREREQ])[echo in A]]) +AC_DEFUN([TESTA_PREREQ], [[echo in A_PREREQ +if test -n "$SEEN_A" ; then echo duplicate ; fi +SEEN_A=:]]) +AC_DEFUN([TESTB], [AC_REQUIRE([TESTA_PREREQ])[echo in B +if test -z "$SEEN_A" ; then echo bug ; fi]]) +AC_DEFUN([TESTC], [AC_REQUIRE([TESTB])[echo in C]]) +AC_DEFUN([OUTER], [[echo in OUTER] +TESTA +TESTC]) OUTER @end example @noindent -the resulting output will then obey all dependency rules: +the resulting output will then obey all dependency rules and avoid any +syntax warnings, whether the script is built with old or new Autoconf +versions: @example in A_PREREQ @@ -22415,6 +22442,7 @@ FAQ * Defining Directories:: Passing @code{datadir} to program * Autom4te Cache:: What is it? Can I remove it? * Present But Cannot Be Compiled:: Compiler and Preprocessor Disagree +* Expanded Before Required:: Expanded Before Required @end menu @node Distributing @@ -22803,6 +22831,48 @@ Present But Cannot Be Compiled See @ref{Particular Headers}, for a list of headers with their prerequisites. +...@node Expanded Before Required +...@section Expanded Before Required + +...@cindex expanded before required +Older versions of Autoconf silently built files with incorrect ordering +between dependent macros if an outer macro first expanded, then later +indirectly required, an inner macro. Starting with Autoconf 2.64, this +situation no longer generates out-of-order code, but results in +duplicate output and a diagnosis of a syntax warning: + +...@example +$ @kbd{cat configure.ac} +...@result{}ac_defun([TESTA], [[echo in A +...@result{}if test -n "$SEEN_A" ; then echo duplicate ; fi +...@result{}seen_a=:]]) +...@result{}ac_defun([TESTB], [AC_REQUIRE([TESTA])[echo in B +...@result{}if test -z "$SEEN_A" ; then echo bug ; fi]]) +...@result{}ac_defun([TESTC], [AC_REQUIRE([TESTB])[echo in C]]) +...@result{}ac_defun([OUTER], [[echo in OUTER] +...@result{}testa +...@result{}testc]) +...@result{}ac_init +...@result{}outer +...@result{}ac_output +$ @kbd{autoconf} +...@result{}configure.ac:11: warning: AC_REQUIRE: +...@result{} `TESTA' was expanded before it was required +...@result{}configure.ac:4: TESTB is expanded from... +...@result{}configure.ac:6: TESTC is expanded from... +...@result{}configure.ac:7: OUTER is expanded from... +...@result{}configure.ac:11: the top level +...@end example + +To avoid this warning, decide what purpose the macro in question serves. +If it only needs to be expanded once (for example, if it provides +initialization text used by later macros), then the fix is to change all +instance of direct calls to instead go through @code{AC_REQUIRE} +(@pxref{Prerequisite Macros}). If, instead, the macro is parameterized +by arguments or by the current definition of other macros in the m4 +environment, then the macro should always be directly expanded instead +of required. + @c ===================================================== History of Autoconf. @node History diff --git a/lib/m4sugar/m4sugar.m4 b/lib/m4sugar/m4sugar.m4 index 1457aef..3a5ce49 100644 --- a/lib/m4sugar/m4sugar.m4 +++ b/lib/m4sugar/m4sugar.m4 @@ -1728,8 +1728,8 @@ m4_define([m4_undivert], # inside a context that will be hoisted in front of the outermost # defun'd macro. In other words, we must be careful not to warn on: # -# | m4_defun([TEST1], [1]) -# | m4_defun([TEST2], [TEST1 REQUIRE([TEST1])]) +# | m4_defun([TEST4], [4]) +# | m4_defun([TEST5], [TEST4 REQUIRE([TEST5])]) # # The implementation of the warning involves tracking the set of # macros which have been provided since the start of the outermost @@ -1739,9 +1739,12 @@ m4_define([m4_undivert], # removed from the set; and when a macro is indirectly required (that # is, when m4_require detects a nested call), the set is checked. If # a macro is in the set, then it has been provided before it was -# required, and a warning is issued because the output will be out of -# order and the user must fix her macros to reflect the true -# dependencies. +# required, and we satisfy dependencies by expanding the macro as if +# it had never been provided; in the example given above, this means +# we now output `1 2 3 1'. Meanwhile, a warning is issued to inform +# the user that her macros trigger the bug in older autoconf versions, +# and that her outupt file now contains redundant contents (and +# possibly new problems, if the repeated macro was not idempotent). # # # 2. Keeping track of the expansion stack @@ -1984,9 +1987,10 @@ m4_define([m4_require], [m4_fatal([$0($1): cannot be used outside of an ]dnl m4_if([$0], [m4_require], [[m4_defun]], [[AC_DEFUN]])['d macro])])]dnl [m4_provide_if([$1], [m4_ifdef([_m4_require], - [m4_set_contains([_m4_provide], [$1], - [m4_warn([syntax], [$0: `$1' was expanded before it was required])])])], - [_m4_require_call([$1], [$2], _m4_divert_dump)])]) + [m4_set_contains([_m4_provide], [$1], [m4_warn([syntax], + [$0: `$1' was expanded before it was required])_m4_require_call], + [m4_ignore])], [m4_ignore])], + [_m4_require_call])([$1], [$2], _m4_divert_dump)]) # _m4_require_call(NAME-TO-CHECK, [BODY-TO-EXPAND = NAME-TO-CHECK], diff --git a/tests/m4sugar.at b/tests/m4sugar.at index a6739cf..613a3c9 100644 --- a/tests/m4sugar.at +++ b/tests/m4sugar.at @@ -545,7 +545,8 @@ c post])dnl outer ]], -[[b +[[a +b pre a c -- 1.6.0.4
