Re: fix frozen diversions on master branch

[Eric Blake]

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

> 
> Jim's recent complaint about not detecting numeric overflow made me realize 
> that the master branch has been broken for more than a year when it comes to 
> freezing a diversion that contains a \.  Fixed as follows.

An even bigger regression (and this time, it's not mine :).  Has anyone been 
seriously using the master branch with frozen files in the last 7 years?  
Fortunately for autoconf, I didn't see any pushdef'd definitions in 
autoconf.m4f, generated under 1.4.11, which does not have this bug.  The bug 
was introduced here:

http://git.savannah.gnu.org/gitweb/?p=m4.git;a=commitdiff;h=95e22#patch17

When Gary reorganized the symbol table to consist of a single key pointing to a 
stack of values, rather than a series of identical keys with a single value 
each and in a particular order, this caused -F to freeze only the top 
definition, rather than the entire stack of definitions, for a pushdef'd macro.

>From 91ba13aceb917fe33fe652fcd3a407d220fe297d Mon Sep 17 00:00:00 2001
From: Eric Blake <address@hidden>
Date: Thu, 15 May 2008 10:59:53 -0600
Subject: [PATCH] Fix frozen file regression in pushdef stacks from 2001-09-01.

* src/freeze.c (dump_symbol_CB): Push all values on the stack, not
just the current definition.
(reverse_symbol_value_stack): New helper method.
* tests/freeze.at (AT_TEST_FREEZE): New helper macro.
(reloading pushdef stack): New test.
(reloading unknown builtin): Enhance test.

Signed-off-by: Eric Blake <address@hidden>
---
 ChangeLog       |   10 ++++
 src/freeze.c    |  114 +++++++++++++++++++++++++---------------
 tests/freeze.at |  159 ++++++++++++++++++++-----------------------------------
 3 files changed, 140 insertions(+), 143 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 1c2a85a..b7bd8f3 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,13 @@
+2008-05-15  Eric Blake  <address@hidden>
+
+       Fix frozen file regression in pushdef stacks from 2001-09-01.
+       * src/freeze.c (dump_symbol_CB): Push all values on the stack, not
+       just the current definition.
+       (reverse_symbol_value_stack): New helper method.
+       * tests/freeze.at (AT_TEST_FREEZE): New helper macro.
+       (reloading pushdef stack): New test.
+       (reloading unknown builtin): Enhance test.
+
 2008-05-13  Eric Blake  <address@hidden>
 
        Fix frozen file regression in diversions from 2007-01-21.
diff --git a/src/freeze.c b/src/freeze.c
index 0b48ac6..ac67d56 100644
--- a/src/freeze.c
+++ b/src/freeze.c
@@ -131,55 +131,85 @@ produce_symbol_dump (m4 *context, FILE *file, 
m4_symbol_table *symtab)
     assert (false);
 }
 
+/* Given a stack of symbol values starting with VALUE, destructively
+   reverse the stack and return the pointer to what was previously the
+   last value in the stack.  VALUE may be NULL.  The symbol table that
+   owns the value stack should not be modified or consulted until this
+   is called again to undo the effect.  */
+static m4_symbol_value *
+reverse_symbol_value_stack (m4_symbol_value *value)
+{
+  m4_symbol_value *result = NULL;
+  m4_symbol_value *next;
+  while (value)
+    {
+      next = VALUE_NEXT (value);
+      VALUE_NEXT (value) = result;
+      result = value;
+      value = next;
+    }
+  return result;
+}
+
+/* Dump the stack of values for SYMBOL, with name SYMBOL_NAME, located
+   in SYMTAB.  USERDATA is interpreted as the FILE* to dump to.  */
 static void *
 dump_symbol_CB (m4_symbol_table *symtab, const char *symbol_name,
                m4_symbol *symbol, void *userdata)
 {
-  m4_module *   module         = SYMBOL_MODULE (symbol);
-  const char   *module_name    = module ? m4_get_module_name (module) : NULL;
-  FILE *       file            = (FILE *) userdata;
-  size_t       symbol_len      = strlen (symbol_name);
-  size_t       module_len      = module_name ? strlen (module_name) : 0;
+  FILE *file = (FILE *) userdata;
+  size_t symbol_len = strlen (symbol_name);
+  m4_symbol_value *value;
+  m4_symbol_value *last;
 
-  if (m4_is_symbol_text (symbol))
+  last = value = reverse_symbol_value_stack (m4_get_symbol_value (symbol));
+  while (value)
     {
-      const char *text = m4_get_symbol_text (symbol);
-      size_t text_len = m4_get_symbol_len (symbol);
-      xfprintf (file, "T%zu,%zu", symbol_len, text_len);
-      if (module)
-       xfprintf (file, ",%zu", module_len);
-      fputc ('\n', file);
+      m4_module *module = VALUE_MODULE (value);
+      const char *module_name = module ? m4_get_module_name (module) : NULL;
+      size_t module_len = module_name ? strlen (module_name) : 0;
 
-      produce_mem_dump (file, symbol_name, symbol_len);
-      produce_mem_dump (file, text, text_len);
-      if (module)
-       produce_mem_dump (file, module_name, module_len);
-      fputc ('\n', file);
-    }
-  else if (m4_is_symbol_func (symbol))
-    {
-      const m4_builtin *bp = m4_get_symbol_builtin (symbol);
-      size_t bp_len;
-      if (bp == NULL)
-       assert (!"INTERNAL ERROR: builtin not found in builtin table!");
-      bp_len = strlen (bp->name);
-
-      xfprintf (file, "F%zu,%zu", symbol_len, bp_len);
-      if (module)
-       xfprintf (file, ",%zu", module_len);
-      fputc ('\n', file);
-
-      produce_mem_dump (file, symbol_name, symbol_len);
-      produce_mem_dump (file, bp->name, bp_len);
-      if (module)
-       produce_mem_dump (file, module_name, module_len);
-      fputc ('\n', file);
+      if (m4_is_symbol_value_text (value))
+       {
+         const char *text = m4_get_symbol_value_text (value);
+         size_t text_len = m4_get_symbol_value_len (value);
+         xfprintf (file, "T%zu,%zu", symbol_len, text_len);
+         if (module)
+           xfprintf (file, ",%zu", module_len);
+         fputc ('\n', file);
+
+         produce_mem_dump (file, symbol_name, symbol_len);
+         produce_mem_dump (file, text, text_len);
+         if (module)
+           produce_mem_dump (file, module_name, module_len);
+         fputc ('\n', file);
+       }
+      else if (m4_is_symbol_value_func (value))
+       {
+         const m4_builtin *bp = m4_get_symbol_value_builtin (value);
+         size_t bp_len;
+         if (bp == NULL)
+           assert (!"INTERNAL ERROR: builtin not found in builtin table!");
+         bp_len = strlen (bp->name);
+
+         xfprintf (file, "F%zu,%zu", symbol_len, bp_len);
+         if (module)
+           xfprintf (file, ",%zu", module_len);
+         fputc ('\n', file);
+
+         produce_mem_dump (file, symbol_name, symbol_len);
+         produce_mem_dump (file, bp->name, bp_len);
+         if (module)
+           produce_mem_dump (file, module_name, module_len);
+         fputc ('\n', file);
+       }
+      else if (m4_is_symbol_value_placeholder (value))
+       ; /* Nothing to do for a builtin we couldn't reload earlier.  */
+      else
+       assert (!"dump_symbol_CB");
+      value = VALUE_NEXT (value);
     }
-  else if (m4_is_symbol_placeholder (symbol))
-    ; /* Nothing to do for a builtin we couldn't reload earlier.  */
-  else
-    assert (!"INTERNAL ERROR: bad token data type in produce_symbol_dump ()");
-
+  reverse_symbol_value_stack (last);
   return NULL;
 }
 
diff --git a/tests/freeze.at b/tests/freeze.at
index 74dc3e9..e8ada67 100644
--- a/tests/freeze.at
+++ b/tests/freeze.at
@@ -16,44 +16,54 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
-
-AT_BANNER([Freezing state.])
-
-## --------------- ##
-## large diversion ##
-## --------------- ##
-
-AT_SETUP([large diversion])
+# AT_TEST_FREEZE([title], [text1], [text2])
+# -----------------------------------------
+# Create a test TITLE, which checks that freezing TEXT1, then reloading
+# it with TEXT2, produces the same results as running TEXT1 and TEXT2 in
+# a single run.
+m4_define([AT_TEST_FREEZE],
+[AT_SETUP([$1])
 AT_KEYWORDS([frozen])
 
-# Check that large diversions are handled across freeze boundaries.
-# Also check for escape character handling.
-AT_DATA([[frozen.m4]], [M4_ONE_MEG_DEFN[divert(2)f
-divert(1)hi
-a\nb
-]])
-
-AT_DATA([[unfrozen.m4]],
-[[divert(3)bye
-]])
+AT_DATA([frozen.m4], [$2])
+AT_DATA([unfrozen.m4], [$3])
 
 # First generate the `expout' output by running over the sources before
 # freezing.
-AT_CHECK_M4([frozen.m4 unfrozen.m4], [0],
-           [stdout], [stderr])
+AT_CHECK_M4([frozen.m4 unfrozen.m4], [0], [stdout], [stderr])
 
 mv stdout expout
 mv stderr experr
 
 # Now freeze the first source file.
-AT_CHECK_M4([-F frozen.m4f frozen.m4], [0])
+AT_CHECK_M4([-F frozen.m4f frozen.m4], [0], [stdout])
+
+mv stdout out1
 
 # Now rerun the original sequence, but using the frozen file.
-AT_CHECK_M4([-R frozen.m4f unfrozen.m4], [0],
-           [expout], [experr])
+AT_CHECK_M4([-R frozen.m4f unfrozen.m4], [0], [stdout], [experr])
+
+AT_CHECK([cat out1 stdout], [0], [expout])
 
 AT_CLEANUP
+])
+
+
+AT_BANNER([Freezing state.])
+
+## --------------- ##
+## large diversion ##
+## --------------- ##
 
+# Check that large diversions are handled across freeze boundaries.
+# Also check for escape character handling.
+AT_TEST_FREEZE([large diversion],
+[M4_ONE_MEG_DEFN[divert(2)f
+divert(1)hi
+a\nb
+]],
+[[divert(3)bye
+]])
 
 ## ---------------- ##
 ## loading format 1 ##
@@ -271,110 +281,53 @@ AT_CLEANUP
 ## changecom ##
 ## --------- ##
 
-AT_SETUP([reloading changecom])
-AT_KEYWORDS([frozen])
-
-# Check that changesyntax is maintained across freeze boundaries.
-
-AT_DATA([[frozen.m4]],
+# Check that changecom/changequote are maintained across freeze boundaries.
+AT_TEST_FREEZE([reloading changecom],
 [[changecom`'changequote(<,>)dnl
-]])
-
-AT_DATA([[unfrozen.m4]],
+]],
 [[define(<foo>, <bar>)
 foo # foo
 ]])
 
-# First generate the `expout' output by running over the sources before
-# freezing.
-AT_CHECK_M4([frozen.m4 unfrozen.m4], [0],
-           [stdout], [stderr])
-
-mv stdout expout
-mv stderr experr
-
-# Now freeze the first source file.
-AT_CHECK_M4([-F frozen.m4f frozen.m4], [0])
-
-# Now rerun the original sequence, but using the frozen file.
-AT_CHECK_M4([-R frozen.m4f unfrozen.m4], [0],
-           [expout], [experr])
-
-AT_CLEANUP
-
-
 ## ------------ ##
 ## changesyntax ##
 ## ------------ ##
 
-AT_SETUP([reloading changesyntax])
-AT_KEYWORDS([frozen])
-
 # Check that changesyntax is maintained across freeze boundaries.
-
-AT_DATA([[frozen.m4]],
+AT_TEST_FREEZE([reloading changesyntax],
 [[changesyntax(`W+.', `({', `)}')dnl
 define{`a.b', `hello $1'}dnl
-]])
-
-AT_DATA([[unfrozen.m4]],
+]],
 [[a.b{world}
 ]])
 
-# First generate the `expout' output by running over the sources before
-# freezing.
-AT_CHECK_M4([frozen.m4 unfrozen.m4], [0],
-           [stdout], [stderr])
-
-mv stdout expout
-mv stderr experr
-
-# Now freeze the first source file.
-AT_CHECK_M4([-F frozen.m4f frozen.m4], [0])
-
-# Now rerun the original sequence, but using the frozen file.
-AT_CHECK_M4([-R frozen.m4f unfrozen.m4], [0],
-           [expout], [experr])
-
-AT_CLEANUP
-
-
 ## ------------- ##
 ## regexp syntax ##
 ## ------------- ##
 
-AT_SETUP([reloading regexp syntax])
-AT_KEYWORDS([frozen])
-
 # Check that regular expression syntax is maintained across freeze boundaries.
-
-AT_DATA([[frozen.m4]],
+AT_TEST_FREEZE([reloading regexp syntax],
 [[changeresyntax(`POSIX_EXTENDED')dnl
-]])
-
-AT_DATA([[unfrozen.m4]],
+]],
 [[regexp(`GNUs not Unix', `\w(\w*)$')
 regexp(`GNUs not Unix', `\w\(\w*\)$', `GNU_M4')
 ]])
 
-# First generate the `expout' output by running over the sources before
-# freezing.
-AT_CHECK_M4([frozen.m4 unfrozen.m4], [0],
-           [stdout], [stderr])
-
-mv stdout expout
-mv stderr experr
-
-# Now freeze the first source file.
-AT_CHECK_M4([-F frozen.m4f frozen.m4], [0],
-           [ignore], [ignore])
-
-# Now rerun the original sequence, but using the frozen file.
-AT_CHECK_M4([-R frozen.m4f unfrozen.m4], [0],
-           [expout], [experr])
-
-AT_CLEANUP
+## ------- ##
+## pushdef ##
+## ------- ##
 
+# Check for pushdef stacks; broken 2001-09-01, fixed 2008-05-15.
+AT_TEST_FREEZE([reloading pushdef stack],
+[[pushdef(`foo', `1')
+pushdef(`foo', defn(`len'))
+pushdef(`foo', `3')
+]],
+[[foo(`abc')popdef(`foo')
+foo(`ab')popdef(`foo')
+foo(`a')popdef(`foo')
+foo
+]])
 
 ## ---------------- ##
 ## unknown builtins ##
@@ -401,6 +354,8 @@ dnl Invoking the macro directly must warn
 a
 dnl Invoking it indirectly must warn
 indir(`a')
+dnl Since it is a placeholder, builtin must reject it
+builtin(`b')
 dnl The copy is a text string, not a placeholder
 c
 dnl Since it is defined, it must have a definition
@@ -418,11 +373,13 @@ AT_CHECK_M4([-R frozen.m4f input.m4], 0,
 
 
 
+
 a
 ]],
 [[m4:input.m4:4: Warning: a: builtin `b' requested by frozen file not found
 m4:input.m4:6: Warning: a: builtin `b' requested by frozen file not found
 m4:input.m4:8: Warning: a: builtin `b' requested by frozen file not found
+m4:input.m4:10: Warning: builtin: undefined builtin `b'
 a:     <<b>>
 c:     `'
 ]])
-- 
1.5.5.1