This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
[patch, fortran] error.c: Cleanup, refactoring, column numbers in filename lines.
- From: Brooks Moses <brooks dot moses at codesourcery dot com>
- To: gcc-patches at gcc dot gnu dot org
- Cc: fortran at gcc dot gnu dot org
- Date: Sun, 05 Nov 2006 23:30:48 -0800
- Subject: [patch, fortran] error.c: Cleanup, refactoring, column numbers in filename lines.
(Why? Well, since I was looking at error.c already from the last patch,
I decided to read through it to get a better idea how it worked, and ...
well, it kind of get refactored a bit and grew some extra comments while
I was understanding it. And people seem to think the column numbers are
handy.)
The attached set of patches are all modifications to error.c,
implementing a sort of general refactoring of some of the error
processing in order to fix some minor bugs, and make the code notably
more legible and maintainable (at least IMHO). In addition, once I'd
done all this, implementing column numbers in the "[file]:[line]:" line
was fairly simple.
I've broken the overall changes up into a set of patches that can be
applied sequentially; I'll explain them in order:
------------------------------------------------------------------
error3.diff:
When an error occurs in a file that is included in another file,
GFortran prints out a "[file]:[line]:" line for the source file with the
error, and follows this with "Included in:" lines for the stack of
including files. Currently, there is a useless blank line before each
"Included in:" line. This patch removes that blank lines. It also adds
some comments to nearby code.
Interestingly, this particular error functionality does not show up
anywhere in the testsuite.
------------------------------------------------------------------
error4.diff:
When there are two error loci on the same line at the same column,
GFortran prints out '12' such that the '2' is one column to the right of
the locus, rather than only printing the '1'. This cleans up the code
that accomplishes this.
The cleanup, incidentally, means that this motion of the '2' can be
accounted for when the line is offset.
------------------------------------------------------------------
error5.diff:
When a line contains only a single locus, and the locus is too far to
the right to fit onscreen without offsetting the offending source-code
line, GFortran does some rather strange calculations to figure out how
to offset the line.
Specifically, it places the locus on the first line 5 columns from the
left margin, and the locus on the second line 20 columns from the left
margin. This is rather bizarre -- since the error is known to be near
the end of the line, it means that very little of the line is actually
shown.
This patch changes the offset calculation to one that makes much more
sense -- namely, setting the loci 5 and 20 columns, respectively, from
the _right_ margin.
In addition, it adds some more comments, including a TODO that I was
meaning to come back and address later.
------------------------------------------------------------------
error6.diff:
Now that the offset calculations are all nearly the same, there's no
point in putting them (and the printing of the column-marker lines) in
three nearly-identical bits of code in show_loci, when it conceptually
fits much better in show_locus anyway. Thus, this patch passes the
column locations of the markers to show_locus, and calculates the offset
there.
------------------------------------------------------------------
error7.diff:
A somewhat trivial patch. This adds a couple of comments, and does a
minor cleanup of the code I introduced to remove the blank space at the
beginning of the "[file]:[line]:" line.
------------------------------------------------------------------
error8.diff:
Also a pretty trivial patch. Aside from the comments, this adds some
cases to the part of error_print that handles '%' characters in the
format, so that an unescaped '%' at the end of the format string doesn't
create a buffer overflow -- instead, the pointer is backed up a
character so that only the '%', and not the trailing '\0', is gobbled.
Ideally, we'll never need this code, but it seems better to have it.
I would have made this an ICE, but having even the vague possibility of
an infinite recursion in the ICE code seemed a bad idea. Maybe it
would make more sense to put in code that emulates gfc_internal_error,
rather than just silently ignoring the problem, though.
Also, this should perhaps be a "default:" rather than "case '\0':", if
we error out.
------------------------------------------------------------------
error9.diff:
This modifies the "[file]:[line]:" line to look like one of the
following, depending on whether or not the loci have column information:
[file]:[line]:
[file]:[line].[column]:
[file]:[line].[column]-[column]:
This is in accordance with the GNU error-message coding standards.
A subsidiary change to this was that it was easier to _not_ use
error_print to print these lines, since the format varies. However, the
integer-printing code from error_print was useful, so I moved it off
into its own function, error_integer (by analogy to error_char and
error_string), where it probably should have been anyway.
------------------------------------------------------------------
error10.diff:
Needless to say, error9.diff breaks the testsuite. This fixes it.
------------------------------------------------------------------
fortran/ChangeLog:
2006-11-05 Brooks Moses <brooks.moses@codesourcery.com>
* error.c (show_loci): Move column-offset calculation to
show_locus.
(show_locus): Remove blank lines before "Included in"
lines, clean up code, calculate column-offsets, print
column number is error-header lines as appropriate.
(error_integer): (new function) Print integer to error
buffer.
(error_print): Use error_integer, avoid possible buffer
overflows from buggy error formats.
testsuite/ChangeLog:
2006-11-05 Brooks Moses <brooks.moses@codesourcery.com>
* lib/gfortran-dg.exp (gfortran-dg-test): Ignore column
numbers in error message headers.
------------------------------------------------------------------
Since trunk bootstrap seems to be broken, I'm regression testing this
against 4.2 at present, and it seems to be working fine so far. (It's
not quite done with the tests, but has gone quite far enough to confirm
that I haven't broken the basic error-message handling.)
Ok for trunk, once someone fixes the bootstrap, and I get a chance to
regression test it there?
- Brooks
Index: error.c
===================================================================
--- error.c (revision 118491)
+++ error.c (working copy)
@@ -114,7 +114,7 @@
}
-/* Show the file, where it was included and the source line, give a
+/* Show the file, where it was included, and the source line, give a
locus. Calls error_printf() recursively, but the recursion is at
most one level deep. */
@@ -132,9 +132,14 @@
displayed or add buffering of arbitrary number of characters in
error messages. */
+ /* Write out the error header line, giving the source file and error
+ location (in GNU standard "[file]:[line]:" format), followed by
+ an "included by" stack and a blank line. This header format is
+ matched by a testsuite parser defined in lib/gfortran-dg.exp. */
+
lb = loc->lb;
f = lb->file;
- error_printf ("%s:%d:\n", f->filename,
+ error_printf ("%s:%d:", f->filename,
#ifdef USE_MAPPED_LOCATION
LOCATION_LINE (lb->location)
#else
@@ -149,12 +154,19 @@
f = f->included_by;
if (f == NULL) break;
- error_printf (" Included at %s:%d\n", f->filename, i);
+ error_printf (" Included at %s:%d:", f->filename, i);
}
+ error_char ('\n');
+
/* Show the line itself, taking care not to print more than what can
- show up on the terminal. Tabs are converted to spaces. */
+ show up on the terminal. Tabs are converted to spaces, and
+ nonprintable characters are converted to a "\xNN" sequence. */
+ /* TODO: Although setting i to the terminal width is clever, it fails
+ to work correctly when nonprintable characters exist. A better
+ solution should be found. */
+
p = lb->line + offset;
i = strlen (p);
if (i > terminal_width)
Index: error.c
===================================================================
--- error.c (revision 118491)
+++ error.c (working copy)
@@ -209,7 +209,7 @@
static void
show_loci (locus * l1, locus * l2)
{
- int offset, flag, i, m, c1, c2, cmax;
+ int offset, i, m, c1, c2, cmax;
if (l1 == NULL || l1->lb == NULL)
{
@@ -233,6 +233,14 @@
if (l1->lb != l2->lb || m > terminal_width - 10)
goto separate;
+
+ /* At this point, we know that there will be two error loci appearing on the
+ same line of output. If the two would appear in the same column, we shift
+ '2' one column to the right, so as to print '12' rather than just '1'. */
+
+ if (c1 == c2)
+ c2 += 1;
+
offset = 0;
cmax = (c1 < c2) ? c2 : c1;
if (cmax > terminal_width - 5)
@@ -246,21 +254,13 @@
show_locus (offset, l1);
- /* Arrange that '1' and '2' will show up even if the two columns are equal. */
for (i = 1; i <= cmax; i++)
{
- flag = 0;
if (i == c1)
- {
- error_char ('1');
- flag = 1;
- }
- if (i == c2)
- {
- error_char ('2');
- flag = 1;
- }
- if (flag == 0)
+ error_char ('1');
+ else if (i == c2)
+ error_char ('2');
+ else
error_char (' ');
}
Index: error.c
===================================================================
--- error.c (revision 118491)
+++ error.c (working copy)
@@ -217,6 +217,10 @@
return;
}
+ /* While calculating parameters for printing the loci, we consider possible
+ reasons for printing one per line, and branch to "separate" in those
+ cases. */
+
c1 = l1->nextc - l1->lb->line;
c2 = 0;
if (l2 == NULL)
@@ -229,11 +233,9 @@
else
m = c1 - c2;
-
if (l1->lb != l2->lb || m > terminal_width - 10)
goto separate;
-
/* At this point, we know that there will be two error loci appearing on the
same line of output. If the two would appear in the same column, we shift
'2' one column to the right, so as to print '12' rather than just '1'. */
@@ -246,6 +248,9 @@
if (cmax > terminal_width - 5)
offset = cmax - terminal_width + 5;
+ /* TODO: Is there a good reason for the following apparently-redundant
+ check, and the similar ones in the single-locus cases below? */
+
if (offset < 0)
offset = 0;
@@ -269,34 +274,40 @@
return;
separate:
+
+ /* Print the first locus on a line by itself. */
+
offset = 0;
if (c1 > terminal_width - 5)
- {
- offset = c1 - 5;
- if (offset < 0)
- offset = 0;
- c1 = c1 - offset;
- }
+ offset = c1 - terminal_width + 5;
+
+ if (offset < 0)
+ offset = 0;
+
+ c1 = c1 - offset;
show_locus (offset, l1);
+
for (i = 1; i < c1; i++)
error_char (' ');
error_char ('1');
error_char ('\n');
+ /* Print the second locus on a line by itself, if it exists. */
+
if (l2 != NULL)
{
offset = 0;
if (c2 > terminal_width - 20)
- {
- offset = c2 - 20;
- if (offset < 0)
- offset = 0;
- c2 = c2 - offset;
- }
+ offset = c2 - terminal_width + 20;
+
+ if (offset < 0)
+ offset = 0;
+
+ c2 = c2 - offset;
show_locus (offset, l2);
Index: error.c
===================================================================
--- error.c (revision 118491)
+++ error.c (working copy)
@@ -121,12 +121,12 @@
static void error_printf (const char *, ...) ATTRIBUTE_GCC_GFC(1,2);
static void
-show_locus (int offset, locus * loc)
+show_locus (int offset, locus * loc, int c1, int c2)
{
gfc_linebuf *lb;
gfc_file *f;
char c, *p;
- int i, m;
+ int i, m, offset;
/* TODO: Either limit the total length and number of included files
displayed or add buffering of arbitrary number of characters in
@@ -159,6 +159,31 @@
error_char ('\n');
+ /* Calculate an appropriate horizontal offset of the source line in
+ order to get the error locus within the visible portion of the
+ line. Note that if the margin of 5 here is changed, the
+ corresponding margin of 10 in show_loci should be changed. */
+
+ offset = 0;
+
+ /* If the two loci would appear in the same column, we shift
+ '2' one column to the right, so as to print '12' rather than
+ just '1'. We do this here so it will be accounted for in the
+ margin calculations. */
+
+ if (c1 == c2)
+ c2 += 1;
+
+ cmax = (c1 < c2) ? c2 : c1;
+ if (cmax > terminal_width - 5)
+ offset = cmax - terminal_width + 5;
+
+ /* TODO: Is there a good reason for the following apparently-redundant
+ check, and the similar ones in the single-locus cases below? */
+
+ if (offset < 0)
+ offset = 0;
+
/* Show the line itself, taking care not to print more than what can
show up on the terminal. Tabs are converted to spaces, and
nonprintable characters are converted to a "\xNN" sequence. */
@@ -198,13 +223,32 @@
}
error_char ('\n');
+
+ /* Show the '1' and/or '2' corresponding to the column of the error
+ locus. Note that a value of -1 for c1 or c2 will simply cause
+ the relevant number not to be printed. */
+
+ c1 -= offset;
+ c2 -= offset;
+
+ for (i = 1; i <= cmax; i++)
+ {
+ if (i == c1)
+ error_char ('1');
+ else if (i == c2)
+ error_char ('2');
+ else
+ error_char (' ');
+ }
+
+ error_char ('\n');
+
}
/* As part of printing an error, we show the source lines that caused
- the problem. We show at least one, possibly two loci. If we're
- showing two loci and they both refer to the same file and line, we
- only print the line once. */
+ the problem. We show at least one, and possibly two loci; the two
+ loci may or may not be on the same source line. */
static void
show_loci (locus * l1, locus * l2)
@@ -218,13 +262,15 @@
}
/* While calculating parameters for printing the loci, we consider possible
- reasons for printing one per line, and branch to "separate" in those
- cases. */
+ reasons for printing one per line. If appropriate, print the loci
+ individually; otherwise we print them both on the same line. */
c1 = l1->nextc - l1->lb->line;
- c2 = 0;
if (l2 == NULL)
- goto separate;
+ {
+ show_locus (offset, l1, c1, -1);
+ return;
+ }
c2 = l2->nextc - l2->lb->line;
@@ -233,90 +279,20 @@
else
m = c1 - c2;
- if (l1->lb != l2->lb || m > terminal_width - 10)
- goto separate;
-
- /* At this point, we know that there will be two error loci appearing on the
- same line of output. If the two would appear in the same column, we shift
- '2' one column to the right, so as to print '12' rather than just '1'. */
-
- if (c1 == c2)
- c2 += 1;
-
- offset = 0;
- cmax = (c1 < c2) ? c2 : c1;
- if (cmax > terminal_width - 5)
- offset = cmax - terminal_width + 5;
+ /* Note that the margin value of 10 here needs to be less than the
+ margin of 5 used in the calculation of offset in show_locus. */
- /* TODO: Is there a good reason for the following apparently-redundant
- check, and the similar ones in the single-locus cases below? */
-
- if (offset < 0)
- offset = 0;
-
- c1 -= offset;
- c2 -= offset;
-
- show_locus (offset, l1);
-
- for (i = 1; i <= cmax; i++)
+ if (l1->lb != l2->lb || m > terminal_width - 10)
{
- if (i == c1)
- error_char ('1');
- else if (i == c2)
- error_char ('2');
- else
- error_char (' ');
+ show_locus (offset, l1, c1, -1);
+ show_locus (offset, l1, -1, c2);
+ return;
}
- error_char ('\n');
+ show_locus (offset, l1, c1, c2);
return;
-separate:
-
- /* Print the first locus on a line by itself. */
-
- offset = 0;
-
- if (c1 > terminal_width - 5)
- offset = c1 - terminal_width + 5;
-
- if (offset < 0)
- offset = 0;
-
- c1 = c1 - offset;
-
- show_locus (offset, l1);
-
- for (i = 1; i < c1; i++)
- error_char (' ');
-
- error_char ('1');
- error_char ('\n');
-
- /* Print the second locus on a line by itself, if it exists. */
-
- if (l2 != NULL)
- {
- offset = 0;
-
- if (c2 > terminal_width - 20)
- offset = c2 - terminal_width + 20;
-
- if (offset < 0)
- offset = 0;
-
- c2 = c2 - offset;
-
- show_locus (offset, l2);
-
- for (i = 1; i < c2; i++)
- error_char (' ');
-
- error_char ('2');
- error_char ('\n');
- }
}
Index: error.c
===================================================================
--- error.c (revision 118491)
+++ error.c (working copy)
@@ -331,8 +331,14 @@
If a locus pointer is given, the actual source line is printed out
and the column is indicated. Since we want the error message at
the bottom of any source file information, we must scan the
- argument list twice. A maximum of two locus arguments are
- permitted. */
+ argument list twice -- once to determine whether the loci are
+ present and record this for printing, and once to print the error
+ message after and loci have been printed. A maximum of two locus
+ arguments are permitted.
+
+ This function is also called (recursively) by show_locus; however,
+ as show_locus does not resupply the loci, the recursion is at most
+ one level deep. */
#define IBUF_LEN 30
#define MAX_ARGS 10
@@ -402,9 +408,12 @@
/* Show the current loci if we have to. */
if (have_l1)
show_loci (l1, l2);
- error_string (type);
+
if (*type)
- error_char (' ');
+ {
+ error_string (type);
+ error_char (' ');
+ }
have_l1 = 0;
format = format0;
Index: error.c
===================================================================
--- error.c (revision 118491)
+++ error.c (working copy)
@@ -308,7 +308,7 @@
inspired by g77's error handling and is similar to printf() with
the following %-codes:
- %c Character, %d Integer, %s String, %% Percent
+ %c Character, %d or %i Integer, %s String, %% Percent
%L Takes locus argument
%C Current locus (no argument)
@@ -321,7 +321,7 @@
arguments are permitted.
This function is also called (recursively) by show_locus; however,
- as show_locus does not resupply the loci, the recursion is at most
+ as show_locus does not resupply any loci, the recursion is at most
one level deep. */
#define IBUF_LEN 30
@@ -385,6 +385,10 @@
case 's':
cp_arg[n++] = va_arg (argp, char *);
break;
+
+ case '\0':
+ format--;
+ break;
}
}
}
@@ -426,8 +430,8 @@
error_string (cp_arg[n++]);
break;
- case 'i':
case 'd':
+ case 'i':
i = i_arg[n++];
if (i < 0)
@@ -456,6 +460,10 @@
error_string (have_l1 ? "(2)" : "(1)");
have_l1 = 1;
break;
+
+ case '\0':
+ format--;
+ break;
}
}
Index: error.c
===================================================================
--- error.c (revision 118491)
+++ error.c (working copy)
@@ -114,6 +114,37 @@
}
+/* Print a formatted integer to the error buffer or output. */
+
+#define IBUF_LEN 30
+
+static void
+error_integer (int i)
+{
+ char *p, int_buf[IBUF_LEN];
+
+ if (i < 0)
+ {
+ i = -i;
+ error_char ('-');
+ }
+
+ p = int_buf + IBUF_LEN - 1;
+ *p-- = '\0';
+
+ if (i == 0)
+ *p-- = '0';
+
+ while (i > 0)
+ {
+ *p-- = i % 10 + '0';
+ i = i / 10;
+ }
+
+ error_string (p + 1);
+}
+
+
/* Show the file, where it was included, and the source line, give a
locus. Calls error_printf() recursively, but the recursion is at
most one level deep. */
@@ -133,19 +164,37 @@
error messages. */
/* Write out the error header line, giving the source file and error
- location (in GNU standard "[file]:[line]:" format), followed by
- an "included by" stack and a blank line. This header format is
- matched by a testsuite parser defined in lib/gfortran-dg.exp. */
+ location (in GNU standard "[file]:[line].[column]:" format),
+ followed by an "included by" stack and a blank line. This header
+ format is matched by a testsuite parser defined in
+ lib/gfortran-dg.exp. */
lb = loc->lb;
f = lb->file;
- error_printf ("%s:%d:", f->filename,
+
+ error_string (f->filename);
+ error_char (':');
+
#ifdef USE_MAPPED_LOCATION
- LOCATION_LINE (lb->location)
+ error_integer (LOCATION_LINE (lb->location));
#else
- lb->linenum
+ error_integer (lb->linenum);
#endif
- );
+
+ if ((c1 > 0) || (c2 > 0))
+ error_char ('.');
+
+ if (c1 > 0)
+ error_integer (c1);
+
+ if ((c1 > 0) && (c2 > 0))
+ error_char ('-');
+
+ if (c2 > 0)
+ error_integer (c2);
+
+ error_char (':');
+ error_char ('\n');
for (;;)
{
@@ -320,18 +369,17 @@
message after and loci have been printed. A maximum of two locus
arguments are permitted.
- This function is also called (recursively) by show_locus; however,
- as show_locus does not resupply any loci, the recursion is at most
- one level deep. */
+ This function is also called (recursively) by show_locus in the
+ case of included files; however, as show_locus does not resupply
+ any loci, the recursion is at most one level deep. */
-#define IBUF_LEN 30
#define MAX_ARGS 10
static void ATTRIBUTE_GCC_GFC(2,0)
error_print (const char *type, const char *format0, va_list argp)
{
- char c, *p, int_buf[IBUF_LEN], c_arg[MAX_ARGS], *cp_arg[MAX_ARGS];
- int i, n, have_l1, i_arg[MAX_ARGS];
+ char c, c_arg[MAX_ARGS], *cp_arg[MAX_ARGS];
+ int n, have_l1, i_arg[MAX_ARGS];
locus *l1, *l2, *loc;
const char *format;
@@ -432,27 +480,7 @@
case 'd':
case 'i':
- i = i_arg[n++];
-
- if (i < 0)
- {
- i = -i;
- error_char ('-');
- }
-
- p = int_buf + IBUF_LEN - 1;
- *p-- = '\0';
-
- if (i == 0)
- *p-- = '0';
-
- while (i > 0)
- {
- *p-- = i % 10 + '0';
- i = i / 10;
- }
-
- error_string (p + 1);
+ error_integer (i_arg[n++]);
break;
case 'C': /* Current locus */
Index: testsuite/lib/gfortran-dg.exp
===================================================================
--- testsuite/lib/gfortran-dg.exp (revision 118491)
+++ testsuite/lib/gfortran-dg.exp (working copy)
@@ -26,28 +26,30 @@
set output_file [lindex $result 1]
# gfortran error messages look like this:
- # [name]:[line]:
+ # [name]:[locus]:
#
# some code
# 1
# Error: Some error at (1)
# or
- # [name]:[line]:
+ # [name]:[locus]:
#
# some code
# 1
- # [name]:[line2]:
+ # [name]:[locus2]:
#
# some other code
# 2
# Error: Some error at (1) and (2)
# or
- # [name]:[line]:
+ # [name]:[locus]:
#
# some code and some more code
# 1 2
# Error: Some error at (1) and (2)
#
+ # Where [locus] is either [line] or [line].[columns] .
+ #
# We collapse these to look like:
# [name]:[line]: Error: Some error at (1) and (2)
# or
@@ -59,13 +61,13 @@
# Note that these regexps only make sense in the combinations used below.
# Note also that is imperative that we first deal with the form with
# two loci.
- set locus_regexp "(\[^\n\]*):\n\n\[^\n\]*\n\[^\n\]*\n"
+ set locus_regexp "(\[^\n\]*):(\[0-9\]*)\[^\n\]*:\n\n\[^\n\]*\n\[^\n\]*\n"
set diag_regexp "(\[^\n\]*)\n"
set two_loci "$locus_regexp$locus_regexp$diag_regexp"
set single_locus "$locus_regexp$diag_regexp"
- regsub -all $two_loci $comp_output "\\1: \\3\n\\2: \\3\n" comp_output
- regsub -all $single_locus $comp_output "\\1: \\2\n" comp_output
+ regsub -all $two_loci $comp_output "\\1:\\2: \\5\n\\3:\\4: \\5\n" comp_output
+ regsub -all $single_locus $comp_output "\\1:\\2: \\3\n" comp_output
return [list $comp_output $output_file]
}