Re: Fix an 8-year-old AC_REQUIRE bug

[Eric Blake]

Eric Blake <ebb9 <at> byu.net> writes:

> Here's my first draft of a patch.  Unfortunately, there is probably existing 
> code that was expecting the old buggy behavior.

And here's my proposed patch that fixes those, by introducing a new macro that 
states when we want the old behavior (AS_IF, AS_CACHE_VAL, and maybe a small 
handful of other places) instead of the new.  As a bonus, wrapping text in 
AC_REQUIRE_HOIST issues a warning on code that was previously triggering the 
buggy AC_REQUIRE behaivior.


From: Eric Blake <address@hidden>
Date: Tue, 30 Dec 2008 17:18:42 -0700
Subject: [PATCH] Introduce m4_require_hoist/AC_REQUIRE_HOIST.

* lib/m4sugar/m4sugar.m4 (m4_require_hoist): New macro.
(m4_require, m4_provide): Use it to restore some measure of
backwards compatibility, when needed, but with safety checking.
* lib/autoconf/general.m4 (AC_REQUIRE_HOIST): New macro.
(AC_CACHE_VAL, AC_CACHE_CHECK): Perform prequisite hoisting.
* lib/m4sugar/m4sh.m4 (AS_CASE, AS_FOR, AS_IF): Likewise.
* doc/autoconf.texi (Prerequisite Macros) <AC_REQUIRE_HOIST>:
Document new macro.
* doc/autoconf.texi (Suggested Ordering) <AC_PROVIDE>: Document
existing macro.
* NEWS: Document this.

Signed-off-by: Eric Blake <address@hidden>
---
 ChangeLog               |   13 +++++++++
 NEWS                    |   11 +++++++-
 doc/autoconf.texi       |   70 +++++++++++++++++++++++++++++++++++++++++++++++
 lib/autoconf/general.m4 |   31 +++++++++++++-------
 lib/m4sugar/m4sh.m4     |   23 +++++++++++----
 lib/m4sugar/m4sugar.m4  |   25 +++++++++++++++--
 6 files changed, 152 insertions(+), 21 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index e5f3872..e114407 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,18 @@
 2008-12-30  Eric Blake  <address@hidden>
 
+       Introduce m4_require_hoist/AC_REQUIRE_HOIST.
+       * lib/m4sugar/m4sugar.m4 (m4_require_hoist): New macro.
+       (m4_require, m4_provide): Use it to restore some measure of
+       backwards compatibility, when needed, but with safety checking.
+       * lib/autoconf/general.m4 (AC_REQUIRE_HOIST): New macro.
+       (AC_CACHE_VAL, AC_CACHE_CHECK): Perform prequisite hoisting.
+       * lib/m4sugar/m4sh.m4 (AS_CASE, AS_FOR, AS_IF): Likewise.
+       * doc/autoconf.texi (Prerequisite Macros) <AC_REQUIRE_HOIST>:
+       Document new macro.
+       * doc/autoconf.texi (Suggested Ordering) <AC_PROVIDE>: Document
+       existing macro.
+       * NEWS: Document this.
+
        Fix output location of m4_defun->m4_defun->m4_require.
        This patch fails the testsuite, due to changed hoisting behavior.
        * lib/m4sugar/m4sugar.m4 (Defining macros): Update comments to
diff --git a/NEWS b/NEWS
index 99da48e..50cbd67 100644
--- a/NEWS
+++ b/NEWS
@@ -36,7 +36,10 @@ GNU Autoconf NEWS - User visible changes.
    This gives `A B PRE C POST' in older versions, and `PRE A B C POST'
    in this version; but since A and PRE don't have any dependency
    relation, both outputs should be equally correct (if they do have a
-   dependency relation, then you need an additional AC_REQUIRE).
+   dependency relation, then you should add AC_REQUIRE([a]) as part of
+   PRE, or wrap the body of outer with the new AC_REQUIRE_HOIST to get
+   the old behavior without the risk of depedencies issued out of
+   order).
 
 ** Autotest testsuites accept an option --jobs[=N] for parallel testing.
 
@@ -53,6 +56,12 @@ GNU Autoconf NEWS - User visible changes.
 ** Autoreconf added aclocal to the set of programs affected by the
    `autoreconf -I dir' option.
 
+** The following autoconf macros are documented now:
+   AC_PROVIDE
+
+** The following autoconf macros are new:
+   AC_REQUIRE_HOIST
+
 ** The following documented m4sugar macros are new:
    m4_chomp  m4_curry  m4_default_quoted  m4_esyscmd_s  m4_map_args
    m4_map_args_pair  m4_map_args_sep  m4_map_args_w  m4_set_map
diff --git a/doc/autoconf.texi b/doc/autoconf.texi
index d619578..dfc7242 100644
--- a/doc/autoconf.texi
+++ b/doc/autoconf.texi
@@ -13109,6 +13109,63 @@ Prerequisite Macros
 at the beginning of a macro.  You can use @code{dnl} to avoid the empty
 lines they leave.
 
address@hidden AC_REQUIRE_HOIST (@var{text})
address@hidden
+Expands @var{text} as normal, except that the current macro behaves as
+if it called @code{AC_REQUIRE} on the same set of macros required by any
+macros defined by @code{AC_DEFUN} and directly invoked (rather than
+required) during the expansion of @var{text}.  Issue a warning if,
+during the course of @var{text}, a macro is first expanded and later
+required, as that is a likely indicator that dependencies could not be
+honored and the output is out of order.
+
+This macro was implemented in Autoconf 2.64, as part of fixing a bug in
address@hidden  All prior versions behave as if this macro were
+always in effect, except that there is no warning when the
address@hidden bug outputs required macros in the wrong order.  To
+remove the warning, either avoid hoisting required macros, or add
address@hidden(address@hidden)} at a point earlier in @var{text} than
+where @var{macro} is directly expanded.
+
+There are very few places where this macro is actually needed.  One use
+is in the implementation of @code{AS_IF}, where it is desirable to hoist
+the prerequisite macro expansions to occur before the overall if
+statement, rather than hidden inside the branch of the conditional that
+contains the macro call that needed the prerequisite.
address@hidden defmac
+
+Here is an example of how @code{AC_REQUIRE_HOIST} affects output.  With
+Autoconf 2.64 and newer, @code{MACRO1} and @code{MACRO2} have different
+output order; with older Autoconf, the use of a preparation sequence
+allows both invocations to match @code{MACRO2}.
+
address@hidden
+dnl Use this sequence to be portable to earlier autoconf versions.
+m4_ifndef([AC_REQUIRE_HOIST],
+ [m4_define([AC_REQUIRE_HOIST], [$1])])dnl
+AC_DEFUN([A1], [[in A1]])dnl
+AC_DEFUN([B1], [[in B1]AC_REQUIRE([A1])])dnl
+AC_DEFUN([MACRO1], [PRE
+B1
+POST])dnl
+MACRO1
address@hidden
address@hidden
address@hidden
address@hidden
+
+AC_DEFUN([A2], [[in A2]])dnl
+AC_DEFUN([B2], [[in B2]AC_REQUIRE([A2])])dnl
+AC_DEFUN([MACRO2], [AC_REQUIRE_HOIST([PRE
+B2
+POST])])dnl
+MACRO2
address@hidden
address@hidden
address@hidden
address@hidden
address@hidden example
+
 @node Suggested Ordering
 @subsection Suggested Ordering
 @cindex Macros, ordering
@@ -13149,6 +13206,19 @@ Suggested Ordering
 that it has been called.
 @end defmac
 
address@hidden AC_PROVIDE (@var{this-macro-name})
address@hidden
+Declares that @var{this-macro-name} has been provided, which affects the
+behavior of @code{AC_REQUIRE} and @code{AC_BEFORE} when checking for
address@hidden  Normally, this is not needed, since
address@hidden automatically provides the current macro.  But sometimes
+it is desirable to have at most one out of a set of multiple
+initialization options executed; by giving the set a unique name, and
+making sure each initialization option provides that name, then it is
+possible to check that the single name has been provided rather than
+iterating through all the options.
address@hidden defmac
+
 @node One-Shot Macros
 @subsection One-Shot Macros
 @cindex One-shot macros
diff --git a/lib/autoconf/general.m4 b/lib/autoconf/general.m4
index bcf8720..65db81b 100644
--- a/lib/autoconf/general.m4
+++ b/lib/autoconf/general.m4
@@ -171,16 +171,18 @@ m4_copy([m4_divert_pop], [AC_DIVERT_POP])
 # AC_DEFUN(NAME, EXPANSION)
 # AC_DEFUN_ONCE(NAME, EXPANSION)
 # AC_BEFORE(THIS-MACRO-NAME, CALLED-MACRO-NAME)
-# AC_REQUIRE(STRING)
+# AC_REQUIRE(MACRO-NAME)
+# AC_REQUIRE_HOIST(TEXT)
 # AC_PROVIDE(MACRO-NAME)
 # AC_PROVIDE_IFELSE(MACRO-NAME, IF-PROVIDED, IF-NOT-PROVIDED)
 # -----------------------------------------------------------
-m4_copy([m4_defun],       [AC_DEFUN])
-m4_copy([m4_defun_once],  [AC_DEFUN_ONCE])
-m4_copy([m4_before],      [AC_BEFORE])
-m4_copy([m4_require],     [AC_REQUIRE])
-m4_copy([m4_provide],     [AC_PROVIDE])
-m4_copy([m4_provide_if],  [AC_PROVIDE_IFELSE])
+m4_copy([m4_defun],        [AC_DEFUN])
+m4_copy([m4_defun_once],   [AC_DEFUN_ONCE])
+m4_copy([m4_before],       [AC_BEFORE])
+m4_copy([m4_require],      [AC_REQUIRE])
+m4_copy([m4_require_hoist],[AC_REQUIRE_HOIST])
+m4_copy([m4_provide],      [AC_PROVIDE])
+m4_copy([m4_provide_if],   [AC_PROVIDE_IFELSE])
 
 
 # AC_OBSOLETE(THIS-MACRO-NAME, [SUGGESTION])
@@ -1991,8 +1993,11 @@ rm -f confcache[]dnl
 # AC_CACHE_VAL(CACHE-ID, COMMANDS-TO-SET-IT)
 # ------------------------------------------
 # The name of shell var CACHE-ID must contain `_cv_' in order to get saved.
-# Should be dnl'ed.  Try to catch common mistakes.
+# Should be dnl'ed.  Try to catch common mistakes.  Additionally, hoist
+# any AC_REQUIRE to occur before the cache check, and issue an error if
+# this is not possible due to a missing AC_REQUIRE.
 m4_defun([AC_CACHE_VAL],
+[m4_require_hoist(
 [AS_LITERAL_IF([$1], [m4_if(m4_index(m4_quote($1), [_cv_]), [-1],
                            [AC_DIAGNOSE([syntax],
 [$0($1, ...): suspicious cache-id, must contain _cv_ to be cached])])])dnl
@@ -2007,20 +2012,24 @@ m4_if(m4_index([$2], [AC_SUBST]), [-1], [],
 AS_VAR_SET_IF([$1],
              [_AS_ECHO_N([(cached) ])],
              [$2])
-])
+])])
 
 
 # AC_CACHE_CHECK(MESSAGE, CACHE-ID, COMMANDS)
 # -------------------------------------------
+# Wrap a cache check with MESSAGE and the result of the check.
+# Additionally, hoist any AC_REQUIRE to occur before this macro, or issue
+# an error if it is not possible.
+#
 # Do not call this macro with a dnl right behind.
 m4_defun([AC_CACHE_CHECK],
-[AC_MSG_CHECKING([$1])
+[m4_require_hoist([AC_MSG_CHECKING([$1])
 AC_CACHE_VAL([$2], [$3])dnl
 AS_LITERAL_IF([$2],
              [AC_MSG_RESULT([$$2])],
              [AS_VAR_COPY([ac_res], [$2])
               AC_MSG_RESULT([$ac_res])])dnl
-])
+])])
 
 # _AC_CACHE_CHECK_INT(MESSAGE, CACHE-ID, EXPRESSION,
 #                     [PROLOGUE = DEFAULT-INCLUDES], [IF-FAILS])
diff --git a/lib/m4sugar/m4sh.m4 b/lib/m4sugar/m4sh.m4
index 358aa81..b042fb3 100644
--- a/lib/m4sugar/m4sh.m4
+++ b/lib/m4sugar/m4sh.m4
@@ -523,6 +523,10 @@ _AS_UNSET_PREPARE
 # realize the impacts of using insufficient m4 quoting.  This macro
 # always provides a default case, to work around a Solaris /bin/sh
 # bug regarding the exit status when no case matches.
+#
+# Additionally, hoist any required macros occuring in the expansion of
+# the arguments to occur prior to the AS_IF expansion, or issue an
+# error alerting the user of a missing AC_REQUIRE.
 m4_define([_AS_CASE],
 [ address@hidden:@(]
   $1[)] m4_default([$2], [:]) ;;])
@@ -531,9 +535,9 @@ m4_define([_AS_CASE_DEFAULT],
   *[)] m4_default([$1], [:]) ;;])
 
 m4_defun([AS_CASE],
-[case $1 in[]m4_map_args_pair([_$0], [_$0_DEFAULT],
+[m4_require_hoist([case $1 in[]m4_map_args_pair([_$0], [_$0_DEFAULT],
    m4_shift(address@hidden(m4_eval([$# & 1]), [1], [,])))
-esac])# AS_CASE
+esac])])# AS_CASE
 
 
 # _AS_EXIT_PREPARE
@@ -581,6 +585,10 @@ m4_defun([AS_EXIT],
 # that element, thus providing a direct value rather than a shell
 # variable indirection.
 #
+# Additionally, hoist any required macros occuring in the expansion of
+# the arguments to occur prior to the AS_IF expansion, or issue an
+# error alerting the user of a missing AC_REQUIRE.
+#
 # Only use the optimization if LIST can be used without additional
 # shell quoting in either a literal or double-quoted context (that is,
 # we give up on default IFS chars, parameter expansion, command
@@ -589,10 +597,10 @@ m4_defun([AS_EXIT],
 m4_defun([AS_FOR],
 [m4_pushdef([$1], m4_if([$3], [], [[$$2]], m4_translit([$3], ]dnl
 m4_dquote(_m4_defn([m4_cr_symbols2]))[[%+=:,./-]), [], [[$3]], [[$$2]]))]dnl
-[for $2[]m4_ifval([$3], [ in $3])
+[m4_require_hoist([for $2[]m4_ifval([$3], [ in $3])
 do
   m4_default([$4], [:])
-done[]_m4_popdef([$1])])
+done[]_m4_popdef([$1])])])
 
 
 # AS_IF(TEST1, [IF-TRUE1 = :]...[IF-FALSE = :])
@@ -608,6 +616,9 @@ done[]_m4_popdef([$1])])
 # | fi
 # with simplifications if IF-TRUE1 and/or IF-FALSE is empty.
 #
+# Additionally, hoist any required macros occuring in the expansion of
+# the arguments to occur prior to the AS_IF expansion, or issue an
+# error alerting the user of a missing AC_REQUIRE.
 m4_define([_AS_IF],
 [elif $1; then
   m4_default([$2], [:])
@@ -618,10 +629,10 @@ m4_define([_AS_IF_ELSE],
   $1])])
 
 m4_defun([AS_IF],
-[if $1; then
+[m4_require_hoist([if $1; then
   m4_default([$2], [:])
 m4_map_args_pair([_$0], [_$0_ELSE], m4_shift2($@))]dnl
-[fi[]])# AS_IF
+[fi[]])])# AS_IF
 
 
 # AS_SET_STATUS(STATUS)
diff --git a/lib/m4sugar/m4sugar.m4 b/lib/m4sugar/m4sugar.m4
index 825001c..d30c4ca 100644
--- a/lib/m4sugar/m4sugar.m4
+++ b/lib/m4sugar/m4sugar.m4
@@ -2104,8 +2104,10 @@ m4_define([m4_require],
 [m4_ifdef([_m4_divert_grow], [],
          [m4_fatal([$0($1): cannot be used outside of an ]dnl
 m4_bmatch([$0], [^AC_], [[AC_DEFUN]], [[m4_defun]])['d macro])])]dnl
-[m4_provide_if([$1], [],
-              [_m4_require_call([$1], [$2], _m4_divert_grow)])])
+[m4_provide_if([$1], [m4_set_contains([_m4_hoist_provide], [$1],
+    [m4_warn([m4_require_hoist: $1 was provided before it was required])])],
+  [_m4_require_call([$1], [$2], m4_ifdef([_m4_hoisting], [_m4_hoisting],
+    [_m4_divert_grow]))m4_set_remove([_m4_hoist_provide], [$1])])])
 
 
 # _m4_require_call(NAME-TO-CHECK, [BODY-TO-EXPAND = NAME-TO-CHECK], DIVERSION)
@@ -2124,6 +2126,22 @@ m4_provide_if([$1], [], [m4_warn([syntax],
 [m4_divert_pop($3)])
 
 
+# m4_require_hoist(TEXT)
+# ----------------------
+# Expand TEXT, except that any m4_require done during TEXT is hoisted
+# to also be a requirement of the current defun'd macro.  In order to
+# prevent subtle bugs, an error is issued if a previously unprovided
+# macro name is first provided and then required within TEXT.
+m4_define([m4_require_hoist],
+[m4_ifdef([_m4_divert_grow], [],
+         [m4_fatal([$0: cannot be used outside of an ]dnl
+m4_if([$0], [m4_require_hoist], [[m4_defun]], [[AC_DEFUN]])['d macro])])]dnl
+[m4_pushdef([_m4_hoisting], m4_ifdef([_m4_hoisting], [_m4_hoisting],
+  [_m4_divert_grow]))$1[]]dnl
+[_m4_popdef([_m4_hoisting])m4_ifdef([_m4_hoisting], [],
+  [m4_set_delete([_m4_hoist_provide])])])
+
+
 # m4_expand_once(TEXT, [WITNESS = TEXT])
 # --------------------------------------
 # If TEXT has never been expanded, expand it *here*.  Use WITNESS as
@@ -2138,7 +2156,8 @@ m4_define([m4_expand_once],
 # ----------------------
 # Declare that MACRO-NAME has been provided.
 m4_define([m4_provide],
-[m4_define([m4_provide($1)])])
+[m4_provide_if([$1], [], [m4_ifdef([_m4_hoisting],
+  [m4_set_add([_m4_hoist_provide], [$1])])m4_define([m4_provide($1)])])])
 
 
 # m4_provide_if(MACRO-NAME, IF-PROVIDED, IF-NOT-PROVIDED)
-- 
1.6.0.4