Re: branch-1_4 off-by-one in line reporting

[Eric Blake]

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

> 
> Oops - for every line except line 1, if an unterminated quote or comment 
began 
> immediately after \n, the error message is off by one.

And that introduced a regression:

$ printf 'dnl ignored' > foo
$ m4
include(foo) still ignored
__file__
foo

The problem here is that dnl was updating global state if the newline is not in 
the same file as the macro invocation, but I was then undoing that update by 
restoring the location cached at the end of argument collection.

And in fixing the regression, I found a long-standing bug:

$ printf '__file__' > foo
$ m4
include(foo)
stdin

Basically, when the parse engine peeks to see if a macro name is followed by an 
open parenthesis, it must not update the global state.

Since users should be ending text files in newlines, neither of these bugs will 
hit in portable usage of m4.  But we might as well make the corner cases 
correct too!

And getting this correct made it trivial to support a long-time wish of mine - 
m4wrap should remember where the wrapped text originated from.

This is difficult to portably test without the help of changeword altering the 
syntax to allow a trailing newline to be part of the macro that causes the scan 
to continue into the next file, but I did add a test to 'make distcheck' so 
that we won't regress.

2006-10-11  Eric Blake  <address@hidden>

        * src/input.c (enum input_type): Add additional types, to shave
        time off the common case.
        (push_wrapup): Wrapped strings remember location.
        (push_string_finish): Normal strings carry no location.
        (push_file): Start new files uninitialized.
        (peek_input, next_char_1): Optimize common cases by updating
        location only on new input types.
        (pop_input): Update to honor new input types.
        (skip_line, push_string_finish): Fix regression in previous patch
        when dnl is not followed by newline in included file.
        (push_string_init): Initialize all fields of INPUT_STRING sooner.
        (peek_token): Simplify.
        (peek_input): Don't pop input files on peek, so that __file__ and
        __line__ as last token of include file work correctly.
        * doc/m4.texinfo (History): Mention 1.4.8.
        (Answers): Split into sections, one per answer.
        (Improved exch, Improved cleardivert, Improved fatal_error): New
        nodes.
        (Dnl, M4wrap, Location, M4exit, Improved fatal_error): Update to
        new m4wrap location semantics.
        (Changeword): Add test that caught the regression.
        * NEWS: Document this.

Index: NEWS
===================================================================
RCS file: /sources/m4/m4/NEWS,v
retrieving revision 1.1.1.1.2.69
diff -u -r1.1.1.1.2.69 NEWS
--- NEWS        11 Oct 2006 17:07:03 -0000      1.1.1.1.2.69
+++ NEWS        11 Oct 2006 22:57:39 -0000
@@ -15,7 +15,11 @@
   `errprint' macro, now always cause a non-zero exit status.
 * Warnings and errors issued during macro expansion are now consistently
   reported at the line where the macro name was detected, rather than
-  where the close parenthesis resides.
+  where the close parenthesis resides.  Text wrapped by `m4wrap' now
+  remembers the location that was in effect when m4wrap was invoked,
+  rather than changing to line 0 and the empty string for a file.  The
+  macros `__line__' and `__file__' now work correctly even as the last
+  token in an included file.
 
 Version 1.4.7 - 25 September 2006, by Eric Blake  (CVS version 1.4.6a)
 
Index: doc/m4.texinfo
===================================================================
RCS file: /sources/m4/m4/doc/m4.texinfo,v
retrieving revision 1.1.1.1.2.81
diff -u -r1.1.1.1.2.81 m4.texinfo
--- doc/m4.texinfo      11 Oct 2006 17:07:03 -0000      1.1.1.1.2.81
+++ doc/m4.texinfo      11 Oct 2006 22:57:39 -0000
@@ -248,6 +248,12 @@
 * Incompatibilities::           Facilities in System V m4 not in GNU M4
 * Other Incompatibilities::     Other incompatibilities
 
+Correct version of some examples
+
+* Improved exch::               Solution for @code{exch}
+* Improved cleardivert::        Solution for @code{cleardivert}
+* Improved fatal_error::        Solution for @code{fatal_error}
+
 How to make copies of this manual
 
 * GNU Free Documentation License::  License for copying this manual
@@ -360,7 +366,7 @@
 Then in 2005 Gary V. Vaughan collected together the many
 patches to @acronym{GNU} @code{m4} 1.4 that were floating around the net and
 released 1.4.3 and 1.4.4.  And in 2006, Eric Blake joined the team and
-prepared patches for the release of 1.4.5, 1.4.6, and 1.4.7.
+prepared patches for the release of 1.4.5, 1.4.6, 1.4.7, and 1.4.8.
 
 Meanwhile, development has continued on new features for @code{m4}, such
 as dynamic module loading and additional builtins.  When complete,
@@ -2464,13 +2470,13 @@
 warning is issued and dnl stops consuming input.
 
 @example
-define(`hi', `HI')
address@hidden
 m4wrap(`m4wrap(`2 hi
 ')0 hi dnl 1 hi')
 @result{}
+define(`hi', `HI')
address@hidden
 ^D
address@hidden: Warning: end of file treated as newline
address@hidden:stdin:1: Warning: end of file treated as newline
 @result{}0 HI 2 HI
 @end example
 
@@ -2879,6 +2885,41 @@
 @result{}bar
 @end example
 
address@hidden
address@hidden One more test of including newline in a macro name; but this
address@hidden does not need to be displayed in the manual.  This ensures
address@hidden that line numbering is correct when dnl cuts across include
address@hidden file boundaries, and when __file__ or __line__ is the last
address@hidden token in an include file.
+
address@hidden
+ifdef(`changeword', `', `errprint(` skipping: no changeword support
+')m4exit(`77')')dnl
+define(`bar
+', defn(`dnl'))dnl
+changeword(`\([_a-zA-Z][_a-zA-Z0-9]*\|bar
+\)')
address@hidden
+__file__:__line__
address@hidden:7
+include(`foo') still ignored
+__file__:__line__
address@hidden:9
+define(`bar
+', defn(`__file__'))
address@hidden
+include(`foo')
address@hidden/examples/foo
+define(`bar
+', defn(`__line__'))
address@hidden
+include(`foo')
address@hidden
+__file__:__line__
address@hidden:16
address@hidden example
address@hidden ignore
+
 @code{changeword} has another function.  If the regular expression
 supplied contains any grouped subexpressions, then text outside
 the first of these is discarded before symbol lookup.  So:
@@ -3032,7 +3073,7 @@
 m4wrap(`m4wrap(`)')len(abc')
 @result{}
 ^D
address@hidden: ERROR: end of file in argument list
address@hidden:stdin:1: ERROR: end of file in argument list
 @end example
 
 @node File Inclusion
@@ -4427,10 +4468,34 @@
 @result{}
 @end example
 
-Currently, all text wrapped with @code{m4wrap} (@pxref{M4wrap}) behaves
-as though it came from line 0 of the file ``''.  It is hoped that a
-future release of @code{m4} can overcome this limitation and remember
-which file invoked the call to @code{m4wrap}.
+The location of macros invoked during the rescanning of macro expansion
+text corresponds to the location in the file where the expansion was
+triggered, regardless of how many newline characters the expansion text
+contains.  As of @acronym{GNU} M4 1.4.8, the location of text wrapped
+with @code{m4wrap} (@pxref{M4wrap}) is the point at which the
address@hidden was invoked.  Previous versions, however, behaved as
+though wrapped text came from line 0 of the file ``''.
+
address@hidden
+define(`echo', `$@')
address@hidden
+define(`foo', `echo(__line__
+__line__)')
address@hidden
+echo(__line__
+__line__)
address@hidden
address@hidden
+m4wrap(`foo
+')
address@hidden
+foo
address@hidden
address@hidden
+^D
address@hidden
address@hidden
address@hidden example
 
 The @code{__program__} macro behaves like @samp{$0} in shell
 terminology.  If you invoke @code{m4} through an absolute path or a link
@@ -4440,7 +4505,8 @@
 formatting that @code{m4} produces internally.  It can also be used
 within @code{syscmd} (@pxref{Syscmd}) to pick the same version of
 @code{m4} that is currently running, rather than whatever version of
address@hidden happens to be first in @env{PATH}.
address@hidden happens to be first in @env{PATH}.  It was first introduced
+in @acronym{GNU} M4 1.4.6.
 
 @node M4exit
 @section Exiting from @code{m4}
@@ -4457,6 +4523,15 @@
 input is read, and all wrapped and diverted text is discarded.
 @end deffn
 
address@hidden
+m4wrap(`This text is lost due to `m4exit'.')
address@hidden
+divert(`1') So is this.
+divert
address@hidden
+m4exit And this is never read.
address@hidden example
+
 A common use of this is to abort processing:
 
 @deffn Composite fatal_error (@var{message})
@@ -4477,22 +4552,13 @@
 After this macro call, @code{m4} will exit with exit status 1.  This macro
 is only intended for error exits, since the normal exit procedures are
 not followed, e.g., diverted text is not undiverted, and saved text
-(@pxref{M4wrap}) is not reread.  (This macro has a subtle bug, when
-invoked from wrapped text.  You should try to see if you can find it and
-correct it.  @pxref{Answers})
-
address@hidden
-m4wrap(`This text is lost to `m4exit'.')
address@hidden
-divert(`1') And so is this.
-divert
address@hidden
-m4exit
address@hidden example
+(@pxref{M4wrap}) is not reread.  (This macro could be made more robust
+to earlier versions of @code{m4}.  You should try to see if you can find
+weaknesses and correct them.  @pxref{Answers})
 
 Note that it is still possible for the exit status to be different than
 what was requested by @code{m4exit}.  If @code{m4} detects some other
-error, such as a write error on standard out, the exit status will be
+error, such as a write error on standard output, the exit status will be
 non-zero even if @code{m4exit} requested zero.
 
 If standard input is seekable, then the file will be positioned at the
@@ -5017,8 +5083,18 @@
 @node Answers
 @chapter Correct version of some examples
 
-Some of the examples in this manuals are buggy, for demonstration
-purposes.  Correctly working macros are presented here.
+Some of the examples in this manuals are buggy or not very robust, for
+demonstration purposes.  Improved versions of these composite macros are
+presented here.
+
address@hidden
+* Improved exch::               Solution for @code{exch}
+* Improved cleardivert::        Solution for @code{cleardivert}
+* Improved fatal_error::        Solution for @code{fatal_error}
address@hidden menu
+
address@hidden Improved exch
address@hidden Solution for @code{exch}
 
 The @code{exch} macro (@pxref{Arguments}) as presented requires clients
 to double quote their arguments.  A nicer definition, which lets
@@ -5035,6 +5111,9 @@
 @result{}expansion text
 @end example
 
address@hidden Improved cleardivert
address@hidden Solution for @code{cleardivert}
+
 The @code{cleardivert} macro (@pxref{Cleardiv}) cannot, as it stands, be
 called without arguments to clear all pending diversions.  That is
 because using undivert with an empty string for an argument is different
@@ -5066,17 +5145,21 @@
 @result{}
 @end example
 
-The @code{fatal_error} macro (@pxref{M4exit}) does not quite match the
-format of internal error messages when invoked inside wrapped text, due
-to the current limitations of @code{__file__} (@pxref{Location}) when
-invoked inside @code{m4wrap}.  Since @code{m4} omits the file and line
-number from its warning messages when there is no current file (or
-equivalently, when the current line is 0, since all files start at line
-1), a better implementation would be:
address@hidden Improved fatal_error
address@hidden Solution for @code{fatal_error}
+
+The @code{fatal_error} macro (@pxref{M4exit}) is not robust to versions
+of @acronym{GNU} M4 earlier than 1.4.8, where invoking @code{__file__}
+(@pxref{Location}) inside @code{m4wrap} would result in an empty string,
+and @code{__line__} resulted in @samp{0} even though all files start at
+line 1.  Furthermore, versions earlier than 1.4.6 did not support the
address@hidden macro.  If you want @code{fatal_error} to work across
+the entire 1.4.x release series, a better implementation would be:
 
 @example
 define(`fatal_error',
-  `errprint(__program__:ifelse(__line__, `0', `',
+  `errprint(ifdef(`__program', `__program__', ``m4'')'dnl
+`:ifelse(__line__, `0', `',
     `__file__:__line__:')` fatal error: $*
 ')m4exit(`1')')
 @result{}
@@ -5084,9 +5167,9 @@
 fatal_error(`inside wrapped text')')
 @result{}
 ^D
address@hidden: Warning: excess arguments to builtin `divnum' ignored
address@hidden:stdin:6: Warning: excess arguments to builtin `divnum' ignored
 @result{}0
address@hidden: fatal error: inside wrapped text
address@hidden:stdin:6: fatal error: inside wrapped text
 @end example
 
 @c ========================================================== Appendices
Index: src/input.c
===================================================================
RCS file: /sources/m4/m4/src/Attic/input.c,v
retrieving revision 1.1.1.1.2.24
diff -u -r1.1.1.1.2.24 input.c
--- src/input.c 11 Oct 2006 17:07:03 -0000      1.1.1.1.2.24
+++ src/input.c 11 Oct 2006 22:57:39 -0000
@@ -61,9 +61,11 @@
 
 enum input_type
 {
-  INPUT_FILE,
-  INPUT_STRING,
-  INPUT_MACRO
+  INPUT_STRING,                /* String resulting from macro expansion.  */
+  INPUT_STRING_WRAP,   /* String resulting from m4wrap.  */
+  INPUT_FILE,          /* File which has had at least one character read.  */
+  INPUT_FILE_INIT,     /* File which has not yet been read.  */
+  INPUT_MACRO          /* Builtin resulting from defn.  */
 };
 
 typedef enum input_type input_type;
@@ -71,14 +73,16 @@
 struct input_block
 {
   struct input_block *prev;    /* previous input_block on the input stack */
-  input_type type;             /* INPUT_FILE, INPUT_STRING or INPUT_MACRO */
+  input_type type;             /* see enum values */
   union
     {
       struct
        {
          char *string;         /* string value */
+         const char *name;     /* file where wrapped text is from */
+         int lineno;           /* line where wrapped text is from */
        }
-      u_s;
+       u_s;    /* INPUT_STRING, INPUT_STRING_WRAP */
       struct
        {
          FILE *file;           /* input file handle */
@@ -89,7 +93,7 @@
          int out_lineno;       /* current output line of previous file */
          boolean advance_line; /* start_of_input_line from next_char () */
        }
-      u_f;
+       u_f;    /* INPUT_FILE, INPUT_FILE_INIT */
       builtin_func *func;      /* pointer to macro's function */
     }
   u;
@@ -184,7 +188,7 @@
 
   i = (input_block *) obstack_alloc (current_input,
                                     sizeof (struct input_block));
-  i->type = INPUT_FILE;
+  i->type = INPUT_FILE_INIT;
 
   i->u.u_f.end = FALSE;
   i->u.u_f.close = close;
@@ -196,10 +200,11 @@
      to expand_macro remembering where the include or sinclude builtin
      invocation began, so don't modify them here.  However, we are
      guaranteed that they are consistent in the context of read or
-     peek.  So we use u_f.lineno == 0 and a non-empty u_f.name as a
-     key that this file is newly pushed, and that current_file/line
-     still needs an update (lineno == 0 and an empty name imply that
-     when we pop this file, there is no more input to return to).
+     peek.  So we use the temporary type INPUT_FILE_INIT as key that
+     this file is newly pushed, u_f.name is the name of this file, and
+     that current_file/line still needs an update; and the
+     longer-lasting type INPUT_FILE as key that the file is in use,
+     with u_f.name as the name of the file that included this file.
      Also, we must save title on a separate obstack, again since
      expand_macro hangs on to file names even after the file is
      closed.  */
@@ -255,6 +260,8 @@
   next = (input_block *) obstack_alloc (current_input,
                                        sizeof (struct input_block));
   next->type = INPUT_STRING;
+  next->u.u_s.name = NULL;
+  next->u.u_s.lineno = 0;
   return current_input;
 }
 
@@ -275,7 +282,8 @@
   if (next == NULL)
     return NULL;
 
-  if (obstack_object_size (current_input) > 0)
+  if (obstack_object_size (current_input) > 0
+      || isp->type == INPUT_STRING_WRAP)
     {
       obstack_1grow (current_input, '\0');
       next->u.u_s.string = obstack_finish (current_input);
@@ -304,8 +312,10 @@
   i = (input_block *) obstack_alloc (wrapup_stack,
                                     sizeof (struct input_block));
   i->prev = wsp;
-  i->type = INPUT_STRING;
+  i->type = INPUT_STRING_WRAP;
   i->u.u_s.string = obstack_copy0 (wrapup_stack, s, strlen (s));
+  i->u.u_s.name = current_file;
+  i->u.u_s.lineno = current_line;
   wsp = i;
 }
 
@@ -323,6 +333,12 @@
 
   switch (isp->type)
     {
+    case INPUT_STRING_WRAP:
+    case INPUT_FILE_INIT:
+      M4ERROR ((warning_status, 0,
+               "INTERNAL ERROR: input stack botch in pop_input ()"));
+      abort ();
+
     case INPUT_STRING:
     case INPUT_MACRO:
       break;
@@ -446,38 +462,31 @@
 peek_input (void)
 {
   int ch;
+  input_block *block = isp;
 
   while (1)
     {
-      if (isp == NULL)
+      if (block == NULL)
        return CHAR_EOF;
 
-      switch (isp->type)
+      switch (block->type)
        {
+       case INPUT_STRING_WRAP:
        case INPUT_STRING:
-         ch = to_uchar (isp->u.u_s.string[0]);
+         ch = to_uchar (block->u.u_s.string[0]);
          if (ch != '\0')
            return ch;
          break;
 
+       case INPUT_FILE_INIT:
        case INPUT_FILE:
-         /* See comments in push_file.  */
-         if (isp->u.u_f.lineno == 0 && isp->u.u_f.name[0] != '\0')
-           {
-             const char *tmp = isp->u.u_f.name;
-             isp->u.u_f.name = current_file;
-             isp->u.u_f.lineno = current_line;
-             current_file = tmp;
-             current_line = 1;
-           }
-
-         ch = getc (isp->u.u_f.file);
+         ch = getc (block->u.u_f.file);
          if (ch != EOF)
            {
-             ungetc (ch, isp->u.u_f.file);
+             ungetc (ch, block->u.u_f.file);
              return ch;
            }
-         isp->u.u_f.end = TRUE;
+         block->u.u_f.end = TRUE;
          break;
 
        case INPUT_MACRO:
@@ -488,12 +497,7 @@
                    "INTERNAL ERROR: input stack botch in peek_input ()"));
          abort ();
        }
-      /* End of current input source --- pop one level if another
-        level still exists.  */
-      if (isp->prev != NULL)
-       pop_input ();
-      else
-       return CHAR_EOF;
+      block = block->prev;
     }
 }
 
@@ -508,8 +512,8 @@
 `-------------------------------------------------------------------------*/
 
 #define next_char() \
-  (isp && isp->type == INPUT_STRING && isp->u.u_s.string[0]            \
-   ? to_uchar (*isp->u.u_s.string++)                                   \
+  (isp && isp->type == INPUT_STRING && isp->u.u_s.string[0]    \
+   ? to_uchar (*isp->u.u_s.string++)                           \
    : next_char_1 ())
 
 static int
@@ -530,23 +534,29 @@
 
       switch (isp->type)
        {
+       case INPUT_STRING_WRAP:
+         current_file = isp->u.u_s.name;
+         current_line = isp->u.u_s.lineno;
+         isp->type = INPUT_STRING;
+         /* fall through */
        case INPUT_STRING:
          ch = to_uchar (*isp->u.u_s.string++);
          if (ch != '\0')
            return ch;
          break;
 
-       case INPUT_FILE:
+       case INPUT_FILE_INIT:
          /* See comments in push_file.  */
-         if (isp->u.u_f.lineno == 0 && isp->u.u_f.name[0] != '\0')
-           {
-             const char *tmp = isp->u.u_f.name;
-             isp->u.u_f.name = current_file;
-             isp->u.u_f.lineno = current_line;
-             current_file = tmp;
-             current_line = 1;
-           }
-
+         {
+           const char *tmp = isp->u.u_f.name;
+           isp->u.u_f.name = current_file;
+           isp->u.u_f.lineno = current_line;
+           current_file = tmp;
+           current_line = 1;
+         }
+         isp->type = INPUT_FILE;
+         /* fall through */
+       case INPUT_FILE:
          /* If stdin is a terminal, calling getc after peek_input
             already called it would make the user have to hit ^D
             twice to quit.  */
@@ -594,6 +604,25 @@
        previous value we stored earlier.  */
     M4ERROR_AT_LINE ((warning_status, 0, file, line,
                      "Warning: end of file treated as newline"));
+  /* On the rare occasion that dnl crosses include file boundaries
+     (either the input file did not end in a newline, or changeword
+     was used), calling next_char can update current_file and
+     current_line, and that update will be undone as we return to
+     expand_macro.  This hack of sticking an empty INPUT_STRING_WRAP
+     with the correct location as the next thing to parse works around
+     the problem.  */
+  if (file != current_file || line != current_line)
+    {
+      input_block *i;
+      i = (input_block *) obstack_alloc (current_input,
+                                        sizeof (struct input_block));
+      i->prev = isp;
+      i->type = INPUT_STRING_WRAP;
+      i->u.u_s.string = "";
+      i->u.u_s.name = current_file;
+      i->u.u_s.lineno = current_line + 1;  /* Account for parsed `\n'.  */
+      isp = i;
+    }
 }
 
 
@@ -828,7 +857,7 @@
   token_type type;
 #ifdef ENABLE_CHANGEWORD
   int startpos;
-  char *orig_text = 0;
+  char *orig_text = NULL;
 #endif
   const char *file;
   int line;
@@ -995,74 +1024,53 @@
 token_type
 peek_token (void)
 {
+  token_type result;
   int ch = peek_input ();
 
   if (ch == CHAR_EOF)
     {
-#ifdef DEBUG_INPUT
-      fprintf (stderr, "peek_token -> EOF\n");
-#endif
-      return TOKEN_EOF;
+      result = TOKEN_EOF;
     }
-  if (ch == CHAR_MACRO)
+  else if (ch == CHAR_MACRO)
     {
-#ifdef DEBUG_INPUT
-      fprintf (stderr, "peek_token -> MACDEF\n");
-#endif
-      return TOKEN_MACDEF;
+      result = TOKEN_MACDEF;
     }
-
-  if (MATCH (ch, bcomm.string, FALSE))
+  else if (MATCH (ch, bcomm.string, FALSE))
     {
-#ifdef DEBUG_INPUT
-      fprintf (stderr, "peek_token -> COMMENT\n");
-#endif
-      return TOKEN_STRING;
+      result = TOKEN_STRING;
     }
-
-  if ((default_word_regexp && (isalpha (ch) || ch == '_'))
+  else if ((default_word_regexp && (isalpha (ch) || ch == '_'))
 #ifdef ENABLE_CHANGEWORD
       || (! default_word_regexp && word_start[ch])
 #endif /* ENABLE_CHANGEWORD */
       )
     {
-#ifdef DEBUG_INPUT
-      fprintf (stderr, "peek_token -> WORD\n");
-#endif
-      return TOKEN_WORD;
+      result = TOKEN_WORD;
     }
-
-  if (MATCH (ch, lquote.string, FALSE))
+  else if (MATCH (ch, lquote.string, FALSE))
     {
-#ifdef DEBUG_INPUT
-      fprintf (stderr, "peek_token -> QUOTE\n");
-#endif
-      return TOKEN_STRING;
+      result = TOKEN_STRING;
     }
+  else
+    switch (ch)
+      {
+      case '(':
+       result = TOKEN_OPEN;
+       break;
+      case ',':
+       result = TOKEN_COMMA;
+       break;
+      case ')':
+       result = TOKEN_CLOSE;
+       break;
+      default:
+       result = TOKEN_SIMPLE;
+      }
 
-  switch (ch)
-    {
-    case '(':
-#ifdef DEBUG_INPUT
-      fprintf (stderr, "peek_token -> OPEN\n");
-#endif
-      return TOKEN_OPEN;
-    case ',':
 #ifdef DEBUG_INPUT
-      fprintf (stderr, "peek_token -> COMMA\n");
-#endif
-      return TOKEN_COMMA;
-    case ')':
-#ifdef DEBUG_INPUT
-      fprintf (stderr, "peek_token -> CLOSE\n");
-#endif
-      return TOKEN_CLOSE;
-    default:
-#ifdef DEBUG_INPUT
-      fprintf (stderr, "peek_token -> SIMPLE\n");
-#endif
-      return TOKEN_SIMPLE;
-    }
+  fprintf (stderr, "peek_token -> %s\n", token_type_string (result));
+#endif /* DEBUG_INPUT */
+  return result;
 }